Re: [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457)

[Richard Biener <rguenther@suse.de>]

On Thu, 11 Aug 2022, Qing Zhao wrote:

> 
> 
> > On Aug 11, 2022, at 3:40 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Wed, 10 Aug 2022, Qing Zhao wrote:
> > 
> >> Hi,
> >> 
> >> As mentioned in the bug report, I reopened this bug since the previous patch:
> >> 
> >> commit r13-1875-gff26f0ba68fe6e870f315d0601b596f889b89680
> >> Author: Richard Biener <rguenther@suse.de>
> >> Date:   Thu Jul 28 10:07:32 2022 +0200
> >> 
> >>    middle-end/106457 - improve array_at_struct_end_p for array objects
> >>    Array references to array objects are never at struct end.
> >> 
> >> 
> >> Didn’t resolve this bug.
> >> 
> >> This is a new patch, and my current work on -fstrict-flex-array depends on this patch.
> >> 
> >> Please take a look at the patch and let me know whether it’s good for committing.
> >> 
> >> Thanks.
> >> 
> >> Qing.
> >> 
> >> 
> >> ======================================
> >> 
> >> [PATCH] middle-end/106457 - improve array_at_struct_end_p for array
> >> objects (PR106457)
> >> 
> >> Array references are not handled correctly by current array_at_struct_end_p,
> >> for the following array references:
> >> 
> >> Example 1: (from gcc/testsuite/gcc.dg/torture/pr50067-[1|2].c):
> >> 
> >> short a[32] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
> >>                 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 };
> >> ... = (*((char(*)[32])&a[0]))[i+8];  // this array reference
> > 
> > For this one we have
> > 
> > (gdb) p debug_generic_expr (ref_to_array)
> > MEM[(char[32] *)&a]
> > 
> > at the
> > 
> > 12782         if (DECL_P (ref_to_array))
> > 
> > check.  The array referenced (char[32]) isn't the decl (which is 
> > short[32]), so the referenced array (char[32]) _is_ at struct end
> > since accessing index 48 is OK - it won't exceed the decl boundary.
> 
> > In fact that -Waggressive-loop-optimizations triggers with your
> > patch shows it is wrong in this case - the code is perfectly
> > OK, no out-of-bound access occurs.  When this warning triggers it
> > hints at the code being miscompiled (we usually elide the exit
> > test).
> 
> Okay, I see. 
> > 
> >> Example 2: (from gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c):
> >> 
> >> int test (uint8_t *p, uint32_t t[1][1], int n) {
> >>  for (int i = 0; i < 4; i++, p++)
> >>    t[i][0] = ...;  // this array reference
> >> ...
> >> }
> > 
> > The parameter declaration uint32_t t[1][1] doesn't guarantee
> > anything - it's just a pointer.  So we cannot reasonably
> > constrain accesses to sizeof(uint32_t).  That means
> > array_at_struct_end_p has to return true.
> 
> Okay.  Then again, the name of the routine “array_at_struct_end_p” is really confusing. -:)
> > 
> >> Example 3: (from gcc/testsuite/g++.dg/debug/debug5.C):
> >> 
> >>  int a = 1;
> >>  int b = 1;
> >>  int e[a][b];
> >>  e[0][0] = 0;  // this array reference
> > 
> > That might be the only case we'd be allowed to say false.  I see
> > a single call with
> > 
> > MEM[(int[0:D.1989][0:D.1986] *)&e.4][0]{lb: 0 sz: 4}
> > 
> > that is, the caller has stripped the outermost [0] already.
> > The issue here is unfortunate lowering of the alloca
> > with constant size we originally have here.  The array typed
> > decl has type int[1] but we access it with type
> > int[0:D.1989][0:D.1986].  We'd now have to prove that
> > D.1989 and D.1986 are '1' (and not less).  That doesn't look
> > possible in general.  Not sure if it is worth the trobble here.
> 
> Okay. 
> > 
> > Eventually array_at_struct_end_p isn't the correct vehicle for
> > diagnostics if you desparately need to avoid 'true' for the above
> > cases.  Note for the first case it's more about treating
> > pointer to array with bounds in a stricter way than we do
> > at the moment, not so much about arrays at struct end.  But
> > array_at_struct_end_p is used to query whether we have to assume
> > there's storage beyond the declared bound of an array that is
> > "valid" to access (and it's valid from the middle-end point of
> > view in this case).
> 
> Then, this is really NOT “array_at_struct_end_p”, it’s “array_is_flexible_p”, which mixes the concept of “array_at_struct_end” and array is flexible (or extendable).
> I am Okay with the name right now in this patch, I will update the comments of this routine in the current patch. And update the name of the routine in a later cleanup patch.

