Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Dumitru Ceara]

Hi everyone,

I think we've hit a false positive warning report when changing some of
our code and using a relatively new gcc version (11.2).

I tried to simplify the reproducer as much as possible and ended up with
test.c below.  With gcc 10.3 I don't see this warning, while with gcc
11.1 we seem to hit it.

I can open a bug for this but I was wondering if it's maybe already
known/reported.

Thank you!

Regards,
Dumitru

---
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/11/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-11.2.1-20210728/obj-x86_64-redhat-linux/isl-install --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 

$ cat test.c
/* Compile with:
 * # gcc -g -O2 -c -o test.o test.c
 * test.c: In function ‘foo’:
 * test.c:47:5: warning: ‘memcpy’ writing 6 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
 *    47 |     memcpy(h2 + 1, &somedata[0], 6);
 *       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 *
 * # gcc -v
 * [...]
 * gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 
 */

#include <string.h>
#include <stdint.h>

static char somedata[1024];

struct pkt {
    void *base_;
    uint16_t l4_ofs;
};

struct hdr1 {
    uint32_t h11;
};

struct hdr2 {
    uint32_t h21;
};

extern void pkt_bar(struct pkt *);

void foo(void)
{
    uint64_t stub[1024 / 8];
    struct pkt p;
 
    p.base_ = &stub[0];
    p.l4_ofs = UINT16_MAX;

    size_t size = 8;
    /* If I comment the next line out the warning goes away. */
    pkt_bar(&p);
    void *data = (char *) p.base_;
    memset(data, 0, size);

    struct hdr1 *h1 = data;
    p.l4_ofs = (uintptr_t)(h1 + 1) - (uintptr_t)p.base_;
 
    void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
    struct hdr2 *h2 = l4data;

    memcpy(h2 + 1, &somedata[0], 6);
    h2->h21 = 0;
}

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Segher Boessenkool]

On Fri, Jan 28, 2022 at 04:01:36PM +0100, Dumitru Ceara via Gcc-help wrote:
>     void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
>     struct hdr2 *h2 = l4data;
> 
>     memcpy(h2 + 1, &somedata[0], 6);

l4data can be 0, and everything false apart from there on.


Segher

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Dumitru Ceara]

On 1/28/22 16:27, Segher Boessenkool wrote:
> On Fri, Jan 28, 2022 at 04:01:36PM +0100, Dumitru Ceara via Gcc-help wrote:
>>     void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
>>     struct hdr2 *h2 = l4data;
>>
>>     memcpy(h2 + 1, &somedata[0], 6);
> 
> l4data can be 0, and everything false apart from there on.
> 

In general, yes, l4data can be 0, if p.l4_ofs == UINT16_MAX, which can
only happen if pkt_bar() changed p.base_.

But the compiler can't know that for sure and the warning makes it sound
like it's a sure thing:

"warning: ‘memcpy’ writing 6 bytes into a region of size 0 overflows the
destination"

In particular [0], we know for sure that l4data will not be NULL and we
can avoid the warning with an extra assertion.  It's just a bit
inconvenient I guess.

In any case, thanks for the quick response!

Regards,
Dumitru

[0]
https://github.com/dceara/ovn/commit/4796a6c480d5d2e35ec2e20ed0ae23ab787fa175

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Segher Boessenkool]

On Fri, Jan 28, 2022 at 05:16:58PM +0100, Dumitru Ceara wrote:
> On 1/28/22 16:27, Segher Boessenkool wrote:
> > On Fri, Jan 28, 2022 at 04:01:36PM +0100, Dumitru Ceara via Gcc-help wrote:
> >>     void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
> >>     struct hdr2 *h2 = l4data;
> >>
> >>     memcpy(h2 + 1, &somedata[0], 6);
> > 
> > l4data can be 0, and everything false apart from there on.

(Wow, writing homonyms already, I must be tired)./

> In general, yes, l4data can be 0, if p.l4_ofs == UINT16_MAX, which can
> only happen if pkt_bar() changed p.base_.

Try it with the user code fixed though?  (Hint: the warning disappears).

> But the compiler can't know that for sure and the warning makes it sound
> like it's a sure thing:
> 
> "warning: ‘memcpy’ writing 6 bytes into a region of size 0 overflows the
> destination"

Yes.

These warnings are often questionable, especially on otherwise broken
code like this, but they are far from helpful then :-(

What perhaps happens here is that the compiler realises l4data is not 0
(because otherwise the program would have undefined behaviour).  But
your program explicitly makes that happen; perhaps the compiler should
have warned for that.  (It will do a runtime error if ever that path is
taken, instead, as things are).


Segher

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Martin Sebor]

