[PATCH] waccess: Fix two -Wnonnull warning issues [PR108986]

[PATCH] waccess: Fix two -Wnonnull warning issues [PR108986]

[Jakub Jelinek]

Hi!

The following patch fixes 2 issues with the -Wnonnull warning.

One, fixed by the second hunk, is that the warning wording is bogus
since r11-3305, instead of printing
warning: argument 1 to ‘int[static 7]’ is null where non-null expected [-Wnonnull]
it prints
warning: argument 1 to ‘int[static 28]’ is null where non-null expected [-Wnonnull]
access_size is measured in bytes, so obviously will be correct as array
number of elements only if it is 1 byte element array.
In the function, access_nelts is either constant (if sizidx == -1) or
when the array size is determined by some other parameter, I believe a value
passed to that argument.
Later on we query the range of it:
      if (get_size_range (m_ptr_qry.rvals, access_nelts, stmt, sizrng, 1))
which I bet must just return accesS_nelts in sizrng[0] and [1] if it is
constant.  access_size is later computed as:
      tree access_size = NULL_TREE;
      if (tree_int_cst_sgn (sizrng[0]) >= 0)
        {
          if (COMPLETE_TYPE_P (argtype))
            {
...
                    wide_int minsize = wi::to_wide (sizrng[0], prec);
                    minsize *= wi::to_wide (argsize, prec);
                    access_size = wide_int_to_tree (sizetype, minsize);
                  }
            }
          else
            access_size = access_nelts;
        }
and immediately after this the code does:
      if (integer_zerop (ptr))
        {
          if (sizidx >= 0 && tree_int_cst_sgn (sizrng[0]) > 0)
            {
some other warning wording
            }
          else if (access_size && access.second.static_p)
            {
this spot
            }
        }
So, because argtype is complete, access_size has been multiplied by
argsize, but in case of this exact warning ("this spot" above)
I believe access_nelts must be really constant, otherwise
"some other warning wording" would handle it.  So, I think access_nelts
is exactly what we want to print there.

The other problem is that since the introduction of -Wdangling-pointer
in r12-6606, the pass has early and late instances and while lots of
stuff in the pass is guarded on being done in the late pass only,
this particular function is not, furthermore it is emitting two different
warnings in a loop and already messes up with stuff like clearing
warning suppression for one of the warning (ugh!).  The end effect is
that we warn twice about the same problem, once in the early and once in
the late pass.  Now, e.g. with -O2 -Wall we warn just once, during the
early pass, as it is then optimized away, so I think just making this
late warning only wouldn't be best.  This patch instead returns early
if either of the warnings is suppressed on the call stmt already.
I think if one of the passes warned on it already (even if say on some other
argument), then warning again (even on some other argument) is unnecessary,
if both problems are visible in the same pass we'll still warn about both.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-03-03  Jakub Jelinek  <jakub@redhat.com>

	PR c/108986
	* gimple-ssa-warn-access.cc (pass_waccess::maybe_check_access_sizes):
	Return immediately if OPT_Wnonnull or OPT_Wstringop_overflow_ is
	suppressed on stmt.  For [static %E] warning, print access_nelts
	rather than access_size.  Fix up comment wording.

	* gcc.dg/Wnonnull-8.c: New test.

--- gcc/gimple-ssa-warn-access.cc.jj	2023-02-22 20:50:27.418519815 +0100
+++ gcc/gimple-ssa-warn-access.cc	2023-03-02 19:04:24.744280016 +0100
@@ -3318,6 +3318,10 @@ void
 pass_waccess::maybe_check_access_sizes (rdwr_map *rwm, tree fndecl, tree fntype,
 					gimple *stmt)
 {
+  if (warning_suppressed_p (stmt, OPT_Wnonnull)
+      || warning_suppressed_p (stmt, OPT_Wstringop_overflow_))
+    return;
+
   auto_diagnostic_group adg;
 
   /* Set if a warning has been issued for any argument (used to decide
@@ -3501,7 +3505,7 @@ pass_waccess::maybe_check_access_sizes (
 	      if (warning_at (loc, OPT_Wnonnull,
 			      "argument %i to %<%T[static %E]%> "
 			      "is null where non-null expected",
-			      ptridx + 1, argtype, access_size))
+			      ptridx + 1, argtype, access_nelts))
 		arg_warned = OPT_Wnonnull;
 	    }
 
@@ -3593,7 +3597,7 @@ pass_waccess::maybe_check_access_sizes (
 		"in a call with type %qT", fntype);
     }
 
-  /* Set the bit in case if was cleared and not set above.  */
+  /* Set the bit in case it was cleared and not set above.  */
   if (opt_warned != no_warning)
     suppress_warning (stmt, opt_warned);
 }
--- gcc/testsuite/gcc.dg/Wnonnull-8.c.jj	2023-03-02 17:28:24.323898410 +0100
+++ gcc/testsuite/gcc.dg/Wnonnull-8.c	2023-03-02 17:27:51.804376322 +0100
@@ -0,0 +1,14 @@
+/* PR c/108986 */
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+void
+foo (int a[static 7])
+{
+}
+
+int
+main ()
+{
+  foo ((int *) 0);	/* { dg-warning "argument 1 to 'int\\\[static 7\\\]' is null where non-null expected" } */
+}

	Jakub

Re: [PATCH] waccess: Fix two -Wnonnull warning issues [PR108986]

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 5466 bytes --]

