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 <ja...@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 <rguent...@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)