On 1/28/22 09:16, Dumitru Ceara via Gcc-help wrote:
> On 1/28/22 16:27, Segher Boessenkool wrote:
>> On Fri, Jan 28, 2022 at 04:01:36PM +0100, Dumitru Ceara via Gcc-help wrote:
>>>      void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
>>>      struct hdr2 *h2 = l4data;
>>>
>>>      memcpy(h2 + 1, &somedata[0], 6);
>>
>> l4data can be 0, and everything false apart from there on.
>>
> 
> In general, yes, l4data can be 0, if p.l4_ofs == UINT16_MAX, which can
> only happen if pkt_bar() changed p.base_.
> 
> But the compiler can't know that for sure and the warning makes it sound
> like it's a sure thing:
> 
> "warning: ‘memcpy’ writing 6 bytes into a region of size 0 overflows the
> destination"

The reason why it makes it sound like it's a sure thing is because
a) the warning sees an invalid statement in the IL, and b) because
GCC warnings work on one IL statement at a time and without regard
to the conditions under which they might be executed.

An IL statement is not always the same as a source code statement.
Some statements are also isolated from the source code even when they
don't appear there, or appear with different arguments.  That's also
what happens in this case: GCC replaces the memcpy call and
the subsequent store with two: one pair is valid  and another pair
of invalid statements.  Here's the problem in the IL the warning
sees and points out:

   <bb 4> [count: 0]:
   memcpy (4B, &somedata[0], 6);        <<< -Warray-bounds
   MEM[(struct hdr2 *)0B].h21 ={v} 0;   <<< causes path isolation
   __builtin_trap ();                       and trap to be added

Before the warning is issued GCC notices the branch of the code where
l4data is null is invalid.  It splits it into two branches: valid and
invalid and adds a trap after the invalid sequence for good measure.
The warning (which runs much later) then discovers the isolated code
is invalid and points it out.  The path isolation can be disabled by
compiling with -fno-isolate-erroneous-paths-dereference.  That also
avoids the warning.

This is an example where the invalid code isolation seems to work at
cross purposes with the warning (or at least without coordination
with it).  It's also one that illustrates an inconsistency in
the handling of invalid code (undefined behavior) in GCC.  Some
instances are eliminated (optimized away), others are replaced with
a trap, others left in place but followed by a trap (the store at
address zero), and others still are just left in place (the invalid
memcpy at address 4).

More to your point, it's also an example where warnings issued for
code that's reachable only under some conditions make it seem like
they point out unconditional bugs.

All these are known problems and all of them should be tackled
somehow.  The last one (warning for conditional code) especially
has been a sore point because it's in everyone's face.  A number
of ideas have been thrown out over the years, starting with simply
rephrasing the warning to be less affirmative.  E.g., instead of
"memcpy writing..." print:

   ‘memcpy’ may write 6 bytes into a region of size 0

The problem with this suggestion is that because most instances of
these warnings occur in some conditionally executed code, and because
all GCC warnings are meant to be taken as possible (as opposed to
definitive) indication of bugs, changing their phrasing wouldn't
actually solve anything.  All it would do is replace one with
the other, without helping you understand why or when the bug might
happen.  What I think is needed here is to mention the conditions
under which the invalid statement is executed.  With that, we should
be able to print the same context as what the static analyzer prints:

warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
    43 |     h2->h21 = 0;
       |     ~~~~~~~~^~~
   ‘foo’: events 1-3
     |
     |   39 |  *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + 
p.l4_ofs : NULL;
     |      | 
       ^
     |      | 
       |
     |      | 
       (1) following ‘false’ branch...

     |......
     |   42 | py(h2 + 1, &somedata[0], 6);
     |      |    ~~~~~~ 

     |      |       |
     |      |       (2) ...to here

     |   43 | h21 = 0;
     |      | ~~~~~~~ 

     |      |     |
     |      |     (3) dereference of NULL ‘<unknown>’

Martin

> 
> In particular [0], we know for sure that l4data will not be NULL and we
> can avoid the warning with an extra assertion.  It's just a bit
> inconvenient I guess.
> 
> In any case, thanks for the quick response!
> 
> Regards,
> Dumitru
> 
> [0]
> https://github.com/dceara/ovn/commit/4796a6c480d5d2e35ec2e20ed0ae23ab787fa175
> 

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Segher Boessenkool]

