[PATCH] git-compat-util.h: Fix build without threads

[PATCH] git-compat-util.h: Fix build without threads

[Bagas Sanjaya]

From: Fabrice Fontaine <fontaine.fabrice@gmail.com>

Git build with toolchains without threads support is broken (as reported
by Buildroot autobuilder [1]) since version 2.29.0, which traces back to
15b52a44e0 (compat-util: type-check parameters of no-op replacement
functions, 2020-08-06):

In file included from cache.h:4,
                 from blame.c:1:
git-compat-util.h:1238:20: error: static declaration of 'flockfile' follows non-static declaration
 static inline void flockfile(FILE *fh)
                    ^~~~~~~~~
In file included from git-compat-util.h:168,
                 from cache.h:4,
                 from blame.c:1:
/nvme/rc-buildroot-test/scripts/instance-0/output-1/host/arm-buildroot-linux-uclibcgnueabihf/sysroot/usr/include/stdio.h:806:13: note: previous declaration of 'flockfile' was here
 extern void flockfile (FILE *__stream) __THROW;
             ^~~~~~~~~
In file included from cache.h:4,
                 from blame.c:1:
git-compat-util.h:1242:20: error: static declaration of 'funlockfile' follows non-static declaration
 static inline void funlockfile(FILE *fh)
                    ^~~~~~~~~~~
In file included from git-compat-util.h:168,
                 from cache.h:4,
                 from blame.c:1:
/nvme/rc-buildroot-test/scripts/instance-0/output-1/host/arm-buildroot-linux-uclibcgnueabihf/sysroot/usr/include/stdio.h:813:13: note: previous declaration of 'funlockfile' was here
 extern void funlockfile (FILE *__stream) __THROW;
             ^~~~~~~~~~~

To avoid this build failure, check if flockfile is available before
defining flockfile, funlockfile and getc_unlocked.

Link: http://autobuild.buildroot.org/results/d41638d1ad8e78dd6f654367c905996b838ee649 [1]
Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
[Bagas: Rebase to current main, resolve minor conflicts, and slight rewording]
Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Makefile          | 5 +++++
 configure.ac      | 6 ++++++
 git-compat-util.h | 3 ++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b258fdbed8..2a8831a9ad 100644
--- a/Makefile
+++ b/Makefile
@@ -149,6 +149,8 @@ include shared.mak
 # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
 # This also implies NO_SETITIMER
 #
+# Define NO_FLOCKFILE if you don't have flockfile()
+#
 # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
 # generally faster on your platform than accessing the working directory.
 #
@@ -1826,6 +1828,9 @@ endif
 ifdef NO_SETITIMER
 	COMPAT_CFLAGS += -DNO_SETITIMER
 endif
+ifdef NO_FLOCKFILE
+	COMPAT_CFLAGS += -DNO_FLOCKFILE
+endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/configure.ac b/configure.ac
index 38ff86678a..3a4c230529 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1094,6 +1094,12 @@ GIT_CHECK_FUNC(setitimer,
 [NO_SETITIMER=YesPlease])
 GIT_CONF_SUBST([NO_SETITIMER])
 #
