nstrftime.c fails to build due to memset overflow

nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Dear Gnulib community,

On Linux, x86_64, Fedora 37, ran, on today's coreutils' HEAD (e68b15), which submodule-includes gnulib f17d3977:

CFLAGS=-Wno-deprecated-declarations ./configure

(as that CFLAGS is necessary, otherwise sha will fail to build due to using deprecated functionality; no big issue).
However, building coreutils fails in gnulib and that does seem to be a significant bug:

make -j8 fails with

lib/nstrftime.c: In function '__strftime_internal':
lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
   147 | # define memset_zero(P, Len) (memset (P, '0', Len), (P) += (Len))
       |                               ^~~~~~~~~~~~~~~~~~~~
lib/nstrftime.c:174:17: note: in expansion of macro 'memset_zero'
   174 |                 memset_zero (p, _delta);                                      \
       |                 ^~~~~~~~~~~
lib/nstrftime.c:188:31: note: in expansion of macro 'width_add'
   188 | # define width_add1(width, c) width_add (width, 1, *p = c)
       |                               ^~~~~~~~~
lib/nstrftime.c:1047:17: note: in expansion of macro 'width_add1'
  1047 |                 width_add1 (0, sign_char);
       |                 ^~~~~~~~~~


Now, 18446744073709551615 + 1 happens to be 2⁶⁴; so we're actually tryingh to `memset(P, '0', -1)` here.

