On March 5, 2020 4:53:43 PM GMT+01:00, Martin Sebor <mse...@gmail.com> wrote:
>On 3/5/20 12:51 AM, Richard Biener wrote:
>> On Thu, Mar 5, 2020 at 12:49 AM Jeff Law <l...@redhat.com> wrote:
>>>
>>> On Wed, 2020-03-04 at 09:22 -0700, Martin Sebor wrote:
>>>>
>>>> I don't remember why the code in -Wrestrict unconditionally
>overwrites
>>>> the statement location rather than only when it's not available,
>but
>>>> I do remember adding conditional code like in your patch in r277076
>>>> to deal with missing location on the statement. So either your fix
>>>> or something like the hunk below might be the right solution (if we
>>>> go with the code below, abstracting it into a utility function
>might
>>>> be nice).
>>> So there's several chunks that are fairly similar to what you
>referenced in
>>> maybe_warn_pointless_strcmp. Factoring all of them into a single
>location is
>>> pretty easy.
>>>
>>> That also gives us a nice place where we can experiment with "does
>extraction of
>>> location information from the expression ever help". The answer is,
>it doesn't,
>>> at least not within our testsuite when run on x86_64.
>>>
>>> I'm hesitant to remove the code that extracts the location out of
>the expression,
>>> but could be convinced to do so.
>>>
>>> Thoughts?
>>
>> Using anything but the actual stmt location is prone to end up at
>random places
>> due to tree sharing issues, CSE and copy propagation. Simply
>consider
>>
>> char one[50];
>> char two[50];
>>
>> void
>> test_strncat (void)
>> {
>> char *p = one;
>> (void) __builtin_strcpy (p, "gh");
>> (void) __builtin_strcpy (two, "ef");
>>
>> #pragma GCC diagnostic push
>> #pragma GCC diagnostic ignored "-Wstringop-overflow="
>> #pragma GCC diagnostic ignored "-Warray-bounds"
>> (void) __builtin_strncat (p, two, 99);
>
>Interestingly, while the expression location points to p, the warning
>points to the statement:
>
>warning: ‘__builtin_strncat’ forming offset [50, 98] is out of the
>bounds [0, 50] of object ‘one’ with type ‘char[50]’ [-Warray-bounds]
> 14 | (void) __builtin_strncat (p, two, 99);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>As it happens, the %G directive in the warning_at() call replaces
>the location passed to it with that of the Gimple call argument to
>the %G directive. Removing the %G directive turns the warning into:
>
>warning: ‘__builtin_strncat’ forming offset [50, 98] is out of the
>bounds [0, 50] of object ‘one’ with type ‘char[50]’ [-Warray-bounds]
> 7 | char *p = one;
> | ^~~
>
>But the code that checks the scope of #pragma GCC diagnostic uses
>the original location passed to warning_at, not the location set
>subsequently by the %G directive, and so the two are out of synch.
Ah, I see.
>We've discussed removing the %G/%K directives before and having
>the diagnostic machinery always print the inlining context instead.
>Let me look into it for GCC 11.
>
>Martin
>
>> #pragma GCC diagnostic pop
>> }
>>
>> where we happily forward p = &one[0] to both uses injecting
>> a "faulty" location. Well, it's actually the correct location
>> computing &one[0] but irrelevant for the actual call.
>>
>> So the question is why we end up with UNKNOWN_LOCATION
>> for such call and if why we need to bother emit a diagnostic
>> at all (and why emitting it for another possibly random location is a
>good idea
>> instead of maybe simply emitting it without location).
>>
>> Richard.
>>
>>> Jeff