On Mon, Jan 31, 2022 at 02:49:43PM -0700, Martin Sebor wrote:
> Some statements are also isolated from the source code even when they
> don't appear there, or appear with different arguments.  That's also
> what happens in this case: GCC replaces the memcpy call and
> the subsequent store with two: one pair is valid  and another pair
> of invalid statements.  Here's the problem in the IL the warning
> sees and points out:
> 
>   <bb 4> [count: 0]:
>   memcpy (4B, &somedata[0], 6);        <<< -Warray-bounds
>   MEM[(struct hdr2 *)0B].h21 ={v} 0;   <<< causes path isolation
>   __builtin_trap ();                       and trap to be added

Yup.  And GCC does not (yet) consider that memcpy to be UB itself, or
the trap would be moved there, removing the warning.  This may be the
best we can get :-)

(This of course requires the analysis to be always correct :-) )

> More to your point, it's also an example where warnings issued for
> code that's reachable only under some conditions make it seem like
> they point out unconditional bugs.

Yes, this is hard to understand often.  It makes it easy to think GCC
is wrong and the user code is correct (esp. for the user who wrote it).


Segher

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Jonathan Wakely]

On Mon, 31 Jan 2022, 22:03 Segher Boessenkool, <segher@kernel.crashing.org>
wrote:

> On Mon, Jan 31, 2022 at 02:49:43PM -0700, Martin Sebor wrote:
> > Some statements are also isolated from the source code even when they
> > don't appear there, or appear with different arguments.  That's also
> > what happens in this case: GCC replaces the memcpy call and
> > the subsequent store with two: one pair is valid  and another pair
> > of invalid statements.  Here's the problem in the IL the warning
> > sees and points out:
> >
> >   <bb 4> [count: 0]:
> >   memcpy (4B, &somedata[0], 6);        <<< -Warray-bounds
> >   MEM[(struct hdr2 *)0B].h21 ={v} 0;   <<< causes path isolation
> >   __builtin_trap ();                       and trap to be added
>
> Yup.  And GCC does not (yet) consider that memcpy to be UB itself, or
> the trap would be moved there, removing the warning.  This may be the
> best we can get :-)
>
> (This of course requires the analysis to be always correct :-) )
>
> > More to your point, it's also an example where warnings issued for
> > code that's reachable only under some conditions make it seem like
> > they point out unconditional bugs.
>
> Yes, this is hard to understand often.  It makes it easy to think GCC
> is wrong and the user code is correct (esp. for the user who wrote it).
>


Especially true when GCC is wrong and the user code is correct as written.

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Richard Sandiford]

Martin Sebor via Gcc-help <gcc-help@gcc.gnu.org> writes:
> On 1/28/22 09:16, Dumitru Ceara via Gcc-help wrote:
>> On 1/28/22 16:27, Segher Boessenkool wrote:
>>> On Fri, Jan 28, 2022 at 04:01:36PM +0100, Dumitru Ceara via Gcc-help wrote:
>>>>      void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
>>>>      struct hdr2 *h2 = l4data;
>>>>
>>>>      memcpy(h2 + 1, &somedata[0], 6);
>>>
>>> l4data can be 0, and everything false apart from there on.
>>>
>> 
>> In general, yes, l4data can be 0, if p.l4_ofs == UINT16_MAX, which can
>> only happen if pkt_bar() changed p.base_.
>> 
>> But the compiler can't know that for sure and the warning makes it sound
>> like it's a sure thing:
>> 
>> "warning: ‘memcpy’ writing 6 bytes into a region of size 0 overflows the
>> destination"
>
> The reason why it makes it sound like it's a sure thing is because
> a) the warning sees an invalid statement in the IL, and b) because
> GCC warnings work on one IL statement at a time and without regard
> to the conditions under which they might be executed.
>
> An IL statement is not always the same as a source code statement.
> Some statements are also isolated from the source code even when they
> don't appear there, or appear with different arguments.  That's also
> what happens in this case: GCC replaces the memcpy call and
> the subsequent store with two: one pair is valid  and another pair
> of invalid statements.  Here's the problem in the IL the warning
> sees and points out:
>
>    <bb 4> [count: 0]:
>    memcpy (4B, &somedata[0], 6);        <<< -Warray-bounds
>    MEM[(struct hdr2 *)0B].h21 ={v} 0;   <<< causes path isolation
>    __builtin_trap ();                       and trap to be added
>
> Before the warning is issued GCC notices the branch of the code where
> l4data is null is invalid.  It splits it into two branches: valid and
> invalid and adds a trap after the invalid sequence for good measure.
> The warning (which runs much later) then discovers the isolated code
> is invalid and points it out.  The path isolation can be disabled by
> compiling with -fno-isolate-erroneous-paths-dereference.  That also
> avoids the warning.
>
> This is an example where the invalid code isolation seems to work at
> cross purposes with the warning (or at least without coordination
> with it).  It's also one that illustrates an inconsistency in
> the handling of invalid code (undefined behavior) in GCC.  Some
> instances are eliminated (optimized away), others are replaced with
> a trap, others left in place but followed by a trap (the store at
> address zero), and others still are just left in place (the invalid
> memcpy at address 4).
>
> More to your point, it's also an example where warnings issued for
> code that's reachable only under some conditions make it seem like
> they point out unconditional bugs.
>
> All these are known problems and all of them should be tackled
> somehow.  The last one (warning for conditional code) especially
> has been a sore point because it's in everyone's face.  A number
> of ideas have been thrown out over the years, starting with simply
> rephrasing the warning to be less affirmative.  E.g., instead of
> "memcpy writing..." print:
>
>    ‘memcpy’ may write 6 bytes into a region of size 0
>
> The problem with this suggestion is that because most instances of
> these warnings occur in some conditionally executed code, and because
> all GCC warnings are meant to be taken as possible (as opposed to
> definitive) indication of bugs, changing their phrasing wouldn't
> actually solve anything.  All it would do is replace one with
> the other, without helping you understand why or when the bug might
> happen.