I'm actually having a hard time debugging this, as, to be completely honest, I'm not sure how `_delta` ends up being -1:
     if (_n < _w) {
       size_t _delta = _w - _n;
…
But it does!

But then again, I'm also not sure why this macro from 1996 has a parameter `f` that it just – ignores. I'm at a point at which I'm not sure who or what to blame ;)

I'll venture the guess that there's a combination of unexpected (un)signedness and side effects from things that should have been passed as arguments to a function instead of being "silently" "captured" by this macro.

Best regards,
Marcus

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

Hi,

Marcus Müller wrote:
> However, building coreutils fails in gnulib

The build only fails because coreutils' configure.ac turns warnings into
errors by default in some situation. Use the configure option
  --disable-gcc-warnings
or
  --enable-gcc-warnings=no
to allow warnings.

> and that does seem to be a significant bug:
> 
> make -j8 fails with
> 
> lib/nstrftime.c: In function '__strftime_internal':
> lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
>    147 | # define memset_zero(P, Len) (memset (P, '0', Len), (P) += (Len))
>        |                               ^~~~~~~~~~~~~~~~~~~~
> lib/nstrftime.c:174:17: note: in expansion of macro 'memset_zero'
>    174 |                 memset_zero (p, _delta);                                      \
>        |                 ^~~~~~~~~~~
> lib/nstrftime.c:188:31: note: in expansion of macro 'width_add'
>    188 | # define width_add1(width, c) width_add (width, 1, *p = c)
>        |                               ^~~~~~~~~
> lib/nstrftime.c:1047:17: note: in expansion of macro 'width_add1'
>   1047 |                 width_add1 (0, sign_char);
>        |                 ^~~~~~~~~~
> 
> 
> Now, 18446744073709551615 + 1 happens to be 2⁶⁴; so we're actually tryingh to `memset(P, '0', -1)` here.
> 
> I'm actually having a hard time debugging this, as, to be completely honest, I'm not sure how `_delta` ends up being -1:
>      if (_n < _w) {
>        size_t _delta = _w - _n;

You are on the right way to understanding this. Namely, _n and _w being of
type size_t (thus, unsigned 64-bit), the only way _w - _n can be = 2^64 - 1
with _n < _w is when _n is 0 and _w is 2^64 - 1. But _n has the value of the
second argument to width_add, and that argument is 1 in line 188.

So, this is not a significant bug. It's merely a false positive flagged by
your compiler.

There are *many* -Wstringop-overflow bugs in recent GCC versions, see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443

Some of them even have the exact same warning message, see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86345
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100477
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106409
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108377

Bruno

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

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

On 14/03/2023 13:55, Marcus Müller wrote:
> Dear Gnulib community,
> 
> On Linux, x86_64, Fedora 37, ran, on today's coreutils' HEAD (e68b15), which submodule-includes gnulib f17d3977:
> 
> CFLAGS=-Wno-deprecated-declarations ./configure
> 
> (as that CFLAGS is necessary, otherwise sha will fail to build due to using deprecated functionality; no big issue).
> However, building coreutils fails in gnulib and that does seem to be a significant bug:
> 
> make -j8 fails with
> 
> lib/nstrftime.c: In function '__strftime_internal':
> lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]

This is triggered by -O2 being implicitly removed when you specified the CFLAGS explicitly.
I.e. there are gcc false positives at lower optimization levels.

Also you're building from git, and so will have more strict
developer appropriate warning settings by default
(which can be controlled with the --enable-gcc-warnings=no configure option).

In my experience of this particular "stringop-overflow" warning,
I've had to work around it so many times in a large build system in work
that I disabled it by default in the central build config.
It probably makes sense to disable this in the coreutils settings at least,
which is done in the attached.
The attached also addresses -Wmaybe-initialized warnings in coreutils
that show up at lower optimization levels.

cheers,
Pádraig

[-- Attachment #2: coreutils-O0.diff --]
[-- Type: text/x-patch, Size: 1479 bytes --]

diff --git a/configure.ac b/configure.ac
index 2b2f9468d..f33c10ccc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -262,6 +262,10 @@ if test $gl_gcc_warnings != no; then
   # FP in careadlinkat.c w/gcc 10.0.1 20200205
   gl_WARN_ADD([-Wno-return-local-addr])
 
+  # FIXME: remove this line when gcc -O0 improves
+  # FP in nstrftime.c w/gcc 12.2.1 20221121
+  gl_WARN_ADD([-Wno-stringop-overflow])
+
   gl_MANYWARN_COMPLEMENT([GNULIB_WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
   AC_SUBST([GNULIB_WARN_CFLAGS])
 
diff --git a/src/digest.c b/src/digest.c
index 6ee8a4854..3c1d27f9a 100644
--- a/src/digest.c
+++ b/src/digest.c
@@ -1125,6 +1125,7 @@ hex_equal (unsigned char const *hex_digest, unsigned char const *bin_buffer)
   return cnt == digest_bin_bytes;
 }
 
+# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
 static bool
 digest_check (char const *checkfile_name)
 {
diff --git a/src/pr.c b/src/pr.c
index 28a695242..75ce756ae 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -2422,7 +2422,7 @@ static bool
 read_line (COLUMN *p)
 {
   int c;
-  int chars;
+  int chars=0;
   int last_input_position;
   int j, k;
   COLUMN *q;
diff --git a/src/sort.c b/src/sort.c
index 8ca7a88c4..15f06e7d8 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1045,7 +1045,7 @@ pipe_fork (int pipefds[2], size_t tries)
   struct tempnode *saved_temphead;
   int saved_errno;
   double wait_retry = 0.25;
-  pid_t pid;
+  pid_t pid = -1;
   struct cs_status cs;
 
   if (pipe2 (pipefds, O_CLOEXEC) < 0)

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

Pádraig Brady wrote:
>    return cnt == digest_bin_bytes;
>  }
>  
> +# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
>  static bool
>  digest_check (char const *checkfile_name)
>  {

Note that this pragma produces a warning with GCC versions < 4.7:

GCC 4.6.4:
foo.c:1:10: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]

GCC 4.1.2:
foo.c:1: warning: ignoring #pragma GCC diagnostic

and with non-GCC-compatible compilers such as AIX xlc:

"foo.c", line 1.1: 1506-224 (I) Incorrect pragma ignored.

It would therefore be better to protect it like this:

# if defined __GNUC__ && (__GNUC__ + (__GNUC_MINOR__ >= 7) > 4)
#  pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
# endif

Bruno

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hi Bruna,

thanks for answering,

On 14.03.23 17:41, Bruno Haible wrote:
> The build only fails because coreutils' configure.ac turns warnings into
> errors by default in some situation.

Obviously, I agree with this on the deprecated functionality warning, but the string 
method argument overflow warning really triggered my curiosity.

> So, this is not a significant bug. It's merely a false positive flagged by
> your compiler.
Uff! Sorry for the noise then.
> There are *many* -Wstringop-overflow bugs in recent GCC versions, see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
>
> Some of them even have the exact same warning message, see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86345
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89337
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100477
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106409
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108377

Didn't expect that; I had expected integer derivation warnings are "pretty safe", as 
integer constant folding is something an optimizing compiler has to do *correctly*, 
anyway. (and, honestly, coming from a C++ background, these are *a lot of warnings* that 
we're getting here).

Thanks,

Marcus

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hi Pádraig,

ah, this literally came in as I sent my reply to Bruno (and of course mistyped his name; 
sorry!); indeed, that makes "sense" (as far as sense and compiler bugs go well together), 
less optimization means fewer stages of constant folding etc; however, it's surprising 
that GCC is *that* buggy here.

Cheers,
Marcus

On 14.03.23 17:50, Pádraig Brady wrote:
> On 14/03/2023 13:55, Marcus Müller wrote:
>> Dear Gnulib community,
>>
>> On Linux, x86_64, Fedora 37, ran, on today's coreutils' HEAD (e68b15), which 
>> submodule-includes gnulib f17d3977:
>>
>> CFLAGS=-Wno-deprecated-declarations ./configure
>>
>> (as that CFLAGS is necessary, otherwise sha will fail to build due to using deprecated 
>> functionality; no big issue).
>> However, building coreutils fails in gnulib and that does seem to be a significant bug:
>>
>> make -j8 fails with
>>
>> lib/nstrftime.c: In function '__strftime_internal':
>> lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 exceeds 
>> maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
>
> This is triggered by -O2 being implicitly removed when you specified the CFLAGS explicitly.
> I.e. there are gcc false positives at lower optimization levels.
>
> Also you're building from git, and so will have more strict
> developer appropriate warning settings by default
> (which can be controlled with the --enable-gcc-warnings=no configure option).
>
> In my experience of this particular "stringop-overflow" warning,
> I've had to work around it so many times in a large build system in work
> that I disabled it by default in the central build config.
> It probably makes sense to disable this in the coreutils settings at least,
> which is done in the attached.
> The attached also addresses -Wmaybe-initialized warnings in coreutils
> that show up at lower optimization levels.
>
> cheers,
> Pádraig

Re: nstrftime.c fails to build due to memset overflow

[Paul Eggert]

On 3/14/23 09:50, Pádraig Brady wrote:
> The attached also addresses -Wmaybe-initialized warnings in coreutils
> that show up at lower optimization levels.

Let's not make that sort of change, please. It makes the code harder to 
read and analyze, because I look at the code and wonder, "why is this 
variable being initialized when it doesn't need to be?" And it doesn't 
insulate the code against the smarter compilers of the future, which I 
presume will warn us against unnecessary assignments.

If you're going to make that sort of change, at least do what GNU Emacs 
does:

   /* 'int x UNINIT;' is equivalent to 'int x;', except it cajoles GCC
    into not warning incorrectly about use of an uninitialized variable. */
   #if defined GCC_LINT || defined lint
   # define UNINIT = {0,}
   #else
   # define UNINIT /* empty */
   #endif

and then say "int x UNINIT;" instead of "int x;". But personally I would 
leave things alone and ask people to use better compiler options that 
don't generate so many false positives.

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hi Paul,

Uff, being a newb to this community, I feel really bad about giving my 2ct to roughly the 
first thing I read on the mailing list, but:

Let's not do what GNU emacs does.

Having different code when linting and not linting makes bugs that only happen in 
non-debug/lint builds impossible to find. That's a terrible price to pay. And for what? If 
I'm doing a release build, the optimizing compiler would never load that initial value if 
it's never getting read. If my compiler doesn't optimize it away, well, then I have caused 
very little overhead.

I'm all for documenting clearly why something was done; whether that happens in a comment 
or a good git commit message depends on the project style.

Best regards,
Marcus

On 14.03.23 22:49, Paul Eggert wrote:
> On 3/14/23 09:50, Pádraig Brady wrote:
>> The attached also addresses -Wmaybe-initialized warnings in coreutils
>> that show up at lower optimization levels.
>
> Let's not make that sort of change, please. It makes the code harder to read and 
> analyze, because I look at the code and wonder, "why is this variable being initialized 
> when it doesn't need to be?" And it doesn't insulate the code against the smarter 
> compilers of the future, which I presume will warn us against unnecessary assignments.
>
> If you're going to make that sort of change, at least do what GNU Emacs does:
>
>   /* 'int x UNINIT;' is equivalent to 'int x;', except it cajoles GCC
>    into not warning incorrectly about use of an uninitialized variable. */
>   #if defined GCC_LINT || defined lint
>   # define UNINIT = {0,}
>   #else
>   # define UNINIT /* empty */
>   #endif
>
> and then say "int x UNINIT;" instead of "int x;". But personally I would leave things 
> alone and ask people to use better compiler options that don't generate so many false 
> positives.

Re: nstrftime.c fails to build due to memset overflow

[Paul Eggert]

On 3/15/23 02:41, Marcus Müller wrote:

> If my compiler doesn't optimize it away, well, then I have caused very 
> little overhead.

It's not really a question of overhead. Unecessary initializations make 
code harder to understand. Polluting code with unnecessary 
initializations together with comments like /* Initialize this variable 
to pacify gcc 13.1 x86-64 when compiled with -Wall -O2; see 
<https://bugs.gnu.org/99999>. */ is better, but it also gets in the way 
of code ability (and is so rarely done right that I hesitate even 
mentioning it). Making code hard to understand (and/or hard to write 
correctly) drives up maintenance costs and that makes bugs more likely.

This sort of code pollution is the tail wagging the dog. The goal should 
not be to pacify compilers' false alarms. The goal should be to have 
code that works correctly, is easy to understand, is efficient, etc.

We should of course encourage compiler-writers to write smarter 
compilers with fewer false alarms, and we can do that by filing bug 
reports (which I do) and getting these false alarms fixed (which does 
happen). This approach is better in the long run than polluting the 
source code.

If these sorts of false alarms were a real problem, we could disable 
them with the appropriate -Wno-... option, rather than polluting the 
code to work around them. However, the false alarms we're talking about, 
which occur when compiling with -O0 -Wmaybe-initialized warnings, are 
not worth worrying about. We can just ask people not to use that 
combination of options (since it's counterproductive) and we can leave 
the code alone. And that is what we should do here.

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hi Paul,

Sorry to  respectfully contradict you here: introducing a macro really doesn't do readability and immediate clarity of effect any better than a commented zero-initialization.

I also disagree with your approach being less of a submission to the false (or over-arching) compiler warnings: you literally introduce two different code paths just to make a compiler happy. That is IMHO much more of an accommodation than just actually initializing the variable. Your macro is much more of a pollution than the simple
 = 0; // initializing to silence false warning in gcc 12.2, see bug#9999
would be, since it would be confusing without the comment, and redundant with, plus, again, I consider code paths that intentionally differ between debug and release builds detrimental to code quality, due to reduced representativeness of debugging for real world.

Generally, when I (and I fully realize that this describes me, not anyone else necessarily) read codes trying to understand bugs, I dread macros. They require jumping around in code, break scoping rules, and potentially introduce conditionality where clarity would be preferred.

The original white_add macro which gcc falsely finds a -1 in, which kicked off this thread? That should really not be part of any code base. If your macros use locals that aren't passed as macro parameters, they're not ugly, but harmful to maintainers... IMHO. My respect for the humans dealing with this 1995 code of limited beauty.
However, your proposed macro is just ugly, as I need to jump around in code to see what it does, I might need to figure out under which circumstances any of the lint preprocessor variables are set, to see whether for my current run the variable is initialized or not. For the reader's sanity's sake - initialize it. That has no real downside but your feeling that you for some reason did not stand up to GCC by putting a macro where a =0 would have done.

Sorry to disagree so strongly,
Best,
Marcus


Am 15. März 2023 23:42:49 MEZ schrieb Paul Eggert <eggert@cs.ucla.edu>:
>On 3/15/23 02:41, Marcus Müller wrote:
>
>> If my compiler doesn't optimize it away, well, then I have caused very little overhead.
>
>It's not really a question of overhead. Unecessary initializations make code harder to understand. Polluting code with unnecessary initializations together with comments like /* Initialize this variable to pacify gcc 13.1 x86-64 when compiled with -Wall -O2; see <https://bugs.gnu.org/99999>. */ is better, but it also gets in the way of code ability (and is so rarely done right that I hesitate even mentioning it). Making code hard to understand (and/or hard to write correctly) drives up maintenance costs and that makes bugs more likely.
>
>This sort of code pollution is the tail wagging the dog. The goal should not be to pacify compilers' false alarms. The goal should be to have code that works correctly, is easy to understand, is efficient, etc.
>
>We should of course encourage compiler-writers to write smarter compilers with fewer false alarms, and we can do that by filing bug reports (which I do) and getting these false alarms fixed (which does happen). This approach is better in the long run than polluting the source code.
>
>If these sorts of false alarms were a real problem, we could disable them with the appropriate -Wno-... option, rather than polluting the code to work around them. However, the false alarms we're talking about, which occur when compiling with -O0 -Wmaybe-initialized warnings, are not worth worrying about. We can just ask people not to use that combination of options (since it's counterproductive) and we can leave the code alone. And that is what we should do here.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: nstrftime.c fails to build due to memset overflow

[Paul Eggert]

On 3/15/23 16:03, Marcus Müller wrote:
> introducing a macro really doesn't do readability

I don't want the macro either. Let's just leave the code alone.

> I consider code paths that intentionally differ between debug and release builds

There's no need for that. Debug with the same options you use in 
production. This is common practice. But anyway the issue is moot if we 
just leave the code alone.

> The original white_add macro which gcc falsely finds a -1 in, which kicked off this thread? That should really not be part of any code base.

That's a different topic. I agree nstrftime.c is messy. It'd be nice if 
someone had the time to clean it up. But in the meantime we should not 
make it messier just to pacify an poorly-used compiler. We should just 
leave the code alone.

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

Hi Marcus,

Please try to understand Paul's arguments.

> Sorry to  respectfully contradict you here: introducing a macro really
> doesn't do readability and immediate clarity of effect any better than
> a commented zero-initialization.

Apparently Paul and you have different ways of reading code, which
leads to different measures of "readability". You two could keep
contradicting each other eternally; that's not fruitful.

> I consider code paths that intentionally differ between debug and release
> builds detrimental to code quality

This is a valid argument. But read the code that Paul proposed: It does
not conditionalize on NDEBUG. It conditionalizes on GCC_LINT || lint.
These macros can be turned on independently of NDEBUG.

> Generally, when I (and I fully realize that this describes me, not
> anyone else necessarily) read codes trying to understand bugs, I
> dread macros.

There's a big difference between macro-loaded code, as in e.g.
   https://gitlab.inria.fr/gustedt/p99/-/tree/master/p99
or https://github.com/LeoVen/C-Macro-Collections ,
and a simple UNINIT macro that Paul proposed, that does not even
hamper debugging with gdb.

> The original white_add macro which gcc falsely finds a -1 in,
> which kicked off this thread? That should really not be part
> of any code base.

GCC would give the same warning if you would pass it the macro-
expanded source code ("gcc -E" output). Therefore it is irrelevant
whether white_add is a macro, a function, or entirely inlined.

Bruno

Re: nstrftime.c fails to build due to memset overflow

[William Bader]

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

> The goal should not be to pacify compilers' false alarms. The goal should be to have code that works correctly, is easy to understand, is efficient, etc.

In my personal code, when gcc complains about an uninitialized variable, even if the code is correct, I usually either add an initialization or restructure the code.

If the control flow is complicated enough that gcc can't follow it, usually the code takes some effort to understand, debugging in the future could waste time verifying that the variable is initialized in all flow paths, and changes to the code could accidentally create flow paths that leave the variable uninitialized.

William

[-- Attachment #2: Type: text/html, Size: 1916 bytes --]

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hey Paul,

I think we're in agreement, even though I've been in the situation that the compiler 
warning me about unitialized variable usage has saved me a lot of headache – and maybe 
that's why I'm more sympathetic to making the compiler happy in such isolated circumstances.
We both would like to avoid the macro, but my benefit/"political/compiler-ecosystem 
optimizing" cost tradeoff would be a bit different.

Cheers,
Marcus

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hi Bruno,

On 16.03.23 04:46, Bruno Haible wrote:
> Apparently Paul and you have different ways of reading code, which
> leads to different measures of "readability". You two could keep
> contradicting each other eternally; that's not fruitful.
I don't intend to do that :) Paul seems to be the more experienced programmer than me, and 
I wouldn't see the value in any kind of flamewar, either. Still was kind of uneasy seeing 
the suggested code and not saying something – here we are!
>> I consider code paths that intentionally differ between debug and release
>> builds detrimental to code quality
> This is a valid argument. But read the code that Paul proposed: It does
> not conditionalize on NDEBUG. It conditionalizes on GCC_LINT || lint.
> These macros can be turned on independently of NDEBUG.

That is an undisputable fact; but my problem here was not the technical impossibility, but 
the ways that comes back to bite someone. See, for example canonicalize-lgpl.c, where 
GCC_LINT gets set conditioned on _LIBC being defined. That is kind of surprising, isn't 
it? That's the "introduction of conditionality where clarity would be preferred" I was 
referring to. Sure, this isn't canonicalize-lgpl.c, so that setting there has no effect 
here, but if I have a codebase that does such things, I'm bound to be checking every 
single expansion for preconditions in all the internal headers I include. That's just hard 
to work with, for me.

Generally, I would (my perspective) try to minimized the moment of surprise; a =0 to me is 
less surprising. I know Paul sees this differently, but I'm not arguing against him being 
right with how he reads the code.
I'm arguing that introduction of separate code paths is the hard-to-justify thing. (But 
Paul already replied that he wouldn't want the macro either, so, not even sure we disagree 
on that. I think we both want the code to be as unmodified as possible. I still would want 
to silence the warning, because the *class* of warnings has proven to be very productive, 
so disabling it, or having false positives in there, is kind of making code improvement 
harder – but, I feel it's fair to stress that – this is my perspective on things. I mean, 
gnulib CHANGELOG tells me that there's a few things that coverity and other tools might 
have prevented.)

> There's a big difference between macro-loaded code, as in e.g.
>     https://gitlab.inria.fr/gustedt/p99/-/tree/master/p99
> or https://github.com/LeoVen/C-Macro-Collections ,
> and a simple UNINIT macro that Paul proposed, that does not even
> hamper debugging with gdb.

a) Wow, didn't know these!
b) No doubt. Still, and I think Paul's last two emails explicitly agreed here, it's better 
to *not* have the macro than to have it.

>> The original white_add macro which gcc falsely finds a -1 in,
>> which kicked off this thread? That should really not be part
>> of any code base.
> GCC would give the same warning if you would pass it the macro-
> expanded source code ("gcc -E" output).

I know, I had to manually copy and paste half-expanded code into the place where it's 
called, just to be sure I'm not missing something – due to the non-locality of what the 
macro uses. Because I honestly thought that it's more likely I can't read the code, 
multi-layered and convoluted and non-explicit-variable depending as it is, than that the 
compiler cries wolf in the presence of naught. Thankfully, I've been proven wrong! That's 
good on multiple levels: Neither is the code dysfunctional, nor am I going insane.

> Therefore it is irrelevant
> whether white_add is a macro, a function, or entirely inlined.

Seeing that you start this email on a personal note (thanks!) to reduce the chance of 
prolonged exchange of contradictory perspectives without a path to actually producing a 
positive outcome, I'm not quite sure how great it'd be of me to contradict you here:

To me, it wasn't irrelevant.
If this macro wasn't so bad, looking at it anyone could have instantly deduced that this 
is a compiler diagnostic in error, and went on to act on that knowledge. I wrote that "bug 
report" email because I felt I couldn't fix this code and must be missing something.

Best regards, and again, thanks for putting up with my perspectives,
Marcus

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

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

On 14/03/2023 16:50, Pádraig Brady wrote:
> On 14/03/2023 13:55, Marcus Müller wrote:
>> Dear Gnulib community,
>>
>> On Linux, x86_64, Fedora 37, ran, on today's coreutils' HEAD (e68b15), which submodule-includes gnulib f17d3977:
>>
>> CFLAGS=-Wno-deprecated-declarations ./configure
>>
>> (as that CFLAGS is necessary, otherwise sha will fail to build due to using deprecated functionality; no big issue).
>> However, building coreutils fails in gnulib and that does seem to be a significant bug:
>>
>> make -j8 fails with
>>
>> lib/nstrftime.c: In function '__strftime_internal':
>> lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
> 
> This is triggered by -O2 being implicitly removed when you specified the CFLAGS explicitly.
> I.e. there are gcc false positives at lower optimization levels.

Actually different -Wmaybe-initialized warnings can trigger at various optimization levels.

> Also you're building from git, and so will have more strict
> developer appropriate warning settings by default
> (which can be controlled with the --enable-gcc-warnings=no configure option).
> 
> In my experience of this particular "stringop-overflow" warning,
> I've had to work around it so many times in a large build system in work
> that I disabled it by default in the central build config.
> It probably makes sense to disable this in the coreutils settings at least,
> which is done in the attached.
> The attached also addresses -Wmaybe-initialized warnings in coreutils
> that show up at lower optimization levels.

I'm applying the two attached patches to coreutils to address these issues.

cheers,
Pádraig

[-- Attachment #2: 0001-build-avoid-incorrect-Wmaybe-uninitialized-warnings.patch --]
[-- Type: text/x-patch, Size: 4952 bytes --]

From b4e20026a49297f024d399f1f49c58dc3134f82b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <P@draigBrady.com>
Date: Thu, 18 May 2023 10:38:11 +0100
Subject: [PATCH 1/2] build: avoid incorrect -Wmaybe-uninitialized warnings

Allow easily building a debug build for example with:
  make CFLAGS='-O0 -ggdb'

False -Wmaybe-uninitialized warnings hit in different
places depending on the compiler passes used.
These changes were tested with gcc 10.2.1, 12.2.1, and 13.1.1 like:
  for o in g s z fast 0 1 2 3; do
    make clean && make -j$(nproc) CFLAGS="-O$o" || break
  done

* src/digest.c: Disable -Wmaybe-uninitialized that gives
false positive here at -O0.
* src/ln.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -O1.
* src/pr.c: Likewise.
* src/sort.c: Likewise.
* src/tee.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -O3 on gcc 13.1.1 at least.
* src/cp.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -Os on gcc 13.1.1 at least.
* src/copy.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -Og on gcc 13.1.1 at least.
* src/head.c: Likewise.
* src/paste.c: Likewise.
---
 src/copy.c   | 4 ++--
 src/cp.c     | 2 +-
 src/digest.c | 3 +++
 src/head.c   | 2 +-
 src/ln.c     | 2 +-
 src/paste.c  | 2 +-
 src/pr.c     | 2 +-
 src/sort.c   | 2 +-
 src/tee.c    | 2 +-
 9 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 0dd059d2e..6b2cf3b29 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1238,8 +1238,8 @@ copy_reg (char const *src_name, char const *dst_name,
           struct stat const *src_sb)
 {
   char *buf = NULL;
-  int dest_desc;
-  int dest_errno;
+  int dest_desc IF_LINT ( = -1);
+  int dest_errno IF_LINT ( = 0);
   int source_desc;
   mode_t src_mode = src_sb->st_mode;
   mode_t extra_permissions;
diff --git a/src/cp.c b/src/cp.c
index 619eb8260..c952eb397 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -441,7 +441,7 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 
       while ((slash = strchr (slash, '/')))
         {
-          struct dir_attr *new;
+          struct dir_attr *new IF_LINT ( = NULL);
           bool missing_dir;
 
           *slash = '\0';
diff --git a/src/digest.c b/src/digest.c
index ab32968db..8c483a425 100644
--- a/src/digest.c
+++ b/src/digest.c
@@ -1125,6 +1125,9 @@ hex_equal (unsigned char const *hex_digest, unsigned char const *bin_buffer)
   return cnt == digest_bin_bytes;
 }
 
+# if defined __GNUC__ && (__GNUC__ + (__GNUC_MINOR__ >= 7) > 4)
+#  pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+# endif
 static bool
 digest_check (char const *checkfile_name)
 {
diff --git a/src/head.c b/src/head.c
index c9d3b0d05..f576a2200 100644
--- a/src/head.c
+++ b/src/head.c
@@ -351,7 +351,7 @@ elide_tail_bytes_pipe (char const *filename, int fd, uintmax_t n_elide_0,
          bytes.  Then, for each new buffer we read, also write an old one.  */
 
       bool eof = false;
-      size_t n_read;
+      size_t n_read IF_LINT ( = 0);
       bool buffered_enough;
       size_t i, i_next;
       char **b = NULL;
diff --git a/src/ln.c b/src/ln.c
index 1c3307cac..bfdac0cd9 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -473,7 +473,7 @@ main (int argc, char **argv)
   char const *backup_suffix = NULL;
   char *version_control_string = NULL;
   char const *target_directory = NULL;
-  int destdir_fd;
+  int destdir_fd IF_LINT ( = -1);
   bool no_target_directory = false;
   int n_files;
   char **file;
diff --git a/src/paste.c b/src/paste.c
index 5c194d8fe..468ef3ab0 100644
--- a/src/paste.c
+++ b/src/paste.c
@@ -233,7 +233,7 @@ paste_parallel (size_t nfiles, char **fnamptr)
 
       for (size_t i = 0; i < nfiles && files_open; i++)
         {
-          int chr;			/* Input character. */
+          int chr IF_LINT ( = -1);	/* Input character. */
           int err;			/* Input errno value.  */
           bool sometodo = false;	/* Input chars to process.  */
 
diff --git a/src/pr.c b/src/pr.c
index 1c32e8c81..6be5b1f33 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -2428,7 +2428,7 @@ static bool
 read_line (COLUMN *p)
 {
   int c;
-  int chars;
+  int chars IF_LINT ( =0);
   int last_input_position;
   int j, k;
   COLUMN *q;
diff --git a/src/sort.c b/src/sort.c
index 8ca7a88c4..5aadc797b 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1045,7 +1045,7 @@ pipe_fork (int pipefds[2], size_t tries)
   struct tempnode *saved_temphead;
   int saved_errno;
   double wait_retry = 0.25;
-  pid_t pid;
+  pid_t pid IF_LINT ( = -1);
   struct cs_status cs;
 
   if (pipe2 (pipefds, O_CLOEXEC) < 0)
diff --git a/src/tee.c b/src/tee.c
index a1c057816..27e83619e 100644
--- a/src/tee.c
+++ b/src/tee.c
@@ -228,7 +228,7 @@ tee_files (int nfiles, char **files, bool pipe_check)
 {
   size_t n_outputs = 0;
   FILE **descriptors;
-  bool *out_pollable;
+  bool *out_pollable IF_LINT ( = NULL);
   char buffer[BUFSIZ];
   ssize_t bytes_read = 0;
   int i;
-- 
2.40.1


[-- Attachment #3: 0002-build-gnulib-avoid-false-Wstringop-overflow-warning.patch --]
[-- Type: text/x-patch, Size: 1159 bytes --]

From ffaba6e8afb2235f758bf4efdedf1441ed8eda8b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <P@draigBrady.com>
Date: Thu, 18 May 2023 11:40:19 +0100
Subject: [PATCH 2/2] build: gnulib: avoid false -Wstringop-overflow warning

Tested on gcc 13.1.1 with: make CFLAGS='-O0 -ggdb'

* configure.ac: Disable -Wstringop-overflow for gnulib.
This warning is far too problematic in my experience:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
and triggers with gcc -O0 with versions 12,13 at least.
---
 configure.ac | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure.ac b/configure.ac
index f61076d2b..27e743157 100644
--- a/configure.ac
+++ b/configure.ac
@@ -261,6 +261,11 @@ if test $gl_gcc_warnings != no; then
   # FP in careadlinkat.c w/gcc 10.0.1 20200205
   gl_WARN_ADD([-Wno-return-local-addr])
 
+  # FIXME: remove this line when gcc improves
+  # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
+  # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
+  gl_WARN_ADD([-Wno-stringop-overflow])
+
   gl_MANYWARN_COMPLEMENT([GNULIB_WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
   AC_SUBST([GNULIB_WARN_CFLAGS])
 
-- 
2.40.1

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

[Dropping coreutils from CC.]

Pádraig Brady wrote:
> Actually different -Wmaybe-initialized warnings can trigger at various optimization levels.

That's an interesting observation. Indeed, compiling a testdir of all of gnulib
at various optimization levels with gcc 13.1.0, through commands like this:

  export CPPFLAGS=-Wall
  mkdir build-O0; cd build-O0; CFLAGS=-O0 ../configure 2>&1 | tee log1; make 2>&1 | tee log2; cd ..
  mkdir build-O1; cd build-O1; CFLAGS=-O1 ../configure 2>&1 | tee log1; make 2>&1 | tee log2; cd ..
  mkdir build-O2; cd build-O2; CFLAGS=-O2 ../configure 2>&1 | tee log1; make 2>&1 | tee log2; cd ..
  mkdir build-O3; cd build-O3; CFLAGS=-O3 ../configure 2>&1 | tee log1; make 2>&1 | tee log2; cd ..
  mkdir build-Os; cd build-Os; CFLAGS=-Os ../configure 2>&1 | tee log1; make 2>&1 | tee log2; cd ..
  mkdir build-Og; cd build-Og; CFLAGS=-Og ../configure 2>&1 | tee log1; make 2>&1 | tee log2; cd ..

I get warnings at various levels:

* All optimizations levels:
../../gllib/astrxfrm.c:159:19: warning: pointer 'result' may be used after 'realloc' [-Wuse-after-free]
../../gllib/get-rusage-data.c:354:1: warning: 'get_rusage_data_via_iterator' defined but not used [-Wunused-function]
../../gllib/hamt.c:201:1: warning: 'init_element' defined but not used [-Wunused-function]
../../gltests/../gllib/stack.h:75:24: warning: 'stack_current_base' defined but not used [-Wunused-function]

* -O0 only:
../../gllib/nstrftime.c:148:31: warning: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
../../gllib/nstrftime.c:147:32: warning: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]

* -O1, -O2, -O3 only:
../../gllib/vasnprintf.c:945:26: warning: argument 1 value '18446744073709551615' exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]

* -O1, -O2, -O3, -Os only:
../../gllib/vasnprintf.c:1392:10: warning: 'e' may be used uninitialized [-Wmaybe-uninitialized]
../../gllib/vasnprintf.c:1410:10: warning: 'e' may be used uninitialized [-Wmaybe-uninitialized]

* -O1, -O2, -O3, -Og, -Os only:
../../gllib/getndelim2.c:191:23: warning: 'c' may be used uninitialized [-Wmaybe-uninitialized]

* -O2, -O3, -Os only:
../../gllib/astrxfrm.c:177:1: warning: function may return address of local variable [-Wreturn-local-addr]

* -O2, -O3 only:
../../gllib/canonicalize.c:385:33: warning: 'end_idx' may be used uninitialized [-Wmaybe-uninitialized]

* -Og only:
../../gllib/bitset/list.c:458:22: warning: 'tmp' may be used uninitialized [-Wmaybe-uninitialized]

I've seen many of these warnings and regularly ignored them. But one of these
warnings points to a real bug (in astrxfrm.c), that was sitting there since
the beginning.

The lesson I learn from this (once again — I already knew it before...) that
it is important to have a warning-free build on glibc systems.

Bruno

astrxfrm: Fix use-after-free bug

[Bruno Haible]

One of the gcc warnings points to an actual bug:

../../gllib/astrxfrm.c:159:19: warning: pointer 'result' may be used after 'realloc' [-Wuse-after-free]

This patch fixes it.


2023-05-18  Bruno Haible  <bruno@clisp.org>

	astrxfrm: Fix use-after-free bug.
	* lib/astrxfrm.c (astrxfrm): Don't use memcpy after realloc succeeded.

diff --git a/lib/astrxfrm.c b/lib/astrxfrm.c
index 214ebc38b8..845f0a0711 100644
--- a/lib/astrxfrm.c
+++ b/lib/astrxfrm.c
@@ -155,10 +155,7 @@ astrxfrm (const char *s, char *resultbuf, size_t *lengthp)
             {
               char *memory = (char *) realloc (result, length);
               if (memory != NULL)
-                {
-                  memcpy (memory, result, length);
-                  result = memory;
-                }
+                result = memory;
             }
         }
     }

silence some "defined but not used" warnings

[Bruno Haible]

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

In order to silence these warnings

../../gllib/get-rusage-data.c:354:1: warning: 'get_rusage_data_via_iterator' defined but not used [-Wunused-function]
../../gllib/hamt.c:201:1: warning: 'init_element' defined but not used [-Wunused-function]
../../gltests/../gllib/stack.h:75:24: warning: 'stack_current_base' defined but not used [-Wunused-function]

I'm making use of _GL_ATTRIBUTE_MAYBE_UNUSED.


2023-05-18  Bruno Haible  <bruno@clisp.org>

	stack: Silence gcc warning in tests.
	* lib/stack.h (init, destroy, empty, current_base, push, pop, discard,
	top, size): Mark as possibly unused.

2023-05-18  Bruno Haible  <bruno@clisp.org>

	hamt: Silence gcc warning.
	* lib/hamt.c (init_element): Mark as possibly unused.

2023-05-18  Bruno Haible  <bruno@clisp.org>

	get-rusage-data: Silence gcc warning.
	* lib/get-rusage-data.c (get_rusage_data_via_iterator): Mark as possibly
	unused.


[-- Attachment #2: 0001-get-rusage-data-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1120 bytes --]

From 1b63db9814cd6734596566433ff9f6230b6ec04a Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:43:14 +0200
Subject: [PATCH 01/10] get-rusage-data: Silence gcc warning.

* lib/get-rusage-data.c (get_rusage_data_via_iterator): Mark as possibly
unused.
---
 ChangeLog             | 6 ++++++
 lib/get-rusage-data.c | 1 +
 2 files changed, 7 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index f105e2db8f..e080e8f670 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	get-rusage-data: Silence gcc warning.
+	* lib/get-rusage-data.c (get_rusage_data_via_iterator): Mark as possibly
+	unused.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	astrxfrm: Fix use-after-free bug.
diff --git a/lib/get-rusage-data.c b/lib/get-rusage-data.c
index 4d5dc4092a..565fcc33f3 100644
--- a/lib/get-rusage-data.c
+++ b/lib/get-rusage-data.c
@@ -350,6 +350,7 @@ vma_iterate_callback (void *data, uintptr_t start, uintptr_t end,
   return 0;
 }
 
+_GL_ATTRIBUTE_MAYBE_UNUSED
 static uintptr_t
 get_rusage_data_via_iterator (void)
 {
-- 
2.34.1


[-- Attachment #3: 0002-hamt-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1009 bytes --]

From 136942a565b7f3ccd6632731c3622460c5f55523 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:45:45 +0200
Subject: [PATCH 02/10] hamt: Silence gcc warning.

* lib/hamt.c (init_element): Mark as possibly unused.
---
 ChangeLog  | 5 +++++
 lib/hamt.c | 1 +
 2 files changed, 6 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index e080e8f670..aff37369ef 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	hamt: Silence gcc warning.
+	* lib/hamt.c (init_element): Mark as possibly unused.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	get-rusage-data: Silence gcc warning.
diff --git a/lib/hamt.c b/lib/hamt.c
index aa76f9029e..8cbca387f0 100644
--- a/lib/hamt.c
+++ b/lib/hamt.c
@@ -197,6 +197,7 @@ free_element (const struct function_table *functions, Hamt_entry *elt)
 }
 
 /* Return the initialized element.  */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 static Hamt_entry *
 init_element (Hamt_entry *elt)
 {
-- 
2.34.1


[-- Attachment #4: 0003-stack-Silence-gcc-warning-in-tests.patch --]
[-- Type: text/x-patch, Size: 3764 bytes --]

From 54504760620fc798d62fc3ec3668ff87e3d9bd56 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:48:54 +0200
Subject: [PATCH 03/10] stack: Silence gcc warning in tests.

* lib/stack.h (init, destroy, empty, current_base, push, pop, discard,
top, size): Mark as possibly unused.
---
 ChangeLog   |  6 ++++++
 lib/stack.h | 17 +++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index aff37369ef..0216321fc6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	stack: Silence gcc warning in tests.
+	* lib/stack.h (init, destroy, empty, current_base, push, pop, discard,
+	top, size): Mark as possibly unused.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	hamt: Silence gcc warning.
diff --git a/lib/stack.h b/lib/stack.h
index ab6b873176..dbd6812bc8 100644
--- a/lib/stack.h
+++ b/lib/stack.h
@@ -58,7 +58,7 @@
      #include "xalloc.h"
 */
 
-/* This file uses _GL_ATTRIBUTE_PURE.  */
+/* This file uses _GL_ATTRIBUTE_MAYBE_UNUSED, _GL_ATTRIBUTE_PURE.  */
 #if !_GL_CONFIG_H_INCLUDED
  #error "Please include config.h first."
 #endif
@@ -86,6 +86,7 @@ typedef struct
 } _GL_STACK_TYPE;
 
 /* Initialize a stack.  */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 GL_STACK_STORAGECLASS void
 _GL_STACK_PREFIX (init) (_GL_STACK_TYPE *stack)
 {
@@ -95,6 +96,7 @@ _GL_STACK_PREFIX (init) (_GL_STACK_TYPE *stack)
 }
 
 /* Destroy a stack by freeing the allocated space.  */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 GL_STACK_STORAGECLASS void
 _GL_STACK_PREFIX (destroy) (_GL_STACK_TYPE *stack)
 {
@@ -102,7 +104,8 @@ _GL_STACK_PREFIX (destroy) (_GL_STACK_TYPE *stack)
 }
 
 /* Return true if the stack currently holds no element.  */
-GL_STACK_STORAGECLASS _GL_ATTRIBUTE_PURE bool
+_GL_ATTRIBUTE_MAYBE_UNUSED _GL_ATTRIBUTE_PURE
+GL_STACK_STORAGECLASS bool
 _GL_STACK_PREFIX (empty) (const _GL_STACK_TYPE *stack)
 {
   return stack->size == 0;
@@ -112,13 +115,15 @@ _GL_STACK_PREFIX (empty) (const _GL_STACK_TYPE *stack)
 
    The result is invalidated as soon as an element is added or removed
    from the stack.  */
-GL_STACK_STORAGECLASS _GL_ATTRIBUTE_PURE GL_STACK_ELEMENT *
+_GL_ATTRIBUTE_MAYBE_UNUSED _GL_ATTRIBUTE_PURE
+GL_STACK_STORAGECLASS GL_STACK_ELEMENT *
 _GL_STACK_PREFIX (current_base) (const _GL_STACK_TYPE *stack)
 {
   return stack->base;
 }
 
 /* Push an element to the top of the stack.  */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 GL_STACK_STORAGECLASS void
 _GL_STACK_PREFIX (push) (_GL_STACK_TYPE *stack, GL_STACK_ELEMENT item)
 {
@@ -130,6 +135,7 @@ _GL_STACK_PREFIX (push) (_GL_STACK_TYPE *stack, GL_STACK_ELEMENT item)
 
 /* Pop the element from the top of the stack, which must be non-empty,
    and return it. */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 GL_STACK_STORAGECLASS GL_STACK_ELEMENT
 _GL_STACK_PREFIX (pop) (_GL_STACK_TYPE *stack)
 {
@@ -139,6 +145,7 @@ _GL_STACK_PREFIX (pop) (_GL_STACK_TYPE *stack)
 
 /* Pop the element from the top of the stack, which must be
    non-empty.  */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 GL_STACK_STORAGECLASS void
 _GL_STACK_PREFIX (discard) (_GL_STACK_TYPE *stack)
 {
@@ -148,6 +155,7 @@ _GL_STACK_PREFIX (discard) (_GL_STACK_TYPE *stack)
 
 /* Return the element from the top of the stack, which must be
    non-empty.  */
+_GL_ATTRIBUTE_MAYBE_UNUSED
 GL_STACK_STORAGECLASS GL_STACK_ELEMENT
 _GL_STACK_PREFIX (top) (const _GL_STACK_TYPE *stack)
 {
@@ -156,7 +164,8 @@ _GL_STACK_PREFIX (top) (const _GL_STACK_TYPE *stack)
 }
 
 /* Return the currently stored number of elements in the stack.  */
-GL_STACK_STORAGECLASS _GL_ATTRIBUTE_PURE idx_t
+_GL_ATTRIBUTE_MAYBE_UNUSED _GL_ATTRIBUTE_PURE
+GL_STACK_STORAGECLASS idx_t
 _GL_STACK_PREFIX (size) (const _GL_STACK_TYPE *stack)
 {
   return stack->size;
-- 
2.34.1

Re: nstrftime.c fails to build due to memset overflow

[Paul Eggert]

Let's revert the "avoid incorrect -Wmaybe-uninitialized warnings" patch.

--enable-gcc-warnings is designed for the default gcc -O2, and we 
shouldn't dumb down our source code for lesser platforms like "gcc -O0", 
or clang, or whatever.

For example, this patch:

> -  int dest_desc;
> -  int dest_errno;
> +  int dest_desc IF_LINT ( = -1);
> +  int dest_errno IF_LINT ( = 0);

means that we won't catch any programming errors caused by code failing 
to initialize dest_errno. That's a net minus to coreutils reliability. 
It's a *good* thing that dest_errno is not initialized here, and that 
GCC will complain if we use it uninitialized. We don't want to lose that 
good thing.

IF_LINT should be used sparingly: ideally only when gcc -O2 issues a 
false positive, and even then only after you've filed a bug report with 
the GCC maintainers because GCC is messing up.

The goal here is software reliability not pacifying compilers, and 
overuse of IF_LINT (such as the above) is a net minus to reliability.

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

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

In order to silence these warnings:

> * -O1, -O2, -O3, -Os only:
> ../../gllib/vasnprintf.c:1392:10: warning: 'e' may be used uninitialized [-Wmaybe-uninitialized]
> ../../gllib/vasnprintf.c:1410:10: warning: 'e' may be used uninitialized [-Wmaybe-uninitialized]
> 
> * -Og only:
> ../../gllib/bitset/list.c:458:22: warning: 'tmp' may be used uninitialized [-Wmaybe-uninitialized]

I'm applying small code changes.

- In vasnprintf.c, gcc is right that the variable e is used unitialized; the
  value is passed to a function that then ignores it.
- In bitset/list.c, gcc is fooled by a loop entry test that can never fail,
  but gcc happens to not notice it. As an optimization of the code, let me
  remove this loop entry test (i.e. transform it to a loop end test).


2023-05-18  Bruno Haible  <bruno@clisp.org>

	vasnprintf, c-vasnprintf: Silence gcc warnings.
	* lib/vasnprintf.c (scale10_round_decimal_decoded): Remove memory==NULL
	test.
	(scale10_round_decimal_long_double, scale10_round_decimal_double): Test
	for memory==NULL here. Remove use of IF_LINT.

2023-05-18  Bruno Haible  <bruno@clisp.org>

	bitset: Silence gcc warning.
	* lib/bitset/list.c (lbitset_copy_): Remove redundant test from the
	loop's first iteration.


[-- Attachment #2: 0001-bitset-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1462 bytes --]

From bcab9f794d04477fdbedc82961510c80f8cbab05 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:40:12 +0200
Subject: [PATCH 1/7] bitset: Silence gcc warning.

* lib/bitset/list.c (lbitset_copy_): Remove redundant test from the
loop's first iteration.
---
 ChangeLog         | 6 ++++++
 lib/bitset/list.c | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 0216321fc6..1796c625ee 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	bitset: Silence gcc warning.
+	* lib/bitset/list.c (lbitset_copy_): Remove redundant test from the
+	loop's first iteration.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	stack: Silence gcc warning in tests.
diff --git a/lib/bitset/list.c b/lib/bitset/list.c
index f73ea70fee..eee7fefeeb 100644
--- a/lib/bitset/list.c
+++ b/lib/bitset/list.c
@@ -441,7 +441,8 @@ lbitset_copy_ (bitset dst, bitset src)
 
   lbitset_elt *prev = 0;
   lbitset_elt *tmp;
-  for (lbitset_elt *elt = head; elt; elt = elt->next)
+  lbitset_elt *elt = head;
+  do
     {
       tmp = lbitset_elt_alloc ();
       tmp->index = elt->index;
@@ -454,7 +455,10 @@ lbitset_copy_ (bitset dst, bitset src)
       prev = tmp;
 
       memcpy (tmp->words, elt->words, sizeof (elt->words));
+
+      elt = elt->next;
     }
+  while (elt);
   LBITSET_TAIL (dst) = tmp;
 
   dst->b.csize = LBITSET_ELT_WORDS;
-- 
2.34.1


[-- Attachment #3: 0002-vasnprintf-c-vasnprintf-Silence-gcc-warnings.patch --]
[-- Type: text/x-patch, Size: 2342 bytes --]

From a654869530ca6fd6cf45f9e1f8257362b1100f7e Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:51:17 +0200
Subject: [PATCH 2/7] vasnprintf, c-vasnprintf: Silence gcc warnings.

* lib/vasnprintf.c (scale10_round_decimal_decoded): Remove memory==NULL
test.
(scale10_round_decimal_long_double, scale10_round_decimal_double): Test
for memory==NULL here. Remove use of IF_LINT.
---
 ChangeLog        |  8 ++++++++
 lib/vasnprintf.c | 16 ++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1796c625ee..ee9a35da35 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	vasnprintf, c-vasnprintf: Silence gcc warnings.
+	* lib/vasnprintf.c (scale10_round_decimal_decoded): Remove memory==NULL
+	test.
+	(scale10_round_decimal_long_double, scale10_round_decimal_double): Test
+	for memory==NULL here. Remove use of IF_LINT.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	bitset: Silence gcc warning.
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 802790e14e..007d280980 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -1177,8 +1177,6 @@ scale10_round_decimal_decoded (int e, mpn_t m, void *memory, int n)
   void *z_memory;
   char *digits;
 
-  if (memory == NULL)
-    return NULL;
   /* x = 2^e * m, hence
      y = round (2^e * 10^n * m) = round (2^(e+n) * 5^n * m)
        = round (2^s * 5^n * m).  */
@@ -1386,10 +1384,13 @@ scale10_round_decimal_decoded (int e, mpn_t m, void *memory, int n)
 static char *
 scale10_round_decimal_long_double (long double x, int n)
 {
-  int e IF_LINT(= 0);
+  int e;
   mpn_t m;
   void *memory = decode_long_double (x, &e, &m);
-  return scale10_round_decimal_decoded (e, m, memory, n);
+  if (memory != NULL)
+    return scale10_round_decimal_decoded (e, m, memory, n);
+  else
+    return NULL;
 }
 
 # endif
@@ -1404,10 +1405,13 @@ scale10_round_decimal_long_double (long double x, int n)
 static char *
 scale10_round_decimal_double (double x, int n)
 {
-  int e IF_LINT(= 0);
+  int e;
   mpn_t m;
   void *memory = decode_double (x, &e, &m);
-  return scale10_round_decimal_decoded (e, m, memory, n);
+  if (memory != NULL)
+    return scale10_round_decimal_decoded (e, m, memory, n);
+  else
+    return NULL;
 }
 
 # endif
-- 
2.34.1

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

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

To silence these warnings:

 * -O0 only:
 ../../gllib/nstrftime.c:148:31: warning: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
 ../../gllib/nstrftime.c:147:32: warning: 'memset' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
 
 * -O1, -O2, -O3 only:
 ../../gllib/vasnprintf.c:945:26: warning: argument 1 value '18446744073709551615' exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
 
 * -O2, -O3, -Os only:
 ../../gllib/astrxfrm.c:177:1: warning: function may return address of local variable [-Wreturn-local-addr]

I am applying these patches. The warning in vasnprintf.c is a consequence of
the use of SIZE_MAX (by xsize.h) to make malloc() fail. The warnings in
nstrftime.c and astrxfrm.c are false positives, due to insufficient data
flow analysis by gcc.


2023-05-18  Bruno Haible  <bruno@clisp.org>

	vasnprintf, c-vasnprintf: Silence gcc warning.
	* lib/vasnprintf.c: Add #pragma GCC diagnostic.

2023-05-18  Bruno Haible  <bruno@clisp.org>

	nstrftime: Silence gcc warning.
	* lib/nstrftime.c: Add #pragma GCC diagnostic.

2023-05-18  Bruno Haible  <bruno@clisp.org>

	astrxfrm: Silence gcc warning.
	* lib/astrxfrm.c: Add #pragma GCC diagnostic.


[-- Attachment #2: 0001-astrxfrm-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1173 bytes --]

From 85a42d4dbb49735056c519f8da58d44e22ebc1c4 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:37:20 +0200
Subject: [PATCH 1/5] astrxfrm: Silence gcc warning.

* lib/astrxfrm.c: Add #pragma GCC diagnostic.
---
 ChangeLog      | 5 +++++
 lib/astrxfrm.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index ee9a35da35..c32b7f5b63 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	astrxfrm: Silence gcc warning.
+	* lib/astrxfrm.c: Add #pragma GCC diagnostic.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	vasnprintf, c-vasnprintf: Silence gcc warnings.
diff --git a/lib/astrxfrm.c b/lib/astrxfrm.c
index 845f0a0711..c3e69c7cee 100644
--- a/lib/astrxfrm.c
+++ b/lib/astrxfrm.c
@@ -24,6 +24,12 @@
 #include <stdlib.h>
 #include <string.h>
 
+/* Avoid false GCC warning "function may return address of local variable"
+   regarding result and tmpbuf.  */
+#if __GNUC__ + (__GNUC_MINOR__ >= 8) > 4
+# pragma GCC diagnostic ignored "-Wreturn-local-addr"
+#endif
+
 char *
 astrxfrm (const char *s, char *resultbuf, size_t *lengthp)
 {
-- 
2.34.1


[-- Attachment #3: 0002-nstrftime-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1485 bytes --]

From 2e99a1af3bfd691462ee6525f16d620a9a82d813 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:46:47 +0200
Subject: [PATCH 2/5] nstrftime: Silence gcc warning.

* lib/nstrftime.c: Add #pragma GCC diagnostic.
---
 ChangeLog       | 5 +++++
 lib/nstrftime.c | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index c32b7f5b63..67100daa91 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	nstrftime: Silence gcc warning.
+	* lib/nstrftime.c: Add #pragma GCC diagnostic.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	astrxfrm: Silence gcc warning.
diff --git a/lib/nstrftime.c b/lib/nstrftime.c
index 2a1dd8d88d..869c97f67d 100644
--- a/lib/nstrftime.c
+++ b/lib/nstrftime.c
@@ -276,6 +276,14 @@ extern char *tzname[];
    more reliable way to accept other sets of digits.  */
 #define ISDIGIT(Ch) ((unsigned int) (Ch) - L_('0') <= 9)
 
+/* Avoid false GCC warning "'memset' specified size 18446744073709551615 exceeds
+   maximum object size 9223372036854775807", caused by insufficient data flow
+   analysis and value propagation of the 'width_add' expansion when GCC is not
+   optimizing.  Cf. <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443>.  */
+#if __GNUC__ >= 7 && !__OPTIMIZE__
+# pragma GCC diagnostic ignored "-Wstringop-overflow"
+#endif
+
 #if FPRINTFTIME
 static void
 fwrite_lowcase (FILE *fp, const CHAR_T *src, size_t len)
-- 
2.34.1


[-- Attachment #4: 0003-vasnprintf-c-vasnprintf-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1597 bytes --]

From 52cd8f06753acc038bd900fe2c34d5598de1d6a3 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:58:23 +0200
Subject: [PATCH 3/5] vasnprintf, c-vasnprintf: Silence gcc warning.

* lib/vasnprintf.c: Add #pragma GCC diagnostic.
---
 ChangeLog        |  5 +++++
 lib/vasnprintf.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 67100daa91..50a9cef230 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	vasnprintf, c-vasnprintf: Silence gcc warning.
+	* lib/vasnprintf.c: Add #pragma GCC diagnostic.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	nstrftime: Silence gcc warning.
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 007d280980..63a6cd60f3 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -927,6 +927,14 @@ divide (mpn_t a, mpn_t b, mpn_t *q)
   return roomptr;
 }
 
+/* Avoid pointless GCC warning "argument 1 value '18446744073709551615' exceeds
+   maximum object size 9223372036854775807", triggered by the use of xsum as
+   argument of malloc.  */
+# if __GNUC__ >= 7
+#  pragma GCC diagnostic push
+#  pragma GCC diagnostic ignored "-Walloc-size-larger-than="
+# endif
+
 /* Convert a bignum a >= 0, multiplied with 10^extra_zeroes, to decimal
    representation.
    Destroys the contents of a.
@@ -983,6 +991,10 @@ convert_to_decimal (mpn_t a, size_t extra_zeroes)
   return c_ptr;
 }
 
+# if __GNUC__ >= 7
+#  pragma GCC diagnostic pop
+# endif
+
 # if NEED_PRINTF_LONG_DOUBLE
 
 /* Assuming x is finite and >= 0:
-- 
2.34.1

Re: nstrftime.c fails to build due to memset overflow

[Paul Eggert]

> --- a/configure.ac
> +++ b/configure.ac
> @@ -261,6 +261,11 @@ if test $gl_gcc_warnings != no; then
>    # FP in careadlinkat.c w/gcc 10.0.1 20200205
>    gl_WARN_ADD([-Wno-return-local-addr])
>  
> +  # FIXME: remove this line when gcc improves
> +  # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
> +  # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
> +  gl_WARN_ADD([-Wno-stringop-overflow])

This patch doesn't look right either. First, 
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443> is a meta-bug 
report, and so is too vague for anybody to see what problem the FIXME is 
referring to. The FIXME should refer to a specific GCC bug report, and 
if no such bug report exists one should be created.

Second, if the bug occurs with -O0 but not with -O2, then gl_WARN_ADD 
should be invoked only if -O0 is specified. -Wstringop-overflow is a 
useful option and should not be suppressed for ordinary (-O2) builds 
simply because there's a problem in -O0 builds.

More generally, I don't think we should spend much time worrying about 
configuring with --enable-gcc-warnings and compiling with -O0. With 
today's GCC it's better to say, "don't do that". That is, either 
configure with a high quality of static checking, with 
--enable-gcc-warnings; or configure with easy-to-debug options like 
CFLAGS="-O0". The current GCC can't do both well, and there's little 
sense making application code less reliable in order to try to make GCC 
do a little bit better for this poorly-supported combination.

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

Paul Eggert wrote:
> The goal here is software reliability not pacifying compilers

But when "gcc -Wall" reports 10 warnings to me, and I don't notice
that one of them is an actual bug because I mentally discard all of
them "oh these new warnings must all be false positives by gcc",
there is something wrong in the way I worked.

This happened to me today, with a full gnulib testdir.

Hence my wish to silence the gcc warnings in gnulib, on recent glibc
systems with recent gcc versions.

> or clang, or whatever.

I'm not talking about clang here. Clang produces tons of warnings, too
much to be useful for my average use.

Bruno

silence some gcc -Wmaybe-uninitialized warnings

[Bruno Haible]

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

Hi Paul,

Regarding these two warnings:

 * -O1, -O2, -O3, -Og, -Os only:
 ../../gllib/getndelim2.c:191:23: warning: 'c' may be used uninitialized [-Wmaybe-uninitialized]
 
 * -O2, -O3 only:
 ../../gllib/canonicalize.c:385:33: warning: 'end_idx' may be used uninitialized [-Wmaybe-uninitialized]

I'm seeing these warnings, although an IF_LINT invocation is meant to suppress
them.

Some time ago, a distro guy explained on this list that IF_LINT does not
help in their situation, because the distro wants to build each package
once, with the settings that produce the best code, _and_ have the compiler
report warnings for this situation.

I'm in a similar situation: When I compile gnulib testdirs or gettext, I
don't want to have two different ways to compile it: once for getting
the warnings and once for getting the binaries, that I then run through
"make check" (optionally with valgrind).

So the IF_LINT mechanism does not work well for me either.

How about the following patches? They replace two uses of IF_LINT with
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized".
The loss for reliability is small: it silences the "maybe uninitialized"
findings for an entire function, instead of just for one variable.
The upside is that it removes the warning on glibc systems and thereby
helps the human developer (me) focusing on real warnings produced by gcc.

Is that OK with you, or do you have objections?

Bruno


[-- Attachment #2: 0001-canonicalize-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 2137 bytes --]

From df699049f9378d03cd7ecfff32873d1557baf790 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:42:00 +0200
Subject: [PATCH 1/2] canonicalize: Silence gcc warning.

* lib/canonicalize.c: Add #pragma GCC diagnostic.
(IF_LINT): Remove macro.
(canonicalize_filename_mode_stk): Remove use of IF_LINT.
---
 ChangeLog          |  7 +++++++
 lib/canonicalize.c | 14 ++++++--------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 50a9cef230..717789d423 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	canonicalize: Silence gcc warning.
+	* lib/canonicalize.c: Add #pragma GCC diagnostic.
+	(IF_LINT): Remove macro.
+	(canonicalize_filename_mode_stk): Remove use of IF_LINT.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	vasnprintf, c-vasnprintf: Silence gcc warning.
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index 010190d275..d73ee2c894 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -34,13 +34,6 @@
 #include "hash-triple.h"
 #include "xalloc.h"
 
-/* Suppress bogus GCC -Wmaybe-uninitialized warnings.  */
-#if defined GCC_LINT || defined lint
-# define IF_LINT(Code) Code
-#else
-# define IF_LINT(Code) /* empty */
-#endif
-
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
 #endif
@@ -51,6 +44,11 @@
 # define SLASHES "/"
 #endif
 
+/* Avoid false GCC warning "'end_idx' may be used uninitialized".  */
+#if __GNUC__ + (__GNUC_MINOR__ >= 7) > 4
+# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#endif
+
 /* Return true if FILE's existence can be shown, false (setting errno)
    otherwise.  Follow symbolic links.  */
 static bool
@@ -369,7 +367,7 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
               buf[n] = '\0';
 
               char *extra_buf = bufs->extra.data;
-              idx_t end_idx IF_LINT (= 0);
+              idx_t end_idx;
               if (end_in_extra_buffer)
                 end_idx = end - extra_buf;
               size_t len = strlen (end);
-- 
2.34.1


[-- Attachment #3: 0002-getndelim2-Silence-gcc-warning.patch --]
[-- Type: text/x-patch, Size: 1797 bytes --]

From 56ddb7d9a223691b914328330dad945c4511b6cb Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Thu, 18 May 2023 22:44:40 +0200
Subject: [PATCH 2/2] getndelim2: Silence gcc warning.

* lib/getndelim2.c: Add #pragma GCC diagnostic.
(IF_LINT): Remove macro.
(getndelim2): Remove use of IF_LINT.
---
 ChangeLog        |  7 +++++++
 lib/getndelim2.c | 10 ++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 717789d423..8e980741c5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2023-05-18  Bruno Haible  <bruno@clisp.org>
+
+	getndelim2: Silence gcc warning.
+	* lib/getndelim2.c: Add #pragma GCC diagnostic.
+	(IF_LINT): Remove macro.
+	(getndelim2): Remove use of IF_LINT.
+
 2023-05-18  Bruno Haible  <bruno@clisp.org>
 
 	canonicalize: Silence gcc warning.
diff --git a/lib/getndelim2.c b/lib/getndelim2.c
index d61ce3a494..b2a40ac5ce 100644
--- a/lib/getndelim2.c
+++ b/lib/getndelim2.c
@@ -50,11 +50,9 @@
 # define SSIZE_MAX ((ssize_t) (SIZE_MAX / 2))
 #endif
 
-/* Use this to suppress gcc's "...may be used before initialized" warnings. */
-#if defined GCC_LINT || defined lint
-# define IF_LINT(Code) Code
-#else
-# define IF_LINT(Code) /* empty */
+/* Avoid false GCC warning "'c' may be used uninitialized".  */
+#if __GNUC__ + (__GNUC_MINOR__ >= 7) > 4
+# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
 #endif
 
 /* The maximum value that getndelim2 can return without suffering from
@@ -108,7 +106,7 @@ getndelim2 (char **lineptr, size_t *linesize, size_t offset, size_t nmax,
       /* Here always ptr + size == read_pos + nbytes_avail.
          Also nbytes_avail > 0 || size < nmax.  */
 
-      int c IF_LINT (= 0);
+      int c;
       const char *buffer;
       size_t buffer_len;
 
-- 
2.34.1

Re: nstrftime.c fails to build due to memset overflow

[Paul Eggert]

On 5/18/23 14:50, Bruno Haible wrote:
> But when "gcc -Wall" reports 10 warnings to me, and I don't notice
> that one of them is an actual bug because I mentally discard all of
> them

If you're using -O0, then in my experience it's a mistake to also use 
--enable-gcc-warnings, as the combination generates too many false 
positives. We shouldn't waste time worrying about them, any more than we 
waste time worrying about the false positives generated by 'clang -O2' 
and --enable-gcc-warnings.

Especially we should avoid pacifing GCC by adding things like 'IF_LINT 
(= 0)', without a really good reason (such as a compiler bug report that 
we've filed). The IF_LINT can cause 'gcc -O2' (the typical case) to 
generate false negatives, and also it can cause GCC to generate 
different code for non-debug builds. Both of these are problematic, and 
we should not add 'IF_LINT (= 0)' merely to pacify 'gcc -O0'.

While I'm on the topic, we should also not worry about other 
lower-priority platforms, such as 'gcc -m32' on x86-64. Each such 
invocation can generate a boatload of false positives, and in my 
experience fixing them is more trouble than it's worth. Of course if you 
try it on a new platform and find real bugs, that's another thing - but 
my experience with running 'gcc -m32' (after a clean 'gcc -m64' build) 
is pretty negative.


PS. As it happens, I gave a lecture today on static checking of 
low-level code, with one topic being GCC's warning diagnostics and their 
limitations, so this stuff is fairly fresh in my mind.

Re: nstrftime.c fails to build due to memset overflow

[Marcus Müller]

Hey everyone,

Thanks for coming back to this.

As I said, I'm primarily not a fan of having different code paths depending on build type; that makes debugging harder than necessary.

Just a remark, in theory, you can have the cake and eat it, but afterwards your plate will be riddled with the crumbs the doing that causes¹:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
The code in question without any conditional initialization
#pragma GCC diagnostic pop

That should only disable the specific, hand-verified to be false-positive warning, and only for that piece of code, and only for GCC.

I'm not advocating for that, though - it feels a bit too verbose.

Best,
Marcus

¹ my apologies for stretching a metaphor this much. 

Am 19. Mai 2023 00:20:20 MESZ schrieb Paul Eggert <eggert@cs.ucla.edu>:
>On 5/18/23 14:50, Bruno Haible wrote:
>> But when "gcc -Wall" reports 10 warnings to me, and I don't notice
>> that one of them is an actual bug because I mentally discard all of
>> them
>
>If you're using -O0, then in my experience it's a mistake to also use --enable-gcc-warnings, as the combination generates too many false positives. We shouldn't waste time worrying about them, any more than we waste time worrying about the false positives generated by 'clang -O2' and --enable-gcc-warnings.
>
>Especially we should avoid pacifing GCC by adding things like 'IF_LINT (= 0)', without a really good reason (such as a compiler bug report that we've filed). The IF_LINT can cause 'gcc -O2' (the typical case) to generate false negatives, and also it can cause GCC to generate different code for non-debug builds. Both of these are problematic, and we should not add 'IF_LINT (= 0)' merely to pacify 'gcc -O0'.
>
>While I'm on the topic, we should also not worry about other lower-priority platforms, such as 'gcc -m32' on x86-64. Each such invocation can generate a boatload of false positives, and in my experience fixing them is more trouble than it's worth. Of course if you try it on a new platform and find real bugs, that's another thing - but my experience with running 'gcc -m32' (after a clean 'gcc -m64' build) is pretty negative.
>
>
>PS. As it happens, I gave a lecture today on static checking of low-level code, with one topic being GCC's warning diagnostics and their limitations, so this stuff is fairly fresh in my mind.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: silence some gcc -Wmaybe-uninitialized warnings

[Paul Eggert]

On 5/18/23 15:09, Bruno Haible wrote:

> Some time ago, a distro guy explained on this list that IF_LINT does not
> help in their situation, because the distro wants to build each package
> once, with the settings that produce the best code, _and_ have the compiler
> report warnings for this situation.

Yes, that's the typical situation.

> How about the following patches? They replace two uses of IF_LINT with
> #pragma GCC diagnostic ignored "-Wmaybe-uninitialized".
> The loss for reliability is small: it silences the "maybe uninitialized"
> findings for an entire function, instead of just for one variable.
> The upside is that it removes the warning on glibc systems and thereby
> helps the human developer (me) focusing on real warnings produced by gcc.

Yes, that's OK. As you say, the loss for reliability is small that way.

Re: silence some gcc -Wmaybe-uninitialized warnings

[Bruno Haible]

Paul Eggert wrote:
> Yes, that's OK. As you say, the loss for reliability is small that way.

Thanks for the review. I pushed the two patches.

Bruno

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 18/05/2023 22:43, Paul Eggert wrote:
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -261,6 +261,11 @@ if test $gl_gcc_warnings != no; then
>>     # FP in careadlinkat.c w/gcc 10.0.1 20200205
>>     gl_WARN_ADD([-Wno-return-local-addr])
>>   
>> +  # FIXME: remove this line when gcc improves
>> +  # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
>> +  # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
>> +  gl_WARN_ADD([-Wno-stringop-overflow])
> 
> This patch doesn't look right either. First,
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443> is a meta-bug
> report, and so is too vague for anybody to see what problem the FIXME is
> referring to. The FIXME should refer to a specific GCC bug report, and
> if no such bug report exists one should be created.
> 
> Second, if the bug occurs with -O0 but not with -O2, then gl_WARN_ADD
> should be invoked only if -O0 is specified. -Wstringop-overflow is a
> useful option and should not be suppressed for ordinary (-O2) builds
> simply because there's a problem in -O0 builds.
> 
> More generally, I don't think we should spend much time worrying about
> configuring with --enable-gcc-warnings and compiling with -O0. With
> today's GCC it's better to say, "don't do that". That is, either
> configure with a high quality of static checking, with
> --enable-gcc-warnings; or configure with easy-to-debug options like
> CFLAGS="-O0". The current GCC can't do both well, and there's little
> sense making application code less reliable in order to try to make GCC
> do a little bit better for this poorly-supported combination.

I'm going to keep this one.

The bug above alludes to the plethora of issues with -Wstringop-overflow,
which is also my experience.
I've lost many hours to analyzing false positives from this one
(in a very large non coreutils code base),
and I've never found a real issue identified by this warning.

cheers,
Pádraig

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 18/05/2023 22:27, Paul Eggert wrote:
> Let's revert the "avoid incorrect -Wmaybe-uninitialized warnings" patch.
> 
> --enable-gcc-warnings is designed for the default gcc -O2, and we
> shouldn't dumb down our source code for lesser platforms like "gcc -O0",
> or clang, or whatever.

OK I'll revert.
I didn't think it was too invasive,
but I see your general point about needlessly initializing.

`gcc -O0` or  `gcc -Og` is a very common requirement though
to allow effective debugging with gdb etc.
So we should to do something to make this easier.
Reconfiguring with --enable-gcc-warnings=no is neither efficient or obvious.
It's fine for us who know the build intimately, that we can:
`make CFLAGS='-O0 -ggdb3' WERROR_CFLAGS=`
though that's not obvious to most not familiar with the build,
and also has the disadvantage of distracting warnings being shown.
Perhaps we can create a debug target or something
to make this more discoverable for folks.

We'll also still have the issue with -O3 or -Ofast etc.,
but I suppose they're less likely to be set in dev builds
and so WERROR_CFLAGS= would be implicit for "dist" builds.

cheers,
Pádraig

Re: nstrftime.c fails to build due to memset overflow

[Bruno Haible]

Pádraig Brady wrote:
> I'm going to keep this one.
> ...
> I've lost many hours to analyzing false positives from this one ...
> and I've never found a real issue identified by this warning.

But can you please remove the line
  # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
since we now have a workaround in gnulib/lib/nstrftime.c ?

Bruno

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 19/05/2023 17:39, Bruno Haible wrote:
> Pádraig Brady wrote:
>> I'm going to keep this one.
>> ...
>> I've lost many hours to analyzing false positives from this one ...
>> and I've never found a real issue identified by this warning.
> 
> But can you please remove the line
>    # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
> since we now have a workaround in gnulib/lib/nstrftime.c ?

Done,

thanks,
Pádraig