| Message ID | Y4fH4rhcSztHwKvK@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-compat-util: avoid redefining system function names | expand |
On Wed, Nov 30, 2022 at 04:15:14PM -0500, Jeff King wrote: > -- >8 -- > Subject: [PATCH] git-compat-util: avoid redefining system function names > > Our git-compat-util header defines a few noop wrappers for system > functions if they are not available. This was originally done with a > macro, but in 15b52a44e0 (compat-util: type-check parameters of no-op > replacement functions, 2020-08-06) we switched to inline functions, > because it gives us basic type-checking. > > This can cause compilation failures when the system _does_ declare those > functions but we choose not to use them, since the compiler will > complain about the redeclaration. This was seen in the real world when > compiling against certain builds of uclibc, which may leave > _POSIX_THREAD_SAFE_FUNCTIONS unset, but still declare flockfile() and > funlockfile(). > > It can also be seen on any platform that has setitimer() if you choose > to compile without it (which plausibly could happen if the system > implementation is buggy). E.g., on Linux: > > $ make NO_SETITIMER=IWouldPreferNotTo git.o > CC git.o > In file included from builtin.h:4, > from git.c:1: > git-compat-util.h:344:19: error: conflicting types for ‘setitimer’; have ‘int(int, const struct itimerval *, struct itimerval *)’ > 344 | static inline int setitimer(int which UNUSED, > | ^~~~~~~~~ > In file included from git-compat-util.h:234: > /usr/include/x86_64-linux-gnu/sys/time.h:155:12: note: previous declaration of ‘setitimer’ with type ‘int(__itimer_which_t, const struct itimerval * restrict, struct itimerval * restrict)’ > 155 | extern int setitimer (__itimer_which_t __which, > | ^~~~~~~~~ > make: *** [Makefile:2714: git.o] Error 1 > > Here I think the compiler is complaining about the lack of "restrict" > annotations in our version, but even if we matched it completely (and > there is no way to match all platforms anyway), it would still complain > about a static declaration following a non-static one. Using macros > doesn't have this problem, because the C preprocessor rewrites the name > in our code before we hit this level of compilation. > > One way to fix this would just be to revert most of 15b52a44e0. What we > really cared about there was catching build problems with > precompose_argv(), which most platforms _don't_ build, and which is our > custom function. So we could just switch the system wrappers back to > macros; most people build the real versions anyway, and they don't > change. So the extra type-checking isn't likely to catch bugs. > > But with a little work, we can have our cake and eat it, too. If we > define the type-checking wrappers with a unique name, and then redirect > the system names to them with macros, we still get our type checking, > but without redeclaring the system function names. > > Signed-off-by: Jeff King <peff@peff.net> > --- > I confirmed that this builds on Linux with NO_SETITIMER, and still > catches type problems if you intentionally break one of the callers. > > Technically these should probably all have "#undef flockfile" and so on, > but we've never done that, and we haven't seen an actual platform that > complains. So I didn't include that here. I don't mind if somebody wants > to, but it should be a separate patch on top. > > git-compat-util.h | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index a76d0526f7..83ec7b7941 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -341,11 +341,12 @@ struct itimerval { > #endif > > #ifdef NO_SETITIMER > -static inline int setitimer(int which UNUSED, > - const struct itimerval *value UNUSED, > - struct itimerval *newvalue UNUSED) { > +static inline int git_setitimer(int which UNUSED, > + const struct itimerval *value UNUSED, > + struct itimerval *newvalue UNUSED) { > return 0; /* pretend success */ > } > +#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) > #endif > > #ifndef NO_LIBGEN_H > @@ -1471,14 +1472,16 @@ int open_nofollow(const char *path, int flags); > #endif > > #ifndef _POSIX_THREAD_SAFE_FUNCTIONS > -static inline void flockfile(FILE *fh UNUSED) > +static inline void git_flockfile(FILE *fh UNUSED) > { > ; /* nothing */ > } > -static inline void funlockfile(FILE *fh UNUSED) > +static inline void git_funlockfile(FILE *fh UNUSED) > { > ; /* nothing */ > } > +#define flockfile(fh) git_flockfile(fh) > +#define funlockfile(fh) git_funlockfile(fh) > #define getc_unlocked(fh) getc(fh) > #endif > Hi Jeff, I got many of redefinition warnings when cross-compiling on Buildroot with the patch above, like: In file included from cache.h:4, from common-main.c:1: git-compat-util.h:1485: warning: "getc_unlocked" redefined 1485 | #define getc_unlocked(fh) getc(fh) | In file included from git-compat-util.h:216, from cache.h:4, from common-main.c:1: /home/bagas/repo/buildroot/output/host/aarch64-buildroot-linux-uclibc/sysroot/usr/include/stdio.h:835: note: this is the location of the previous definition 835 | #define getc_unlocked(_fp) __GETC_UNLOCKED(_fp) Thanks.
On Fri, Dec 02, 2022 at 05:05:14PM +0700, Bagas Sanjaya wrote: > I got many of redefinition warnings when cross-compiling on Buildroot > with the patch above, like: > > In file included from cache.h:4, > from common-main.c:1: > git-compat-util.h:1485: warning: "getc_unlocked" redefined > 1485 | #define getc_unlocked(fh) getc(fh) > | > In file included from git-compat-util.h:216, > from cache.h:4, > from common-main.c:1: > /home/bagas/repo/buildroot/output/host/aarch64-buildroot-linux-uclibc/sysroot/usr/include/stdio.h:835: note: this is the location of the previous definition > 835 | #define getc_unlocked(_fp) __GETC_UNLOCKED(_fp) I imagine you'd get that without my patch, too, since I didn't touch the getc_unlocked() line at all. Or maybe it simply didn't get that far because of the other redeclared functions. Anyway, we probably want this on top of the other patch. -- >8 -- Subject: [PATCH] git-compat-util: undefine system names before redeclaring them When we define a macro to point a system function (e.g., flockfile) to our custom wrapper, we should make sure that the system did not already define it as a macro. This is rarely a problem, but can cause compilation failures if both of these are true: - we decide to define our own wrapper even though the system provides the function; we know this happens at least with uclibc, which may declare flockfile, etc, without _POSIX_THREAD_SAFE_FUNCTIONS - the system version is declared as a macro; we know this happens at least with uclibc's version of getc_unlocked() So just handling getc_unlocked() would be sufficient to deal with the real-world case we've seen. But since it's easy to do, we may as well be defensive about the other macro wrappers added in the previous patch. Signed-off-by: Jeff King <peff@peff.net> --- There may be other similar cases lurking throughout the code base, but I don't think it's worth anybody's time to go looking for them. If one of them triggers on a real platform, we can deal with it then. git-compat-util.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 83ec7b7941..76e4b11131 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -346,6 +346,7 @@ static inline int git_setitimer(int which UNUSED, struct itimerval *newvalue UNUSED) { return 0; /* pretend success */ } +#undef setitimer #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) #endif @@ -1480,6 +1481,9 @@ static inline void git_funlockfile(FILE *fh UNUSED) { ; /* nothing */ } +#undef flockfile +#undef funlockfile +#undef getc_unlocked #define flockfile(fh) git_flockfile(fh) #define funlockfile(fh) git_funlockfile(fh) #define getc_unlocked(fh) getc(fh)
On Wed, Nov 30 2022, Jeff King wrote: > On Mon, Nov 28, 2022 at 12:01:35AM -0500, Jeff King wrote: > >> All this points to 15b52a44e0 being a bit too loose with its >> assumptions. It is assuming that if the posix flag is not defined, we >> are free to use those system names. But here's an example where that is >> not true. And the only way around it is with a macro, which is what we >> had before that commit. >> >> So I think we'd want to revert the flockfile() bits of that commit. And >> I'd guess setitimer is in the same boat (the system may declare it, but >> for whatever reason somebody may choose not to use it by feeding >> NO_SETITIMER to make, at which point the compiler will complain about >> the duplicate declaration. > > After sleeping on this, here's a best-of-both-worlds solution. > > Junio: this is perhaps maint material in the long run, but the breakage > goes back to v2.29.0, so definitely not urgent for the release period. > Note that if you go farther back than what will become maint-2.39, > there's a minor textual conflict around the UNUSED markers. > > -- >8 -- > Subject: [PATCH] git-compat-util: avoid redefining system function names > > Our git-compat-util header defines a few noop wrappers for system > functions if they are not available. This was originally done with a > macro, but in 15b52a44e0 (compat-util: type-check parameters of no-op > replacement functions, 2020-08-06) we switched to inline functions, > because it gives us basic type-checking. > > This can cause compilation failures when the system _does_ declare those > functions but we choose not to use them, since the compiler will > complain about the redeclaration. This was seen in the real world when > compiling against certain builds of uclibc, which may leave > _POSIX_THREAD_SAFE_FUNCTIONS unset, but still declare flockfile() and > funlockfile(). > > It can also be seen on any platform that has setitimer() if you choose > to compile without it (which plausibly could happen if the system > implementation is buggy). E.g., on Linux: > > $ make NO_SETITIMER=IWouldPreferNotTo git.o > CC git.o > In file included from builtin.h:4, > from git.c:1: > git-compat-util.h:344:19: error: conflicting types for ‘setitimer’; have ‘int(int, const struct itimerval *, struct itimerval *)’ > 344 | static inline int setitimer(int which UNUSED, > | ^~~~~~~~~ > In file included from git-compat-util.h:234: > /usr/include/x86_64-linux-gnu/sys/time.h:155:12: note: previous declaration of ‘setitimer’ with type ‘int(__itimer_which_t, const struct itimerval * restrict, struct itimerval * restrict)’ > 155 | extern int setitimer (__itimer_which_t __which, > | ^~~~~~~~~ > make: *** [Makefile:2714: git.o] Error 1 > > Here I think the compiler is complaining about the lack of "restrict" > annotations in our version, but even if we matched it completely (and > there is no way to match all platforms anyway), it would still complain > about a static declaration following a non-static one. Using macros > doesn't have this problem, because the C preprocessor rewrites the name > in our code before we hit this level of compilation. ... > One way to fix this would just be to revert most of 15b52a44e0. What we > really cared about there was catching build problems with > precompose_argv(), which most platforms _don't_ build, and which is our > custom function. So we could just switch the system wrappers back to > macros; most people build the real versions anyway, and they don't > change. So the extra type-checking isn't likely to catch bugs. > > But with a little work, we can have our cake and eat it, too. If we > define the type-checking wrappers with a unique name, and then redirect > the system names to them with macros, we still get our type checking, > but without redeclaring the system function names. This looks good to me. The only thing I'd like to note is that while the above explanation might read as though this is something novel, it's really just doing exactly what we're already doing for e.g. git_vsnprintf: $ git -P grep git_snprintf compat/snprintf.c:int git_snprintf(char *str, size_t maxsize, const char *format, ...) git-compat-util.h:#define snprintf git_snprintf git-compat-util.h:int git_snprintf(char *str, size_t maxsize, Now, that's not a downside here but an upside, plain old boring and using existing precedence is a goood thing. Except that.... > Signed-off-by: Jeff King <peff@peff.net> > --- > I confirmed that this builds on Linux with NO_SETITIMER, and still > catches type problems if you intentionally break one of the callers. > > Technically these should probably all have "#undef flockfile" and so on, > but we've never done that, and we haven't seen an actual platform that > complains. So I didn't include that here. I don't mind if somebody wants > to, but it should be a separate patch on top. > > git-compat-util.h | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index a76d0526f7..83ec7b7941 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -341,11 +341,12 @@ struct itimerval { > #endif > > #ifdef NO_SETITIMER > -static inline int setitimer(int which UNUSED, > - const struct itimerval *value UNUSED, > - struct itimerval *newvalue UNUSED) { > +static inline int git_setitimer(int which UNUSED, > + const struct itimerval *value UNUSED, > + struct itimerval *newvalue UNUSED) { > return 0; /* pretend success */ > } > +#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) ...this part is where we differ from the existing pattern. I don't think it matters except for the redundant verbosity, but as we're not re-arranging the parameters here, why not simply: #define setitimer git_setitimer ? > +#define flockfile(fh) git_flockfile(fh) > +#define funlockfile(fh) git_funlockfile(fh) > #define getc_unlocked(fh) getc(fh) > #endif Ditto. I went looking a bit more, and we also have existing examples of these sort of macros, but could probably have this on top of "next" if we care to make these consistent. This leaves only the /git.*\(/ macros whose parameters we need to alter as defining parameters: diff --git a/git-compat-util.h b/git-compat-util.h index 83ec7b7941e..87a16d8cfcd 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -346,7 +346,7 @@ static inline int git_setitimer(int which UNUSED, struct itimerval *newvalue UNUSED) { return 0; /* pretend success */ } -#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) +#define setitimer git_setitimer #endif #ifndef NO_LIBGEN_H @@ -546,7 +546,7 @@ static inline int git_has_dir_sep(const char *path) { return !!strchr(path, '/'); } -#define has_dir_sep(path) git_has_dir_sep(path) +#define has_dir_sep git_has_dir_sep #endif #ifndef query_user_email @@ -831,17 +831,17 @@ int git_munmap(void *start, size_t length); #ifdef stat #undef stat #endif -#define stat(path, buf) git_stat(path, buf) +#define stat git_stat int git_stat(const char *, struct stat *); #ifdef fstat #undef fstat #endif -#define fstat(fd, buf) git_fstat(fd, buf) +#define fstat git_fstat int git_fstat(int, struct stat *); #ifdef lstat #undef lstat #endif -#define lstat(path, buf) git_lstat(path, buf) +#define lstat git_lstat int git_lstat(const char *, struct stat *); #endif @@ -923,7 +923,7 @@ char *gitstrdup(const char *s); # ifdef fopen # undef fopen # endif -# define fopen(a,b) git_fopen(a,b) +# define fopen git_fopen # endif FILE *git_fopen(const char*, const char*); #endif @@ -1480,16 +1480,16 @@ static inline void git_funlockfile(FILE *fh UNUSED) { ; /* nothing */ } -#define flockfile(fh) git_flockfile(fh) -#define funlockfile(fh) git_funlockfile(fh) -#define getc_unlocked(fh) getc(fh) +#define flockfile git_flockfile +#define funlockfile git_funlockfile +#define getc_unlocked getc #endif #ifdef FILENO_IS_A_MACRO int git_fileno(FILE *stream); # ifndef COMPAT_CODE_FILENO # undef fileno -# define fileno(p) git_fileno(p) +# define fileno git_fileno # endif #endif @@ -1499,7 +1499,7 @@ int git_access(const char *path, int mode); # ifdef access # undef access # endif -# define access(path, mode) git_access(path, mode) +# define access git_access # endif #endif
On Fri, Dec 02, 2022 at 12:31:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > > But with a little work, we can have our cake and eat it, too. If we > > define the type-checking wrappers with a unique name, and then redirect > > the system names to them with macros, we still get our type checking, > > but without redeclaring the system function names. > > This looks good to me. The only thing I'd like to note is that while the > above explanation might read as though this is something novel, it's > really just doing exactly what we're already doing for > e.g. git_vsnprintf: > > $ git -P grep git_snprintf > compat/snprintf.c:int git_snprintf(char *str, size_t maxsize, const char *format, ...) > git-compat-util.h:#define snprintf git_snprintf > git-compat-util.h:int git_snprintf(char *str, size_t maxsize, > > Now, that's not a downside here but an upside, plain old boring and > using existing precedence is a goood thing. Except that.... Yes, though the motivation here is just a tiny bit different. In the case of snprintf, say, the reason we intercept it is that we know the platform version sucks and we want to replace it. With the functions touched by 15b52a44e0, the idea was that we're replacing them because the platform doesn't provide them at all, and so macros weren't needed. But that assumption turns out sometimes not to be true. > > +#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) > > ...this part is where we differ from the existing pattern. I don't think > it matters except for the redundant verbosity, but as we're not > re-arranging the parameters here, why not simply: > > #define setitimer git_setitimer The two aren't quite the same. Try this: -- >8 -- gcc -E - <<-\EOF #define no_parens replaced #define with_parens(x) replaced(x) struct foo { no_parens x; with_parens x; }; no_parens(foo); with_parens(foo); EOF -- 8< -- If the macro is defined with parentheses, it's replaced only in function-call contexts. Whereas without it, any token is replaced. I doubt it matters much in the real world either way. Replacing only in function context seems to match the intent a bit more, though note that if you tried to take a function pointer to a macro-replaced name, you'd get the original. As you saw, there are many examples of each style, and I don't think we've ever expressed a preference for one or the other. Note that for some, like snprintf, we traditionally _had_ to use the non-function form because we couldn't count on handling varargs correctly. In the opposite direction, many macros have to use the function form because they modify the arguments (e.g., foo(x) pointing to repo_foo(the_repository, x)). > I went looking a bit more, and we also have existing examples of these > sort of macros, but could probably have this on top of "next" if we care > to make these consistent. I don't have a strong feeling either way. I think you'd need to argue in the commit message why one form is better than the other. The function pointer thing is probably the most compelling to me. -Peff
On Fri, Dec 02, 2022 at 06:05:38AM -0500, Jeff King wrote: > -- >8 -- > Subject: [PATCH] git-compat-util: undefine system names before redeclaring > them > > When we define a macro to point a system function (e.g., flockfile) to > our custom wrapper, we should make sure that the system did not already > define it as a macro. This is rarely a problem, but can cause > compilation failures if both of these are true: > > - we decide to define our own wrapper even though the system provides > the function; we know this happens at least with uclibc, which may > declare flockfile, etc, without _POSIX_THREAD_SAFE_FUNCTIONS > > - the system version is declared as a macro; we know this happens at > least with uclibc's version of getc_unlocked() > > So just handling getc_unlocked() would be sufficient to deal with the > real-world case we've seen. But since it's easy to do, we may as well be > defensive about the other macro wrappers added in the previous patch. > > Signed-off-by: Jeff King <peff@peff.net> > --- > There may be other similar cases lurking throughout the code base, but I > don't think it's worth anybody's time to go looking for them. If one of > them triggers on a real platform, we can deal with it then. > > git-compat-util.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/git-compat-util.h b/git-compat-util.h > index 83ec7b7941..76e4b11131 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -346,6 +346,7 @@ static inline int git_setitimer(int which UNUSED, > struct itimerval *newvalue UNUSED) { > return 0; /* pretend success */ > } > +#undef setitimer > #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) > #endif > > @@ -1480,6 +1481,9 @@ static inline void git_funlockfile(FILE *fh UNUSED) > { > ; /* nothing */ > } > +#undef flockfile > +#undef funlockfile > +#undef getc_unlocked > #define flockfile(fh) git_flockfile(fh) > #define funlockfile(fh) git_funlockfile(fh) > #define getc_unlocked(fh) getc(fh) The warnings gone away, thanks! For this patch and the previous one [1], Tested-by: Bagas Sanjaya <bagasdotme@gmail.com> [1]: https://lore.kernel.org/git/Y4fH4rhcSztHwKvK@coredump.intra.peff.net/
On Sat, Dec 03, 2022 at 03:02:05PM +0700, Bagas Sanjaya wrote: > On Fri, Dec 02, 2022 at 06:05:38AM -0500, Jeff King wrote: > > -- >8 -- > > Subject: [PATCH] git-compat-util: undefine system names before redeclaring > > them > > > > When we define a macro to point a system function (e.g., flockfile) to > > our custom wrapper, we should make sure that the system did not already > > define it as a macro. This is rarely a problem, but can cause > > compilation failures if both of these are true: > > > > - we decide to define our own wrapper even though the system provides > > the function; we know this happens at least with uclibc, which may > > declare flockfile, etc, without _POSIX_THREAD_SAFE_FUNCTIONS > > > > - the system version is declared as a macro; we know this happens at > > least with uclibc's version of getc_unlocked() > > > > So just handling getc_unlocked() would be sufficient to deal with the > > real-world case we've seen. But since it's easy to do, we may as well be > > defensive about the other macro wrappers added in the previous patch. > > > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > There may be other similar cases lurking throughout the code base, but I > > don't think it's worth anybody's time to go looking for them. If one of > > them triggers on a real platform, we can deal with it then. > > > > git-compat-util.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/git-compat-util.h b/git-compat-util.h > > index 83ec7b7941..76e4b11131 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -346,6 +346,7 @@ static inline int git_setitimer(int which UNUSED, > > struct itimerval *newvalue UNUSED) { > > return 0; /* pretend success */ > > } > > +#undef setitimer > > #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) > > #endif > > > > @@ -1480,6 +1481,9 @@ static inline void git_funlockfile(FILE *fh UNUSED) > > { > > ; /* nothing */ > > } > > +#undef flockfile > > +#undef funlockfile > > +#undef getc_unlocked > > #define flockfile(fh) git_flockfile(fh) > > #define funlockfile(fh) git_funlockfile(fh) > > #define getc_unlocked(fh) getc(fh) > > The warnings gone away, thanks! > > For this patch and the previous one [1], > > Tested-by: Bagas Sanjaya <bagasdotme@gmail.com> > > [1]: https://lore.kernel.org/git/Y4fH4rhcSztHwKvK@coredump.intra.peff.net/ > PING? Is your patch ready for v2.39?
On Wed, Dec 07, 2022 at 03:33:59PM +0700, Bagas Sanjaya wrote:
> Is your patch ready for v2.39?
It won't be in v2.39.0, as it came in during the release freeze, and
isn't fixing a regression during this cycle. Both patches are in 'next',
though, and will likely be part of the next maint release (2.39.1).
-Peff
diff --git a/git-compat-util.h b/git-compat-util.h index a76d0526f7..83ec7b7941 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -341,11 +341,12 @@ struct itimerval { #endif #ifdef NO_SETITIMER -static inline int setitimer(int which UNUSED, - const struct itimerval *value UNUSED, - struct itimerval *newvalue UNUSED) { +static inline int git_setitimer(int which UNUSED, + const struct itimerval *value UNUSED, + struct itimerval *newvalue UNUSED) { return 0; /* pretend success */ } +#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue) #endif #ifndef NO_LIBGEN_H @@ -1471,14 +1472,16 @@ int open_nofollow(const char *path, int flags); #endif #ifndef _POSIX_THREAD_SAFE_FUNCTIONS -static inline void flockfile(FILE *fh UNUSED) +static inline void git_flockfile(FILE *fh UNUSED) { ; /* nothing */ } -static inline void funlockfile(FILE *fh UNUSED) +static inline void git_funlockfile(FILE *fh UNUSED) { ; /* nothing */ } +#define flockfile(fh) git_flockfile(fh) +#define funlockfile(fh) git_funlockfile(fh) #define getc_unlocked(fh) getc(fh) #endif