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@xxxxxxxx>
> ---
> 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.
--
An old man doll... just what I always wanted! - Clara