+# Define NO_FLOCKFILE if you don't have flockfile.
+GIT_CHECK_FUNC(flockfile,
+[NO_FLOCKFILE=],
+[NO_FLOCKFILE=YesPlease])
+GIT_CONF_SUBST([NO_FLOCKFILE])
+#
 # Define NO_STRCASESTR if you don't have strcasestr.
 GIT_CHECK_FUNC(strcasestr,
 [NO_STRCASESTR=],
diff --git a/git-compat-util.h b/git-compat-util.h
index a76d0526f7..034f564614 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1470,7 +1470,8 @@ int open_nofollow(const char *path, int flags);
 # define SHELL_PATH "/bin/sh"
 #endif
 
-#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+
+#if !defined(_POSIX_THREAD_SAFE_FUNCTIONS) && defined(NO_FLOCKFILE)
 static inline void flockfile(FILE *fh UNUSED)
 {
 	; /* nothing */

base-commit: c000d916380bb59db69c78546928eadd076b9c7d
-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] git-compat-util.h: Fix build without threads

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 25 2022, Bagas Sanjaya wrote:

> From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>
> Git build with toolchains without threads support is broken (as reported
> by Buildroot autobuilder [1]) since version 2.29.0, which traces back to

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1470,7 +1470,8 @@ int open_nofollow(const char *path, int flags);
>  # define SHELL_PATH "/bin/sh"
>  #endif
>  
> -#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
> +
> +#if !defined(_POSIX_THREAD_SAFE_FUNCTIONS) && defined(NO_FLOCKFILE)

Per f43cce23add (git-compat-util: add fallbacks for unlocked stdio,
2015-04-16) wouldn't it make more sense to do something like:

#ifdef NO_FLOCKFILE
#undef _POSIX_THREAD_SAFE_FUNCTIONS
#endif

Or the other way around here? I.e. have _POSIX_THREAD_SAFE_FUNCTIONS
define/undefine NO_FLOCKFILE?

Re: [PATCH] git-compat-util.h: Fix build without threads

[Jeff King]

On Fri, Nov 25, 2022 at 04:23:39PM +0700, Bagas Sanjaya wrote:

> From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> 
> Git build with toolchains without threads support is broken (as reported
> by Buildroot autobuilder [1]) since version 2.29.0, which traces back to
> 15b52a44e0 (compat-util: type-check parameters of no-op replacement
> functions, 2020-08-06):
> 
> In file included from cache.h:4,
>                  from blame.c:1:
> git-compat-util.h:1238:20: error: static declaration of 'flockfile' follows non-static declaration
>  static inline void flockfile(FILE *fh)
>                     ^~~~~~~~~
> In file included from git-compat-util.h:168,
>                  from cache.h:4,
>                  from blame.c:1:
> /nvme/rc-buildroot-test/scripts/instance-0/output-1/host/arm-buildroot-linux-uclibcgnueabihf/sysroot/usr/include/stdio.h:806:13: note: previous declaration of 'flockfile' was here

We'll only compile our flockfile wrapper if _POSIX_THREAD_SAFE_FUNCTIONS
isn't set. So this is a platform where flockfile() is declared in a
header, but that flag is not defined.

If flockfile() really is available, I think a better fix here is to
figure out why the posix flag isn't set, or to set it manually, so that
we use the system flockfile(). That would give better performance.

From the filename, I'm assuming it's uclibc. And poking at the uclibc
source, it looks like the flag is sometimes set, but may be unset if
__UCLIBC_HAS_THREADS__ isn't set. And flockfile() is defined regardless.
So it may be correctly telling us that there's no thread support, but
still declares flockfile() anyway. Which is a little weird, but I think
not strictly violating POSIX.

If that's the case, then the patch here seems like the wrong thing.
We'll avoid the extra noop declaration of flockfile() in the header
which is blocking your compilation, but we'll still try to call
flockfile() in config.c. That will try to call the system version that
the headers told us should not be used. How will it behave? Maybe OK,
maybe not, depending on the platform.


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.

There's more discussion in the original thread:

  https://lore.kernel.org/git/20200807032723.GA15119@coredump.intra.peff.net/

Junio said "we'll cross that bridge when we need to port to such a
system". I guess that time is now. ;)

In theory we should also #undef all of the macros we're replacing, in
case the platform implements them as macros. I'm OK to wait on that
until we see such a system, though (I was mildly surprised that the
compiler didn't also complain about getc_unlocked(), because I believe
that it can be a macro, but it looks like it isn't in uclibc).

-Peff

Re: [PATCH] git-compat-util.h: Fix build without threads

[Jeff King]

On Sat, Nov 26, 2022 at 12:47:27AM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Nov 25 2022, Bagas Sanjaya wrote:
> 
> > From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> >
> > Git build with toolchains without threads support is broken (as reported
> > by Buildroot autobuilder [1]) since version 2.29.0, which traces back to
> 
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -1470,7 +1470,8 @@ int open_nofollow(const char *path, int flags);
> >  # define SHELL_PATH "/bin/sh"
> >  #endif
> >  
> > -#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
> > +
> > +#if !defined(_POSIX_THREAD_SAFE_FUNCTIONS) && defined(NO_FLOCKFILE)
> 
> Per f43cce23add (git-compat-util: add fallbacks for unlocked stdio,
> 2015-04-16) wouldn't it make more sense to do something like:
> 
> #ifdef NO_FLOCKFILE
> #undef _POSIX_THREAD_SAFE_FUNCTIONS
> #endif

That doesn't work, because the NO_FLOCKFILE is actually overriding the
_lack_ of _POSIX_THREAD_SAFE_FUNCTIONS. So it's kind of confusingly
named. In this patch, NO_FLOCKFILE means "do not define a noop wrapper
flockfile()", which can only work if the system really does define it.

I do think this patch is doing the wrong thing, though. See my other
reply.

-Peff

Re: [PATCH] git-compat-util.h: Fix build without threads

[Bagas Sanjaya]

On 11/26/22 06:47, Ævar Arnfjörð Bjarmason wrote:
> Per f43cce23add (git-compat-util: add fallbacks for unlocked stdio,
> 2015-04-16) wouldn't it make more sense to do something like:
> 
> #ifdef NO_FLOCKFILE
> #undef _POSIX_THREAD_SAFE_FUNCTIONS
> #endif
> 
> Or the other way around here? I.e. have _POSIX_THREAD_SAFE_FUNCTIONS
> define/undefine NO_FLOCKFILE?

From the commit you mentioned, I think that above is OK. However,
because I'm no C expert, I'm unsure whether I should go with #undef
suggestion alone or #undef following by no-op declaration below #endif.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] git-compat-util.h: Fix build without threads

[Bagas Sanjaya]

On 11/29/22 10:30, Bagas Sanjaya wrote:
> On 11/26/22 06:47, Ævar Arnfjörð Bjarmason wrote:
>> Per f43cce23add (git-compat-util: add fallbacks for unlocked stdio,
>> 2015-04-16) wouldn't it make more sense to do something like:
>>
>> #ifdef NO_FLOCKFILE
>> #undef _POSIX_THREAD_SAFE_FUNCTIONS
>> #endif
>>
>> Or the other way around here? I.e. have _POSIX_THREAD_SAFE_FUNCTIONS
>> define/undefine NO_FLOCKFILE?
> 
> From the commit you mentioned, I think that above is OK. However,
> because I'm no C expert, I'm unsure whether I should go with #undef
> suggestion alone or #undef following by no-op declaration below #endif.
> 
> Thanks.
> 

Also, I think NO_FLOCKFILE is rather misnomer: it is the knob
when there is no _POSIX_THREAD_SAFE_FUNCTION, so the knob name should
have been "NO_POSIX_THREAD_SAFE_FUNCTION" instead.

-- 
An old man doll... just what I always wanted! - Clara

[PATCH] git-compat-util: avoid redefining system function names

[Jeff King]

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.

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
 
-- 
2.39.0.rc1.456.gb53e2f823e

Re: [PATCH] git-compat-util: avoid redefining system function names

[Bagas Sanjaya]

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.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] git-compat-util: avoid redefining system function names

[Jeff King]

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)
-- 
2.39.0.rc1.456.gb53e2f823e

Re: [PATCH] git-compat-util: avoid redefining system function names

[Ævar Arnfjörð Bjarmason]

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
 

Re: [PATCH] git-compat-util: avoid redefining system function names

[Jeff King]

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

Re: [PATCH] git-compat-util: avoid redefining system function names

[Bagas Sanjaya]

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/

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] git-compat-util: avoid redefining system function names

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

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?

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] git-compat-util: avoid redefining system function names

[Jeff King]

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