Yes, the routine checks whether the access is possibly to a flex array.

> 
> Since the new FIELD DECL_NOT_FLEXARRAY for the patch -fstrict-flex-arrays:
> 
> +/* Used in a FIELD_DECL to indicate whether this field is not a flexible
> +   array member.  */
> +#define DECL_NOT_FLEXARRAY(NODE) \
> +  (FIELD_DECL_CHECK (NODE)->decl_common.decl_not_flexarray)
> +
> 
> Is ONLY for field in a structure, so it will NOT control the result for such flexible array access.  i.e, for all the flexible array access similar as in the Example 1, 2, or 3, 
> They are always flexible,  -fstrict-flex-arrays should NOT control them, right?

Yes.  I don't think we ever can make general "arrays" stricter in C,
they are really just pointers.  Iff there are languages where array
types and accesses via pointer to array types can be more strictly
constrained a bit like DECL_NOT_FLEXARRAY would have to reside on the
array type or in reverse, the C and C++ frontends would need to zero
the upper bound of the domain carefully in some contexts.

Thanks,
Richard.

> Thanks
> 
> Qing
> 
> > 
> > Richard.
> > 
> >> All the above array references are identified as TRUE by the current
> >> array_at_struct_end_p, therefore treated as flexible array members.
> >> Obviously, they are just simple array references, not an array refs
> >> to the last field of a struture. The current array_at_struct_end_p handles
> >> such array references incorrectly.
> >> 
> >> In order to handle array references correctly, we could recursively check
> >> its first operand if it's a MEM_REF or COMPONENT_REF and stop as FALSE
> >> when otherwise. This resolved all the issues for ARRAY_REF.
> >> 
> >> bootstrapped and regression tested on both X86 and Aarch64.
> >> Multiple testing cases behave differently due to array_at_struct_end_p now
> >> behave correctly (return FALSE now, then they are not flexible array member
> >> anymore). Adjust these testing cases.
> >> 
> >> There is one regression for gcc/target/aarch64/vadd_reduc-2.c is left
> >> unresolved since the loop transformation is changed due to the changed behavior
> >> of array_at_struct_end_p, simple adjustment of the testing case doesnt work.
> >> I will file a bug to record this regression.
> >> 
> >> gcc/ChangeLog:
> >> 
> >>        PR middle-end/106457
> >>        * tree.cc (array_at_struct_end_p): Handle array objects recursively
> >>        through its first operand.
> >> 
> >> gcc/testsuite/ChangeLog:
> >> 
> >>        PR middle-end/106457
> >>        * gcc.dg/torture/pr50067-1.c: Add -Wno-aggressive-loop-optimizations
> >>        to suppress warnings.
> >>        * gcc.dg/torture/pr50067-2.c: Likewise.
> >>        * gcc.target/aarch64/vadd_reduc-2.c: Likewise.
> >>        * gcc.target/i386/pr104059.c: Likewise.
> >> 
> >> 
> >> The complete patch is at:
> >> 
> >> 
> >> 
> > 
> > -- 
> > 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)
> 
> 

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