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

From: Jeff King <peff@peff.net>
Date: Nov 28, 2022, 05:01 UTC
Message-ID: <Y4RAr04vS/TOM5uh@coredump.intra.peff.net>
In-Reply-To: <20221125092339.29433-1-bagasdotme@gmail.com>

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

Show 16 quoted lines

> 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