I agree that changing the wording doesn't solve the whole problem,
but I think it does solve something.  At the moment, we're effectively
asking each individual user to be aware of the context above, to know what
meaning is being attached to the present tense.  Making the message more
equivocal would at least suggest that the compiler doesn't “know”.

It's not the pass's “fault” that it can't tell how closely the IL
it sees matches the original code.  But it is GCC's “fault” as a whole,
not the user's, and I think that's what matters here.

> What I think is needed here is to mention the conditions
> under which the invalid statement is executed.  With that, we should
> be able to print the same context as what the static analyzer prints:
>
> warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
>     43 |     h2->h21 = 0;
>        |     ~~~~~~~~^~~
>    ‘foo’: events 1-3
>      |
>      |   39 |  *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + 
> p.l4_ofs : NULL;
>      |      | 
>        ^
>      |      | 
>        |
>      |      | 
>        (1) following ‘false’ branch...
>
>      |......
>      |   42 | py(h2 + 1, &somedata[0], 6);
>      |      |    ~~~~~~ 
>
>      |      |       |
>      |      |       (2) ...to here
>
>      |   43 | h21 = 0;
>      |      | ~~~~~~~ 
>
>      |      |     |
>      |      |     (3) dereference of NULL ‘<unknown>’

I agree this would be better, but I don't think it should hold back
tweaks to the error messages in the meantime.  (And if the tracing
was an optional feature, we'd still want wording that makes sense
when the feature is turned off.)

I realise this is rehashing an old discussion, sorry.  But it seems
like it's a discussion that gets rehashed precisely because it's
about an issue that users keep hitting.

Thanks,
Richard

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Segher Boessenkool]

On Tue, Feb 01, 2022 at 05:18:57AM +0000, Richard Sandiford wrote:
> I agree that changing the wording doesn't solve the whole problem,
> but I think it does solve something.  At the moment, we're effectively
> asking each individual user to be aware of the context above, to know what
> meaning is being attached to the present tense.  Making the message more
> equivocal would at least suggest that the compiler doesn't “know”.

The compiler *does* know (in this case), but only for one internal copy
of the code.  The warning message (which could be an error in this case,
the code replaced with a trap: a dest or source that does not point at
an object is undefined behaviour, for memcpy).

But the error message suggests that this happens on *every* execution of
the memcpy.  Which is wrong, misleading, the cause of all the confusion
here.

> It's not the pass's “fault” that it can't tell how closely the IL
> it sees matches the original code.  But it is GCC's “fault” as a whole,
> not the user's, and I think that's what matters here.

Yeah.

> > What I think is needed here is to mention the conditions
> > under which the invalid statement is executed.  With that, we should
> > be able to print the same context as what the static analyzer prints:

> I agree this would be better, but I don't think it should hold back
> tweaks to the error messages in the meantime.  (And if the tracing
> was an optional feature, we'd still want wording that makes sense
> when the feature is turned off.)

Exactly.  It shouldn't be all that hard to steer the user towards
discovering what is wrong with his code, instead of causing the user to
think GCC is wrong again.

