| Message ID | 20221125092339.29433-1-bagasdotme@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-compat-util.h: Fix build without threads | expand |
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?
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
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
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.
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.
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 */