Re: [PING][PATCH 2/4] remove %G and %K from calls in front end and middle end (PR 98512)

[David Malcolm <dmalcolm@redhat.com>]

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?

>  			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"?

[...]

> diff --git a/gcc/testsuite/gcc.dg/Wfree-nonheap-object-4.c b/gcc/testsuite/gcc.dg/Wfree-nonheap-object-4.c
> index a7d921248c4..fdef9e6b3ea 100644
> --- a/gcc/testsuite/gcc.dg/Wfree-nonheap-object-4.c
> +++ b/gcc/testsuite/gcc.dg/Wfree-nonheap-object-4.c
> @@ -26,7 +26,7 @@ void g2 (struct A *p) { g1 (p); }
>  
>  #define NOIPA __attribute__ ((noipa))
>  
> -extern int array[];
> +extern int array[];           // { dg-message "declared here" "note on line 29" }
>  
>  /* Verify the warning is issued even for calls in a system header inlined
>     into a function outside the header.  */
> @@ -39,7 +39,7 @@ NOIPA void warn_g0 (struct A *p)
>    g0 (p);
>  }
>  
> -// { dg-message "inlined from 'warn_g0'" "" { target *-*-* } 0 }
> +// { dg-message "inlined from 'warn_g0'" "note on line 42" { target *-*-* } 0 }
>  
>  
>  /* Also verify the warning can be suppressed.  */
> @@ -65,8 +65,8 @@ NOIPA void warn_g1 (struct A *p)
>    g1 (p);
>  }
>  
> -// { dg-message "inlined from 'g1'" "" { target *-*-* } 0 }
> -// { dg-message "inlined from 'warn_g1'" "" { target *-*-* } 0 }
> +// { dg-message "inlined from 'g1'" "note on line 68" { target *-*-* } 0 }
> +// { dg-message "inlined from 'warn_g1'" "note on line 69" { target *-*-* } 0 }
>  
>  
>  NOIPA void nowarn_g1 (struct A *p)
> @@ -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?


> 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.

> +
> +
> +static void ignore0 (int *p)
> +{
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +  memset (p, __LINE__, 3);
> +#pragma GCC diagnostic pop
> +}
> +
> +static void nowarn1_ignore0 (int *p)
> +{
> +  ignore0 (p + 1);
> +}
> +
> +static void nowarn2_ignore0 (int *p)
> +{
> +  nowarn1_ignore0 (p + 1);
> +}
> +
> +int b2[2];
> +
> +void nowarn3_ignore0 (void)
> +{
> +  nowarn2_ignore0 (b2 + 1);
> +}
> +
> +
> +static void nowarn0_ignore1 (int *p)
> +{
> +  memset (p, __LINE__, 3);
> +}
> +
> +static void ignore1 (int *p)
> +{
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +  nowarn0_ignore1 (p + 1);
> +#pragma GCC diagnostic pop
> +}
> +
> +void nowarn2_ignore1 (int *p)
> +{
> +  ignore1 (p + 1);
> +}
> +
> +int c2[2];
> +
> +void nowarn3_ignore1 (void)
> +{
> +  nowarn2_ignore1 (c2 + 1);
> +}
> +
> +
> +static void nowarn0_ignore2 (int *p)
> +{
> +  memset (p, __LINE__, 3);
> +}
> +
> +static void nowarn1_ignore2 (int *p)
> +{
> +  nowarn0_ignore2 (p + 1);
> +}
> +
> +static void ignore2 (int *p)
> +{
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +  nowarn1_ignore2 (p + 1);
> +#pragma GCC diagnostic pop
> +}
> +
> +int d2[2];
> +
> +void nowarn3_ignore2 (void)
> +{
> +  ignore2 (c2 + 1);
> +}
> +
> +
> +
> +static void nowarn0_ignore3 (int *p)
> +{
> +  memset (p, __LINE__, 3);
> +}
> +
> +static void nowarn1_ignore3 (int *p)
> +{
> +  nowarn0_ignore3 (p + 1);
> +}
> +
> +static void nowarn2_ignore3 (int *p)
> +{
> +  nowarn1_ignore3 (p + 1);
> +}
> +
> +int e2[2];
> +
> +void ignore3 (void)
> +{
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +  nowarn2_ignore3 (e2 + 1);
> +#pragma GCC diagnostic pop
> +}

[...]

Dave