On 6/30/21 5:35 PM, David Malcolm wrote:
> On Wed, 2021-06-30 at 13:45 -0600, Martin Sebor wrote:
>> On 6/30/21 9:39 AM, Martin Sebor wrote:
>>> Ping.  Attached is the same patch rebased on top the latest trunk.
>>
>> Please see the attached patch instead.  The previous one had typo
>> in it.
>>
>>>
>>> As discussed in the review of Aldy's recent changes to the backwards
>>> threader, he has run into the same bug the patch fixes.  Getting this
>>> patch set reviewed and approved would be helpful in keeping him from
>>> having to work around the bug.
>>>
>>> https://gcc.gnu.org/pipermail/gcc/2021-June/236608.html
>>>
>>> On 6/10/21 5:27 PM, Martin Sebor wrote:
>>>> This diff removes the uses of %G and %K from all warning_at() calls
>>>> throughout GCC front end and middle end.  The inlining context is
>>>> included in diagnostic output whenever it's present.
>>>
>>
> 
> Thanks for writing the patch.
> 
> I went through the full patch, though my eyes may have glazed over in
> places at all of the %G and %K removals.  I *think* you got them mostly
> correct, apart from the following possible issues and nits...
> 
>> diff --git a/gcc/expr.c b/gcc/expr.c
>> index 025033c9ecf..b9fe1cf91d7 100644
>> --- a/gcc/expr.c
>> +++ b/gcc/expr.c
> 
> [...]
> 
>> @@ -11425,10 +11425,10 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
>>   					 DECL_ATTRIBUTES (fndecl))) != NULL)
>>   	  {
>>   	    const char *ident = lang_hooks.decl_printable_name (fndecl, 1);
>> -	    warning_at (tree_nonartificial_location (exp),
>> +	    warning_at (EXPR_LOCATION (exp),
> 
> Are we preserving the existing behavior for
> __attribute__((__artificial__)) here?
> Is this behavior handled somewhere else in the patch kit?

Yes.  The warning infrastructure (set_inlining_locations) uses
the location of the site into which the statement has been inlined
regardless of whether the inlined function is artificial.

> 
>>   			OPT_Wattribute_warning,
>> -			"%Kcall to %qs declared with attribute warning: %s",
>> -			exp, identifier_to_locale (ident),
>> +			"call to %qs declared with attribute warning: %s",
>> +			identifier_to_locale (ident),
>>   			TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
>>   	  }
>>   
> 
> [...]
> 
>> diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
>> index 02771e4cd60..efb8db98393 100644
>> --- a/gcc/gimple-ssa-warn-restrict.c
>> +++ b/gcc/gimple-ssa-warn-restrict.c
> 
> [...]
> 
>> @@ -1689,7 +1689,7 @@ maybe_diag_access_bounds (gimple *call, tree func, int strict,
>>   			  const builtin_memref &ref, offset_int wroff,
>>   			  bool do_warn)
>>   {
>> -  location_t loc = gimple_or_expr_nonartificial_location (call, ref.ptr);
>> +  location_t loc = gimple_location (call);
> 
> Likewise here.
> 
>> @@ -2065,7 +2065,7 @@ check_bounds_or_overlap (range_query *query,
>>   	}
>>       }
>>   
>> -  location_t loc = gimple_or_expr_nonartificial_location (call, dst);
>> +  location_t loc = gimple_location (call);
> 
> Likewise here.
> 
> [...]
> 
>> diff --git a/gcc/testsuite/g++.dg/warn/Wdtor1.s b/gcc/testsuite/g++.dg/warn/Wdtor1.s
>> new file mode 100644
>> index 00000000000..e69de29bb2d
> 
> Is this an empty .s file?  Was this a misfire with "git add"?

It must have been, yes.

> 
> [...]
>> @@ -90,8 +90,8 @@ NOIPA void warn_g2 (struct A *p)
>>     g2 (p);
>>   }
>>   
>> -// { dg-message "inlined from 'g2'" "" { target *-*-* } 0 }
>> -// { dg-message "inlined from 'warn_g2'" "" { target *-*-* } 0 }
>> +// { dg-message "inlined from 'g2'" "note on line 93" { target *-*-* } 0 }
>> +// { dg-message "inlined from 'warn_g2'" "note on line 94" { target *-*-* } 0 }
> 
> You've added descriptions to disambiguate all of the various directives
> on line 0, which is good, but I don't like the use of line numbers in
> the descriptions, since it will get very confusing if the numbering
> changes.
> 
> Would it work to use the message text as the description e.g.
> 
>    // { dg-message "inlined from 'warn_g2'" "inlined from 'warn_g2'" { target *-*-* } 0 }
> 
> or somesuch?

It would certainly work, they're just informational labels printed
by DejaGnu when the assertions fail.  I added them to help me see
what they went with while working with the test.  I'm not concerned
about the line numbers changing.  If they do and someone notices,
they can update them, the same way they might want to if they
rename the functions they're inlined into.

> 
> 
>> diff --git a/gcc/testsuite/gcc.dg/Wfree-nonheap-object-5.c b/gcc/testsuite/gcc.dg/Wfree-nonheap-object-5.c
>> new file mode 100644
>> index 00000000000..979e1e3d78f
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/Wfree-nonheap-object-5.c
>> @@ -0,0 +1,45 @@
>> +/* Similar to Wfree-nonheap-object-4.c but without system headers:
>> +   verify that warnings for the same call site from distinct callers
>> +   include the correct function names in the inlining stack.
>> +   { dg-do compile }
>> +   { dg-options "-O2 -Wall" } */
>> +
>> +struct A
>> +{
>> +  void *p;
>> +};
>> +
>> +void f0 (struct A *p)
>> +{
>> +  __builtin_free (p->p);      // { dg-warning "\\\[-Wfree-nonheap-object" }
>> +}
>> +
>> +// Expect two instances of the text below:
>> +// { dg-regexp "In function 'f0'," "f0 prefix" { target *-*-* } 0 }
>> +// { dg-regexp "In function 'f0'," "f0 prefix" { target *-*-* } 0 }
>> +
>> +void f1 (struct A *p) { f0 (p); }
>> +void f2 (struct A *p) { f1 (p); }
>> +
>> +extern int array[];
>> +// Also expect two instances of the note:
>> +// { dg-regexp "declared here" "note on line 24" { target *-*-* } .-2 }
>> +// { dg-regexp "declared here" "note on line 24" { target *-*-* } .-3 }
>> +
>> +void foo (struct A *p)
>> +{
>> +  p->p = array + 1;
>> +  f0 (p);
>> +}
>> +
>> +// { dg-message "inlined from 'foo'" "note on line 35" { target *-*-* } 0 }
>> +
>> +
>> +void bar (struct A *p)
>> +{
>> +  p->p = array + 2;
>> +  f1 (p);
>> +}
>> +
>> +// { dg-message "inlined from 'f1'" "note on line 44" { target *-*-* } 0 }
>> +// { dg-message "inlined from 'bar'" "note on line 45" { target *-*-* } 0 }
> 
> Likewise here with the hardcoded line numbers.
> 
> [...]
> 
>> diff --git a/gcc/testsuite/gcc.dg/pragma-diag-9.c b/gcc/testsuite/gcc.dg/pragma-diag-9.c
>> new file mode 100644
>> index 00000000000..d7eac558128
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pragma-diag-9.c
>> @@ -0,0 +1,134 @@
>> +/* Verify that #pragma GCC diagnostic down the inlining stack suppresses
>> +   a warning that would otherwise be issued for inlined calls higher up
>> +   the inlining stack.
>> +   { dg-do compile }
>> +   { dg-options "-O2 -Wall -Wno-array-bounds" } */
>> +
>> +extern void* memset (void*, int, __SIZE_TYPE__);
>> +
>> +static void warn0 (int *p)
>> +{
>> +  memset (p, __LINE__, 3);    // { dg-warning "\\\[-Wstringop-overflow" }
>> +}
>> +
>> +static void warn1 (int *p)
>> +{
>> +  warn0 (p + 1);
>> +}
>> +
>> +static void warn2 (int *p)
>> +{
>> +  warn1 (p + 1);
>> +}
>> +
>> +int a2[2];                    // { dg-message "at offset 12 into destination object 'a2' of size 8" }
>> +
>> +void warn3 (void)
>> +{
>> +  warn2 (a2 + 1);
>> +}
> 
> After reading through this and trying to grok it, I see that this file
> logically can be split into several parts: the "warn*" functions, then
> the "nowarn*_ignore0" functions, then the "nowarn*_ignore_1" functions
> etc.
> 
> Please add some kind of separator comment between each of these parts
> to make it easy for the reader to see this structure.

Sure, I've added a comment.

Attached is the revised patch for reference.  Since it just removes
the uses of the %K and %G directives made redundant by the first
patch in the series I'll go ahead and commit it as obvious in a day
or so after patch 1 unless someone has further questions or requests
for changes.

Thanks!

Martin