> I realise this is rehashing an old discussion, sorry.  But it seems
> like it's a discussion that gets rehashed precisely because it's
> about an issue that users keep hitting.

Agreed, this needs to be fixed.  The bigger issue is communication, the
way diagnostic messages are phrased, not the analysis itself :-)


Segher

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Martin Sebor]

On 1/31/22 22:18, Richard Sandiford wrote:
> Martin Sebor via Gcc-help <gcc-help@gcc.gnu.org> writes:
>> On 1/28/22 09:16, Dumitru Ceara via Gcc-help wrote:
>>> On 1/28/22 16:27, Segher Boessenkool wrote:
>>>> On Fri, Jan 28, 2022 at 04:01:36PM +0100, Dumitru Ceara via Gcc-help wrote:
>>>>>       void *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ + p.l4_ofs : NULL;
>>>>>       struct hdr2 *h2 = l4data;
>>>>>
>>>>>       memcpy(h2 + 1, &somedata[0], 6);
>>>>
>>>> l4data can be 0, and everything false apart from there on.
>>>>
>>>
>>> In general, yes, l4data can be 0, if p.l4_ofs == UINT16_MAX, which can
>>> only happen if pkt_bar() changed p.base_.
>>>
>>> But the compiler can't know that for sure and the warning makes it sound
>>> like it's a sure thing:
>>>
>>> "warning: ‘memcpy’ writing 6 bytes into a region of size 0 overflows the
>>> destination"
>>
>> The reason why it makes it sound like it's a sure thing is because
>> a) the warning sees an invalid statement in the IL, and b) because
>> GCC warnings work on one IL statement at a time and without regard
>> to the conditions under which they might be executed.
>>
>> An IL statement is not always the same as a source code statement.
>> Some statements are also isolated from the source code even when they
>> don't appear there, or appear with different arguments.  That's also
>> what happens in this case: GCC replaces the memcpy call and
>> the subsequent store with two: one pair is valid  and another pair
>> of invalid statements.  Here's the problem in the IL the warning
>> sees and points out:
>>
>>     <bb 4> [count: 0]:
>>     memcpy (4B, &somedata[0], 6);        <<< -Warray-bounds
>>     MEM[(struct hdr2 *)0B].h21 ={v} 0;   <<< causes path isolation
>>     __builtin_trap ();                       and trap to be added
>>
>> Before the warning is issued GCC notices the branch of the code where
>> l4data is null is invalid.  It splits it into two branches: valid and
>> invalid and adds a trap after the invalid sequence for good measure.
>> The warning (which runs much later) then discovers the isolated code
>> is invalid and points it out.  The path isolation can be disabled by
>> compiling with -fno-isolate-erroneous-paths-dereference.  That also
>> avoids the warning.
>>
>> This is an example where the invalid code isolation seems to work at
>> cross purposes with the warning (or at least without coordination
>> with it).  It's also one that illustrates an inconsistency in
>> the handling of invalid code (undefined behavior) in GCC.  Some
>> instances are eliminated (optimized away), others are replaced with
>> a trap, others left in place but followed by a trap (the store at
>> address zero), and others still are just left in place (the invalid
>> memcpy at address 4).
>>
>> More to your point, it's also an example where warnings issued for
>> code that's reachable only under some conditions make it seem like
>> they point out unconditional bugs.
>>
>> All these are known problems and all of them should be tackled
>> somehow.  The last one (warning for conditional code) especially
>> has been a sore point because it's in everyone's face.  A number
>> of ideas have been thrown out over the years, starting with simply
>> rephrasing the warning to be less affirmative.  E.g., instead of
>> "memcpy writing..." print:
>>
>>     ‘memcpy’ may write 6 bytes into a region of size 0
>>
>> The problem with this suggestion is that because most instances of
>> these warnings occur in some conditionally executed code, and because
>> all GCC warnings are meant to be taken as possible (as opposed to
>> definitive) indication of bugs, changing their phrasing wouldn't
>> actually solve anything.  All it would do is replace one with
>> the other, without helping you understand why or when the bug might
>> happen.
> 
> I agree that changing the wording doesn't solve the whole problem,
> but I think it does solve something.  At the moment, we're effectively
> asking each individual user to be aware of the context above, to know what
> meaning is being attached to the present tense.  Making the message more
> equivocal would at least suggest that the compiler doesn't “know”.

The compiler can never "know" for certain if any statement will be
executed.  Every warning message that involves control or data flow
must be interpreted in the context of the surrounding code, whether
it's an expression, statement, or the whole function.  Every warning
message must necessarily also be understood as only suggesting that
"there may be a problem" in the program rather than "there definitely
is a bug."

