| Message ID | a667da3985a0fe943cc0ff6ee8513d731d75a299.1721171853.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sparse: ignore warning from new glibc headers | expand |
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > With at least glibc 2.39, glibc provides a function declaration that > matches with this POSIX interface: > > int regexec(const regex_t *restrict preg, const char *restrict string, > size_t nmatch, regmatch_t pmatch[restrict], int eflags); > > such prototype requires variable-length-array for `pmatch'. > ... > Thus, sparse reports this error: > >> ../add-patch.c: note: in included file (through ../git-compat-util.h): >> /usr/include/regex.h:682:41: error: undefined identifier '__nmatch' >> /usr/include/regex.h:682:41: error: bad constant expression type >> /usr/include/regex.h:682:41: error: Variable length array is used. I get the same with $ sparse --version v0.6.4-66-g0196afe1 What I have locally in /usr/include may be a bit older. It reads like this: extern int regexec (const regex_t *_Restrict_ __preg, const char *_Restrict_ __String, size_t __nmatch, regmatch_t __pmatch[_Restrict_arr_ _REGEX_NELTS (__nmatch)], int __eflags); where _Restrct_arr_ and _Restrict_ would become an empty string for older compilers, and _REGEX_NELTS(foo) becomes empty when VLA is not available. I think their intention, when the compiler fully supports all the necessary features, is to turn the fourth parameter into regmatch_t __pmatch[restrict __nmatch] I can see how your patch forces the fourth parameter to become (ISO C99) regmatch_t __pmatch[restrict] or even plain vanilla regmatch_t __pmatch[] to erase the mention of __nmatch that is not understood by sparse. > diff --git a/Makefile b/Makefile > index bc81d3395032a..4b9daca1dcc58 100644 > --- a/Makefile > +++ b/Makefile > @@ -1381,7 +1381,7 @@ ARFLAGS = rcs > PTHREAD_CFLAGS = > > # For the 'sparse' target > -SPARSE_FLAGS ?= -std=gnu99 > +SPARSE_FLAGS ?= -std=gnu99 -D__STDC_NO_VLA__ > SP_EXTRA_FLAGS = -Wno-universal-initializer > > # For informing GIT-BUILD-OPTIONS of the SANITIZE=leak,address targets But it makes me feel a bit dirty to define the macro that only compiler implementations are expected to define (or not)[*1*] to cause header files behave the way they would with a compiler without VLA. I dunno. [Reference] *1* https://port70.net/~nsz/c/c11/n1570.html#6.10.8p2
On 17/07/2024 17:54, Junio C Hamano wrote: > Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > >> With at least glibc 2.39, glibc provides a function declaration that >> matches with this POSIX interface: >> >> int regexec(const regex_t *restrict preg, const char *restrict string, >> size_t nmatch, regmatch_t pmatch[restrict], int eflags); >> >> such prototype requires variable-length-array for `pmatch'. >> ... >> Thus, sparse reports this error: >> >>> ../add-patch.c: note: in included file (through ../git-compat-util.h): >>> /usr/include/regex.h:682:41: error: undefined identifier '__nmatch' >>> /usr/include/regex.h:682:41: error: bad constant expression type >>> /usr/include/regex.h:682:41: error: Variable length array is used. Yes, I noted this about 2 years ago! If memory serves, it was when the libc6-dev package went from v2.31 to 2.35 (well 2.31-0ubuntu9.9). As I said at the time, this only affected glibc platforms (so not newlib on cygwin for example) of a certain vintage, so I just added SPARSE_FLAGS += -D__STDC_NO_VLA__ to my config.mak file. > I get the same with > > $ sparse --version > v0.6.4-66-g0196afe1 > I mentioned this problem to Luc on the sparse mailing list[1] and he produced a patch which 'fixed' the problem in one way, but caused a different problem[2]. Namely, because git passes -Wvla to gcc, it now issues the 'used vla' warnings, which gcc does not because of some '# pragma GCC diagnostic ignored "-Wvla"' which sparse does not honor! :( So, his patch was not applied in the end. ATB, Ramsay Jones The sparse mailing list archive can be found at: https://lore.kernel.org/linux-sparse The messages below were from December 2023 (approx. 20/12/2023) [1] Message-ID: <6f853a6b-9ac3-4bfd-a968-89d43fbcce2a@ramsayjones.plus.com> [2] Message-ID: <24cb6194-d04d-4c80-bd95-4f7356667884@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 17/07/2024 17:54, Junio C Hamano wrote: >> Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: >> >>> With at least glibc 2.39, glibc provides a function declaration that >>> matches with this POSIX interface: >>> >>> int regexec(const regex_t *restrict preg, const char *restrict string, >>> size_t nmatch, regmatch_t pmatch[restrict], int eflags); >>> >>> such prototype requires variable-length-array for `pmatch'. >>> ... >>> Thus, sparse reports this error: >>> >>>> ../add-patch.c: note: in included file (through ../git-compat-util.h): >>>> /usr/include/regex.h:682:41: error: undefined identifier '__nmatch' >>>> /usr/include/regex.h:682:41: error: bad constant expression type >>>> /usr/include/regex.h:682:41: error: Variable length array is used. > > Yes, I noted this about 2 years ago! If memory serves, it was when the > libc6-dev package went from v2.31 to 2.35 (well 2.31-0ubuntu9.9). Yes, our mails crossed. I was just writing to linux-sparse@vger ;-) > I mentioned this problem to Luc on the sparse mailing list[1] and > he produced a patch which 'fixed' the problem in one way, but > caused a different problem[2]. Namely, because git passes -Wvla > to gcc, it now issues the 'used vla' warnings, which gcc does > not because of some '# pragma GCC diagnostic ignored "-Wvla"' which > sparse does not honor! :( Sorry, but I do not follow. Isn't -Wno-vla an instruction to sparse to tell it *not* to complain about use of vla? We do not pass -Wvla or -Wno-vla to sparse ourselves. Because the tool comes from the Linux land where VLA is not welcome, we'd by default get the "hey, you used vla here---did you mean it?" error. And the patch by Luc Van Oostenryck in the thread you raised at around the end of 2023 does apply to the tip and with SP_EXTRA_FLAGS += -Wno-vla in Makefile, sparse seems to be happy when I do "make sparse".
On 17/07/2024 19:51, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: [snip] >> I mentioned this problem to Luc on the sparse mailing list[1] and >> he produced a patch which 'fixed' the problem in one way, but >> caused a different problem[2]. Namely, because git passes -Wvla >> to gcc, it now issues the 'used vla' warnings, which gcc does >> not because of some '# pragma GCC diagnostic ignored "-Wvla"' which >> sparse does not honor! :( > > Sorry, but I do not follow. Isn't -Wno-vla an instruction to sparse > to tell it *not* to complain about use of vla? It's a warning flag for both sparse and gcc. At the time, I was trying to find a solution which didn't disable the warning for gcc at the same time (iff you used sparse as a front-end to gcc, which people on git tend not to do; ie _don't_ use 'cgcc -no-compile' ;) ). Also, I wanted a solution that didn't require setting SPARSE_FLAGS (SP_EXTRA_FLAGS didn't exist then) in the Makefile (only some people were affected). > We do not pass -Wvla or -Wno-vla to sparse ourselves. Because the > tool comes from the Linux land where VLA is not welcome, we'd by > default get the "hey, you used vla here---did you mean it?" error. > > And the patch by Luc Van Oostenryck in the thread you raised at > around the end of 2023 does apply to the tip and with > > SP_EXTRA_FLAGS += -Wno-vla > > in Makefile, sparse seems to be happy when I do "make sparse". Yes, that works because 'make sparse' does not use cgcc as a front end to gcc, and the command line has -Wvla followed by -Wno-vla, so last one wins (the sparse specific flags come after the gcc flags). ATB, Ramsay Jones
On 17/07/2024 20:20, Ramsay Jones wrote:
[snip]
> (SP_EXTRA_FLAGS didn't exist then)
This is absolute rubbish, of course! ;)
I don't know what I was thinking, but I suspect I was thinking about
the recent _APPEND variables - except they were only for CFLAGS and
LDFLAGS! Ho Hum.
Sorry about that.
ATB,
Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 17/07/2024 20:20, Ramsay Jones wrote: > [snip] > >> (SP_EXTRA_FLAGS didn't exist then) > This is absolute rubbish, of course! ;) > > I don't know what I was thinking, but I suspect I was thinking about > the recent _APPEND variables - except they were only for CFLAGS and > LDFLAGS! Ho Hum. > > Sorry about that. That's OK. So in short, with a separate SP_EXTRA_FLAGS with "-Wno-vla", Luc's patch is a sufficient fix without any downsides, no?
On 17/07/2024 23:53, Junio C Hamano wrote: [snip] > That's OK. So in short, with a separate SP_EXTRA_FLAGS with "-Wno-vla", > Luc's patch is a sufficient fix without any downsides, no? > Yes, assuming you're only concerned with 'make sparse' usage. BTW, I didn't expect it to take this long for this issue to come back to the list! I expected it to almost immediately cause problems with the sparse ci job, when the version of Ubuntu was updated to the LTS (now previous LTS!). So, I just found a simple solution for now (which turned into 2 years). ATB, Ramsay Jones
On 2024-07-18 01:02:54+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > > On 17/07/2024 23:53, Junio C Hamano wrote: > [snip] > > That's OK. So in short, with a separate SP_EXTRA_FLAGS with "-Wno-vla", > > Luc's patch is a sufficient fix without any downsides, no? > > > > Yes, assuming you're only concerned with 'make sparse' usage. > > BTW, I didn't expect it to take this long for this issue to come > back to the list! I expected it to almost immediately cause > problems with the sparse ci job, when the version of Ubuntu was > updated to the LTS (now previous LTS!). So, I just found a simple > solution for now (which turned into 2 years). Well, yeah, -Wno-vla would work, I used that macro __STDC_NO_VLA__ because I'm not sure Git want to use vla or not, so I only tried to disable it for system headers. And yes, the vla declarationw as added into glibc 2.35.
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 17/07/2024 23:53, Junio C Hamano wrote: > [snip] >> That's OK. So in short, with a separate SP_EXTRA_FLAGS with "-Wno-vla", >> Luc's patch is a sufficient fix without any downsides, no? >> > > Yes, assuming you're only concerned with 'make sparse' usage. Is there anything else in the context of this project I should be concerned with, wrt sparse and recent </usr/include/regex.h> that uses vla in prototype parameters? > BTW, I didn't expect it to take this long for this issue to come > back to the list! I expected it to almost immediately cause > problems with the sparse ci job, when the version of Ubuntu was > updated to the LTS (now previous LTS!). So, I just found a simple > solution for now (which turned into 2 years). ;-) Thanks. It really makes me appreciate whenever I learn that we are blessed with project friends who are involved in many other projects we rely on.
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: >> BTW, I didn't expect it to take this long for this issue to come >> back to the list! I expected it to almost immediately cause >> problems with the sparse ci job, when the version of Ubuntu was >> updated to the LTS (now previous LTS!). So, I just found a simple >> solution for now (which turned into 2 years). > > Well, yeah, -Wno-vla would work, I used that macro __STDC_NO_VLA__ > because I'm not sure Git want to use vla or not, so I only tried to > disable it for system headers. Defining __STDC_NO_VLA__ would rid use of variable length arrays in the regex.h header, so "-Wno-vla" would not be necessary. It's just that it makes me feel a bit dirty to define the macro that only compiler implementations are expected to define in order to cause header files behave the way they would with a compiler without VLA. If we apply Luc's patch [*1*] to sparse, the header would use vla in parameter in the prototype, sparse would grok it, *and* then complain that we are using vla, so we still need "-Wno-vla" on top (but "-Wno-vla" alone would not make (unpatched) sparse grok the construct, of course). > And yes, the vla declarationw as added into glibc 2.35. Thanks. [Reference] *1* https://lore.kernel.org/all/uug4xslokvlxr6z24q52z4pt7nrtiimbzunz2gz3kpilk4kxts@7jljsksi6baq/
On 2024-07-18 09:39:44+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > On 2024-07-18 01:02:54+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > > > > > On 17/07/2024 23:53, Junio C Hamano wrote: > > [snip] > > > That's OK. So in short, with a separate SP_EXTRA_FLAGS with "-Wno-vla", > > > Luc's patch is a sufficient fix without any downsides, no? > > > > > > > Yes, assuming you're only concerned with 'make sparse' usage. > > > > BTW, I didn't expect it to take this long for this issue to come > > back to the list! I expected it to almost immediately cause > > problems with the sparse ci job, when the version of Ubuntu was > > updated to the LTS (now previous LTS!). So, I just found a simple > > solution for now (which turned into 2 years). > > Well, yeah, -Wno-vla would work, I used that macro __STDC_NO_VLA__ > because I'm not sure Git want to use vla or not, so I only tried to > disable it for system headers. Eh, I replied too soon, -Wno-vla doesn't work with my compiler: $ rm -f builtin/am.sp && make V=1 builtin/am.sp cgcc -no-compile -I. -I. -fstack-protector-strong -D_FORTIFY_SOURCE=2 -pipe -O2 -g -march=native -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' \ -Wsparse-error \ -std=gnu99 -Wno-universal-initializer -Wno-vla builtin/am.c && \ >builtin/am.sp builtin/am.c: note: in included file (through git-compat-util.h, builtin.h): /usr/include/regex.h:682:41: error: undefined identifier '__nmatch' /usr/include/regex.h:682:41: error: bad constant expression type make: *** [Makefile:3263: builtin/am.sp] Error 1 $ gcc --version gcc (GCC) 13.2.0 Copyright (C) 2023 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
On 18/07/2024 05:47, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> On 17/07/2024 23:53, Junio C Hamano wrote: >> [snip] >>> That's OK. So in short, with a separate SP_EXTRA_FLAGS with "-Wno-vla", >>> Luc's patch is a sufficient fix without any downsides, no? >>> >> >> Yes, assuming you're only concerned with 'make sparse' usage. > > Is there anything else in the context of this project I should be > concerned with, wrt sparse and recent </usr/include/regex.h> that > uses vla in prototype parameters? No, I don't think so. At the time I remember looking around for more uses of VLA's in the glibc header files and finding nothing of concern: $ find /usr/include -iname '*.h' | > xargs grep -n __STDC_NO_VLA__ /usr/include/regex.h:527: && !defined __STDC_NO_VLA__) /usr/include/brotli/port.h:251: !defined(__STDC_NO_VLA__) && !defined(__cplusplus) && \ $ The <regex.h> header has only a single use of the _REGEX_NELTS macro in the declaration of the regexec() function. The <brotli/port.h> header defines an BROTLI_ARRAY_PARAM macro, but does not use it in the header file. It is available for use in your own source file, but ... I suppose it is possible that other headers use VLA's in their declarations and are not protected by __STDC_NO_VLA__, but that seems unlikely. The headers that we actually use (for the symbols we use), can't include any VLA's or gcc would complain (via -Wvla). Also, it seems <regex.h> is the only header that suppresses -Wvla: $ find /usr/include -iname '*.h' | > xargs grep -n '\-Wvla' /usr/include/regex.h:536:# pragma GCC diagnostic ignored "-Wvla" $ ATB, Ramsay Jones
diff --git a/Makefile b/Makefile index bc81d3395032a..4b9daca1dcc58 100644 --- a/Makefile +++ b/Makefile @@ -1381,7 +1381,7 @@ ARFLAGS = rcs PTHREAD_CFLAGS = # For the 'sparse' target -SPARSE_FLAGS ?= -std=gnu99 +SPARSE_FLAGS ?= -std=gnu99 -D__STDC_NO_VLA__ SP_EXTRA_FLAGS = -Wno-universal-initializer # For informing GIT-BUILD-OPTIONS of the SANITIZE=leak,address targets
With at least glibc 2.39, glibc provides a function declaration that matches with this POSIX interface: int regexec(const regex_t *restrict preg, const char *restrict string, size_t nmatch, regmatch_t pmatch[restrict], int eflags); such prototype requires variable-length-array for `pmatch'. Thus, sparse reports this error: > ../add-patch.c: note: in included file (through ../git-compat-util.h): > /usr/include/regex.h:682:41: error: undefined identifier '__nmatch' > /usr/include/regex.h:682:41: error: bad constant expression type > /usr/include/regex.h:682:41: error: Variable length array is used. Note: `__nmatch' is POSIX's nmatch. The glibc's intention is informing their users to provides a large enough buffer to hold `__nmatch' results and provides diagnosis if necessary. It's merely a glibc' implementation detail. Hide that usage from sparse by using standard C11's macro: __STDC_NO_VLA__ Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)