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

[Richard Biener <rguenther@suse.de>]

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

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

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

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

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)