There's plenty of literature out there that explains this, including
the GCC manual, so I'd expect most C/C++ programmers to understand
that.  I don't think that rewording every warning message just to
drill that message home and without addressing the bigger problem
would make enough of a difference to justify the effort. (Users
don't feel any better about -Wmaybe-uninitialized false positives
than about -Wuninitalized, and they've periodically argued to
remove the former from -Wall despite its equivocal phrasing.)

(That said, with my tongue in cheek, the original phrasing of these
warnings (up to GCC 6) was:

   call to 'memcpy' will always overflow destination buffer

so some progress has been made on this front ;-)

> 
> It's not the pass's “fault” that it can't tell how closely the IL
> it sees matches the original code.  But it is GCC's “fault” as a whole,
> not the user's, and I think that's what matters here.

I agree.  I think there are at least two underlying problems: users
expecting every warning message to point out an actual bug in their
code, and GCC failing to somehow indicate the conditional nature of
the problems in the messages.

>> What I think is needed here is to mention the conditions
>> under which the invalid statement is executed.  With that, we should
>> be able to print the same context as what the static analyzer prints:
>>
>> warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
>>      43 |     h2->h21 = 0;
>>         |     ~~~~~~~~^~~
>>     ‘foo’: events 1-3
>>       |
>>       |   39 |  *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ +
>> p.l4_ofs : NULL;
>>       |      |
>>         ^
>>       |      |
>>         |
>>       |      |
>>         (1) following ‘false’ branch...
>>
>>       |......
>>       |   42 | py(h2 + 1, &somedata[0], 6);
>>       |      |    ~~~~~~
>>
>>       |      |       |
>>       |      |       (2) ...to here
>>
>>       |   43 | h21 = 0;
>>       |      | ~~~~~~~
>>
>>       |      |     |
>>       |      |     (3) dereference of NULL ‘<unknown>’
> 
> I agree this would be better, but I don't think it should hold back
> tweaks to the error messages in the meantime.  (And if the tracing
> was an optional feature, we'd still want wording that makes sense
> when the feature is turned off.)
> 
> I realise this is rehashing an old discussion, sorry.  But it seems
> like it's a discussion that gets rehashed precisely because it's
> about an issue that users keep hitting.

It's an old issue that users are running into increasingly often
as the middle end warnings keep getting "smarter."  Although that's
not the case here, the switch to Ranger in GCC 12 in particular has
made the data flow analysis used by warnings like -Wstringop-overflow
much more accurate.  That's exactly what we wanted; what we
underestimated is the extent to which the warnings expose unreachable
statements, or those that are reachable under complex conditions, or
that are synthesized by GCC seemingly out of thin air.  All these are
problematic, especially when the conditions involve layers of libstdc++
(or other third party) template code.  From a user's point of view,
there's no difference between the two.

Indicating the flow like in the above isn't too difficult.  I put
together a prototype that does that for some warnings, including
-Wmaybe-uninitialized and a subset of -Wstringop-overflow.  What
it doesn't capture (and what I think is essential) is conditions
involved in determining ranges of values like pointer offsets,
array bounds, or memcpy sizes.  To capture those we either need
to extend Ranger or develop a separate range analyzer just for
these warnings.

Martin

> 
> Thanks,
> Richard

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Jonathan Wakely]

On Tue, 1 Feb 2022 at 23:55, Martin Sebor via Gcc-help
<gcc-help@gcc.gnu.org> wrote:
>
> On 1/31/22 22:18, Richard Sandiford wrote:
> > I agree that changing the wording doesn't solve the whole problem,
> > but I think it does solve something.  At the moment, we're effectively
> > asking each individual user to be aware of the context above, to know what
> > meaning is being attached to the present tense.  Making the message more
> > equivocal would at least suggest that the compiler doesn't “know”.
>
> The compiler can never "know" for certain if any statement will be
> executed.  Every warning message that involves control or data flow
> must be interpreted in the context of the surrounding code, whether
> it's an expression, statement, or the whole function.  Every warning
> message must necessarily also be understood as only suggesting that
> "there may be a problem" in the program rather than "there definitely
> is a bug."
>
> There's plenty of literature out there that explains this, including
> the GCC manual, so I'd expect most C/C++ programmers to understand
> that.

I disagree. So does the manual:

      -Warray-bounds
      -Warray-bounds=n
          This option is only active when -ftree-vrp is active
(default for -O2 and above). It
          warns about subscripts to arrays that are always out of
bounds. This warning is
          enabled by -Wall.