On Fri, 3 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> The following patch fixes 2 issues with the -Wnonnull warning.
> 
> One, fixed by the second hunk, is that the warning wording is bogus
> since r11-3305, instead of printing
> warning: argument 1 to ‘int[static 7]’ is null where non-null expected [-Wnonnull]
> it prints
> warning: argument 1 to ‘int[static 28]’ is null where non-null expected [-Wnonnull]
> access_size is measured in bytes, so obviously will be correct as array
> number of elements only if it is 1 byte element array.
> In the function, access_nelts is either constant (if sizidx == -1) or
> when the array size is determined by some other parameter, I believe a value
> passed to that argument.
> Later on we query the range of it:
>       if (get_size_range (m_ptr_qry.rvals, access_nelts, stmt, sizrng, 1))
> which I bet must just return accesS_nelts in sizrng[0] and [1] if it is
> constant.  access_size is later computed as:
>       tree access_size = NULL_TREE;
>       if (tree_int_cst_sgn (sizrng[0]) >= 0)
>         {
>           if (COMPLETE_TYPE_P (argtype))
>             {
> ...
>                     wide_int minsize = wi::to_wide (sizrng[0], prec);
>                     minsize *= wi::to_wide (argsize, prec);
>                     access_size = wide_int_to_tree (sizetype, minsize);
>                   }
>             }
>           else
>             access_size = access_nelts;
>         }
> and immediately after this the code does:
>       if (integer_zerop (ptr))
>         {
>           if (sizidx >= 0 && tree_int_cst_sgn (sizrng[0]) > 0)
>             {
> some other warning wording
>             }
>           else if (access_size && access.second.static_p)
>             {
> this spot
>             }
>         }
> So, because argtype is complete, access_size has been multiplied by
> argsize, but in case of this exact warning ("this spot" above)
> I believe access_nelts must be really constant, otherwise
> "some other warning wording" would handle it.  So, I think access_nelts
> is exactly what we want to print there.
> 
> The other problem is that since the introduction of -Wdangling-pointer
> in r12-6606, the pass has early and late instances and while lots of
> stuff in the pass is guarded on being done in the late pass only,
> this particular function is not, furthermore it is emitting two different
> warnings in a loop and already messes up with stuff like clearing
> warning suppression for one of the warning (ugh!).  The end effect is
> that we warn twice about the same problem, once in the early and once in
> the late pass.  Now, e.g. with -O2 -Wall we warn just once, during the
> early pass, as it is then optimized away, so I think just making this
> late warning only wouldn't be best.  This patch instead returns early
> if either of the warnings is suppressed on the call stmt already.
> I think if one of the passes warned on it already (even if say on some other
> argument), then warning again (even on some other argument) is unnecessary,
> if both problems are visible in the same pass we'll still warn about both.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2023-03-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/108986
> 	* gimple-ssa-warn-access.cc (pass_waccess::maybe_check_access_sizes):
> 	Return immediately if OPT_Wnonnull or OPT_Wstringop_overflow_ is
> 	suppressed on stmt.  For [static %E] warning, print access_nelts
> 	rather than access_size.  Fix up comment wording.
> 
> 	* gcc.dg/Wnonnull-8.c: New test.
> 
> --- gcc/gimple-ssa-warn-access.cc.jj	2023-02-22 20:50:27.418519815 +0100
> +++ gcc/gimple-ssa-warn-access.cc	2023-03-02 19:04:24.744280016 +0100
> @@ -3318,6 +3318,10 @@ void
>  pass_waccess::maybe_check_access_sizes (rdwr_map *rwm, tree fndecl, tree fntype,
>  					gimple *stmt)
>  {
> +  if (warning_suppressed_p (stmt, OPT_Wnonnull)
> +      || warning_suppressed_p (stmt, OPT_Wstringop_overflow_))
> +    return;
> +
>    auto_diagnostic_group adg;
>  
>    /* Set if a warning has been issued for any argument (used to decide
> @@ -3501,7 +3505,7 @@ pass_waccess::maybe_check_access_sizes (
>  	      if (warning_at (loc, OPT_Wnonnull,
>  			      "argument %i to %<%T[static %E]%> "
>  			      "is null where non-null expected",
> -			      ptridx + 1, argtype, access_size))
> +			      ptridx + 1, argtype, access_nelts))
>  		arg_warned = OPT_Wnonnull;
>  	    }
>  
> @@ -3593,7 +3597,7 @@ pass_waccess::maybe_check_access_sizes (
>  		"in a call with type %qT", fntype);
>      }
>  
> -  /* Set the bit in case if was cleared and not set above.  */
> +  /* Set the bit in case it was cleared and not set above.  */
>    if (opt_warned != no_warning)
>      suppress_warning (stmt, opt_warned);
>  }
> --- gcc/testsuite/gcc.dg/Wnonnull-8.c.jj	2023-03-02 17:28:24.323898410 +0100
> +++ gcc/testsuite/gcc.dg/Wnonnull-8.c	2023-03-02 17:27:51.804376322 +0100
> @@ -0,0 +1,14 @@
> +/* PR c/108986 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wall" } */
> +
> +void
> +foo (int a[static 7])
> +{
> +}
> +
> +int
> +main ()
> +{
> +  foo ((int *) 0);	/* { dg-warning "argument 1 to 'int\\\[static 7\\\]' is null where non-null expected" } */
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)