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

[Richard Biener <rguenther@suse.de>]

[-- 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)