* [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457)
@ 2022-08-10 21:39 Qing Zhao
2022-08-11 7:40 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Qing Zhao @ 2022-08-10 21:39 UTC (permalink / raw)
To: richard Biener; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]
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
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
...
}
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
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:
[-- Attachment #2: 0001-middle-end-106457-improve-array_at_struct_end_p-for-.patch --]
[-- Type: application/octet-stream, Size: 5665 bytes --]
From b09e4b56a4cfc11d1ea15226359643175f8466b0 Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Tue, 9 Aug 2022 15:03:25 +0000
Subject: [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
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
...
}
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
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.
---
gcc/testsuite/gcc.dg/torture/pr50067-1.c | 1 +
gcc/testsuite/gcc.dg/torture/pr50067-2.c | 1 +
gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c | 2 +-
gcc/testsuite/gcc.target/i386/pr104059.c | 2 +-
gcc/tree.cc | 8 ++++----
5 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/torture/pr50067-1.c b/gcc/testsuite/gcc.dg/torture/pr50067-1.c
index 8201ebfdc91b..8b6c84d0a3c1 100644
--- a/gcc/testsuite/gcc.dg/torture/pr50067-1.c
+++ b/gcc/testsuite/gcc.dg/torture/pr50067-1.c
@@ -1,4 +1,5 @@
/* { dg-do run } */
+/* { dg-options "-Wno-aggressive-loop-optimizations" } */
/* Make sure data-dependence analysis does not compute a bogus
distance vector for the different sized accesses. */
diff --git a/gcc/testsuite/gcc.dg/torture/pr50067-2.c b/gcc/testsuite/gcc.dg/torture/pr50067-2.c
index f9728a766786..d130c23ef435 100644
--- a/gcc/testsuite/gcc.dg/torture/pr50067-2.c
+++ b/gcc/testsuite/gcc.dg/torture/pr50067-2.c
@@ -1,4 +1,5 @@
/* { dg-do run } */
+/* { dg-options "-Wno-aggressive-loop-optimizations" } */
/* Make sure data-dependence analysis does not compute a bogus
distance vector for the different sized accesses. */
diff --git a/gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c b/gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c
index 0ad96954ff7d..5f9e08d53ffa 100644
--- a/gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c
+++ b/gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-additional-options "-O3 -std=c99" } */
+/* { dg-additional-options "-O3 -std=c99 -Wno-aggressive-loop-optimizations" } */
/* { dg-final { check-function-bodies "**" "" "" } } */
#include <stdint.h>
diff --git a/gcc/testsuite/gcc.target/i386/pr104059.c b/gcc/testsuite/gcc.target/i386/pr104059.c
index 4815fa38d217..2ae87eac10b3 100644
--- a/gcc/testsuite/gcc.target/i386/pr104059.c
+++ b/gcc/testsuite/gcc.target/i386/pr104059.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-mavx2 -O2 -fdump-rtl-cprop_hardreg-details" } */
+/* { dg-options "-mavx2 -O2 -fdump-rtl-cprop_hardreg-details -Wno-aggressive-loop-optimizations" } */
/* { dg-final { scan-rtl-dump-not {replaced reg [0-9]* with [0-9]*} "cprop_hardreg" } } */
#include<stdint.h>
diff --git a/gcc/tree.cc b/gcc/tree.cc
index fed1434d141d..f34d7efb3de1 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -12692,6 +12692,10 @@ array_at_struct_end_p (tree ref)
{
atype = TREE_TYPE (TREE_OPERAND (ref, 0));
ref = TREE_OPERAND (ref, 0);
+ if (TREE_CODE (ref) == COMPONENT_REF || TREE_CODE (ref) == MEM_REF)
+ return array_at_struct_end_p (ref);
+ else
+ return false;
}
else if (TREE_CODE (ref) == COMPONENT_REF
&& TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 1))) == ARRAY_TYPE)
@@ -12778,10 +12782,6 @@ array_at_struct_end_p (tree ref)
&& DECL_SIZE_UNIT (ref)
&& TREE_CODE (DECL_SIZE_UNIT (ref)) == INTEGER_CST)
{
- /* If the object itself is the array it is not at struct end. */
- if (DECL_P (ref_to_array))
- return false;
-
/* Check whether the array domain covers all of the available
padding. */
poly_int64 offset;
--
2.27.0
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457)
2022-08-10 21:39 [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457) Qing Zhao
@ 2022-08-11 7:40 ` Richard Biener
2022-08-11 13:26 ` Qing Zhao
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2022-08-11 7:40 UTC (permalink / raw)
To: Qing Zhao; +Cc: gcc-patches
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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457)
2022-08-11 7:40 ` Richard Biener
@ 2022-08-11 13:26 ` Qing Zhao
2022-08-12 6:46 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Qing Zhao @ 2022-08-11 13:26 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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.
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?
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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457)
2022-08-11 13:26 ` Qing Zhao
@ 2022-08-12 6:46 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2022-08-12 6:46 UTC (permalink / raw)
To: Qing Zhao; +Cc: gcc-patches
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-12 6:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-10 21:39 [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457) Qing Zhao
2022-08-11 7:40 ` Richard Biener
2022-08-11 13:26 ` Qing Zhao
2022-08-12 6:46 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).