If we're going to claim that it's common knowledge that warnings are
always contextual and not definite, can we not use language like
"always out of bounds"? How else am I supposed to read that other than
"always"? Always, under specific conditions? That's not what the word
means.

And the problem with saying they must be interpreted in the context of
the surrounding code is that a compiler can completely restructure the
code, and create unreachable paths that are not present in the
surrounding code. We have numerous examples where the surrounding code
makes it very clear that the problem cannot happen, and yet GCC warns,
e.g. https://gcc.gnu.org/PR104017#c1

So really you have to interpret the warning in the context of some
*different* piece of code that was dreamt up by the compiler, which is
not at all obvious unless you know how to read the entrails in a GCC
dump file.






  I don't think that rewording every warning message just to
> drill that message home and without addressing the bigger problem
> would make enough of a difference to justify the effort. (Users
> don't feel any better about -Wmaybe-uninitialized false positives
> than about -Wuninitalized, and they've periodically argued to
> remove the former from -Wall despite its equivocal phrasing.)
>
> (That said, with my tongue in cheek, the original phrasing of these
> warnings (up to GCC 6) was:
>
>    call to 'memcpy' will always overflow destination buffer
>
> so some progress has been made on this front ;-)
>
> >
> > It's not the pass's “fault” that it can't tell how closely the IL
> > it sees matches the original code.  But it is GCC's “fault” as a whole,
> > not the user's, and I think that's what matters here.
>
> I agree.  I think there are at least two underlying problems: users
> expecting every warning message to point out an actual bug in their
> code, and GCC failing to somehow indicate the conditional nature of
> the problems in the messages.
>
> >> What I think is needed here is to mention the conditions
> >> under which the invalid statement is executed.  With that, we should
> >> be able to print the same context as what the static analyzer prints:
> >>
> >> warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
> >>      43 |     h2->h21 = 0;
> >>         |     ~~~~~~~~^~~
> >>     ‘foo’: events 1-3
> >>       |
> >>       |   39 |  *l4data = p.l4_ofs != UINT16_MAX ? (char *) p.base_ +
> >> p.l4_ofs : NULL;
> >>       |      |
> >>         ^
> >>       |      |
> >>         |
> >>       |      |
> >>         (1) following ‘false’ branch...
> >>
> >>       |......
> >>       |   42 | py(h2 + 1, &somedata[0], 6);
> >>       |      |    ~~~~~~
> >>
> >>       |      |       |
> >>       |      |       (2) ...to here
> >>
> >>       |   43 | h21 = 0;
> >>       |      | ~~~~~~~
> >>
> >>       |      |     |
> >>       |      |     (3) dereference of NULL ‘<unknown>’
> >
> > I agree this would be better, but I don't think it should hold back
> > tweaks to the error messages in the meantime.  (And if the tracing
> > was an optional feature, we'd still want wording that makes sense
> > when the feature is turned off.)
> >
> > I realise this is rehashing an old discussion, sorry.  But it seems
> > like it's a discussion that gets rehashed precisely because it's
> > about an issue that users keep hitting.
>
> It's an old issue that users are running into increasingly often
> as the middle end warnings keep getting "smarter."  Although that's
> not the case here, the switch to Ranger in GCC 12 in particular has
> made the data flow analysis used by warnings like -Wstringop-overflow
> much more accurate.  That's exactly what we wanted; what we
> underestimated is the extent to which the warnings expose unreachable
> statements, or those that are reachable under complex conditions, or
> that are synthesized by GCC seemingly out of thin air.  All these are
> problematic, especially when the conditions involve layers of libstdc++
> (or other third party) template code.  From a user's point of view,
> there's no difference between the two.
>
> Indicating the flow like in the above isn't too difficult.  I put
> together a prototype that does that for some warnings, including
> -Wmaybe-uninitialized and a subset of -Wstringop-overflow.  What
> it doesn't capture (and what I think is essential) is conditions
> involved in determining ranges of values like pointer offsets,
> array bounds, or memcpy sizes.  To capture those we either need
> to extend Ranger or develop a separate range analyzer just for
> these warnings.
>
> Martin
>
> >
> > Thanks,
> > Richard
>

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Segher Boessenkool]

On Wed, Feb 02, 2022 at 10:12:44AM +0000, Jonathan Wakely wrote:
> On Tue, 1 Feb 2022 at 23:55, Martin Sebor via Gcc-help
> > There's plenty of literature out there that explains this, including
> > the GCC manual, so I'd expect most C/C++ programmers to understand
> > that.
> 
> I disagree. So does the manual:
> 
>       -Warray-bounds
>       -Warray-bounds=n
>           This option is only active when -ftree-vrp is active
> (default for -O2 and above). It
>           warns about subscripts to arrays that are always out of
> bounds. This warning is
>           enabled by -Wall.

