Re: [PATCH] suppress -Wstringop-overflow when no-warning is set (PR 83508)

[Jeff Law <law at redhat dot com>]

On 01/11/2018 04:49 PM, Martin Sebor wrote:
> On 01/11/2018 04:24 PM, Jeff Law wrote:
>> On 01/10/2018 01:26 PM, Martin Sebor wrote:
>>> To avoid issuing duplicate warnings for the same function call
>>> in the source code the -Wrestrict warning code makes sure
>>> the no-warning bit is propagated between trees and GIMPLE and
>>> tested before issuing a warning.  But the warning also detects
>>> some of the same problems as -Wstringop-overflow, and that
>>> warning was not updated to pay attention to the no-warning bit.
>>> This can in turn lead to two warnings for what boils down to
>>> the same bug.  The warnings can be confusing when the first
>>> one references the function as it appears in the source code
>>> and the second one the one it was transformed to by GCC after
>>> the first warning was issued.
>>>
>>> The attached patch corrects this oversight by having
>>> the buffer overflow checker test the no-warning bit and skip
>>> issuing a diagnostic.  (The function that does the overflow
>>> checking still runs so that it can continue to indicate to its
>>> callers whether an overflow has been detected.)
>>>
>>> Bootstrap on x86_64-linux in progress.
>>>
>>> Martin
>>>
>>> gcc-83508.diff
>>>
>>>
>>> PR other/83508 - c-c++-common/Wrestrict.c fails since r255836
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     PR other/83508
>>>     * gcc.dg/Wstringop-overflow-2.c: New test.
>>>
>>> gcc/ChangeLog:
>>>
>>>     PR other/83508
>>>     * builtins.c (check_access): Avoid warning when the no-warning bit
>>>     is set.
>> Would it be better to check for TREE_NO_WARNING at the very start of
>> check_access rather than sprinkling it at various sites later in the
>> code?
> 
> The function is called from a couple of places to check that
> there is no overflow and avoid expansion (we discussed this
> not too long ago when I enhanced compute_objsize() to handle
> ranges).  I didn't want to change that (I mention it in the
> last sentence above.)
OK.  Just wanted to raise it as a possibility.

> 
> Looking forward, I would like to see these middle-end checkers
> used to avoid making certain kinds of transformations that we
> know are dangerous (we discussed emitting __builtin_trap or
> __builtin_unreachable) for some such cases.  I realize they
> aren't suitable for it quite it yet and will need work to make
> them so (assuming we can agree on that approach), but I mention
> it to explain what I was thinking when I sprinkled the tests
> in check_access() the way I did.
Understood.

> 
> As an aside, even though I think GCC should issue only one
> warning per function, I'm not sure that -Wrestrict should
> trump -Wstringop-overflow.  It seems like it should be the
> other way around because the latter is more severe.  That's
> also how it worked before -Wrestrict was moved into its own
> pass.  To make it work like that again, -Wstringop-overflow
> would need to run either before -Wrestrict or at the same
> time.  Maybe it should be moved in GCC 9.
I think we're OK for gcc-8.  We can revisit ordering and such for gcc-9.

FWIW, I went ahead and committed your patch to the trunk.

jeff