Yes, that is wrong as written.  Please open a PR?

> If we're going to claim that it's common knowledge that warnings are
> always contextual and not definite, can we not use language like
> "always out of bounds"? How else am I supposed to read that other than
> "always"? Always, under specific conditions? That's not what the word
> means.

I agree.

It is essentially always possible to phrase an error to be friendlier as
well as much more correct at the same time, without being much more
verbose.  This takes effort, but it is an investment that pays off
greatly and immediately.


Segher

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Jonathan Wakely]

On Wed, 2 Feb 2022 at 18:35, Segher Boessenkool wrote:
>
> On Wed, Feb 02, 2022 at 10:12:44AM +0000, Jonathan Wakely wrote:
> > On Tue, 1 Feb 2022 at 23:55, Martin Sebor via Gcc-help
> > > There's plenty of literature out there that explains this, including
> > > the GCC manual, so I'd expect most C/C++ programmers to understand
> > > that.
> >
> > I disagree. So does the manual:
> >
> >       -Warray-bounds
> >       -Warray-bounds=n
> >           This option is only active when -ftree-vrp is active
> > (default for -O2 and above). It
> >           warns about subscripts to arrays that are always out of
> > bounds. This warning is
> >           enabled by -Wall.
>
> Yes, that is wrong as written.  Please open a PR?

Done: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104355

Re: Potentially false-positive -Wstringop-overflow= warning with gcc >= 11.1

[Segher Boessenkool]

On Tue, Feb 01, 2022 at 04:54:17PM -0700, Martin Sebor wrote:
> On 1/31/22 22:18, Richard Sandiford wrote:
> >I agree that changing the wording doesn't solve the whole problem,
> >but I think it does solve something.  At the moment, we're effectively
> >asking each individual user to be aware of the context above, to know what
> >meaning is being attached to the present tense.  Making the message more
> >equivocal would at least suggest that the compiler doesn't “know”.
> 
> The compiler can never "know" for certain if any statement will be
> executed.

It can when it sees the whole source code.  Since noreturn attributes
are not required for functions that do not return, you can not tell
otherwise, sure.  For warnings you can assume any function that does not
say it does not not return does return, but yes you cannot know.

> Every warning message that involves control or data flow
> must be interpreted in the context of the surrounding code, whether
> it's an expression, statement, or the whole function.

Of course.  But if a warning message says "your code is wrong", it is a
bit of a leap to read that as "your code might be wrong", or "it looks
suspicious", etc.

> Every warning
> message must necessarily also be understood as only suggesting that
> "there may be a problem" in the program rather than "there definitely
> is a bug."

But that is for a very different reason: every warning also has false
positives.  If not, it should be an error, not a warning!

> There's plenty of literature out there that explains this, including
> the GCC manual, so I'd expect most C/C++ programmers to understand
> that.

But if your warning message says "your code is wrong", you cannot blame
the user for understanding that exactly as written.

> I don't think that rewording every warning message just to
> drill that message home and without addressing the bigger problem
> would make enough of a difference to justify the effort.

Our diagnostic messages should not lie.  This is definitely worth the
effort.  Yes, it can be hard, but the whole *goal* is to help the user,
so it makes sense to put in some effort to do help the user, instead of
frustrating him/her.

> (Users
> don't feel any better about -Wmaybe-uninitialized false positives
> than about -Wuninitalized, and they've periodically argued to
> remove the former from -Wall despite its equivocal phrasing.)

Of course they are not happy to see a messsage like "this or that may be
used uninitialised", but in what world does that make it okay for the
compiler to see "thhis *is* used uninitialised"?  That makes no sense.

> I agree.  I think there are at least two underlying problems: users
> expecting every warning message to point out an actual bug in their
> code,

You make that problem worse by telling the user that his code is wrong
(instead of saying it loooks suspicious, it may be wrong).

> and GCC failing to somehow indicate the conditional nature of
> the problems in the messages.

Yes.

We should have a way to say "this is UB if it ever is executed".  And we
do have one such way: we insert an abort before the statement.  Which we
do not do here.  If we want to not do this (the aborts are kind of
unfriendly after all), it would be nice to have a helper warning message
function warning_if_executed or similar, that would take care of the
phrasing and make it consistently correct for all such warnings.


Segher