| Message ID | 4117138.BLyiyAZBGq@polaris |
|---|---|
| State | New |
| Headers | show |
| Series | Fix ICE in verify_sra_access_forest | expand |
Hi, On Thu, Jun 11 2020, Eric Botcazou wrote: > Hi, > > this fixes an issue with reverse storage order in SRA, which is caught by the > built-in verifier: > > ===========================GNAT BUG DETECTED==============================+ > | 11.0.0 20200610 (experimental) (x86_64-suse-linux) GCC error: | > | in verify_sra_access_forest, at tree-sra.c:2359 > > gcc_assert (reverse == access->reverse); > > The problem is that propagate_subaccesses_from_rhs changes the type of an > access from aggregate to scalar and, as discussed elsewhere, this must be done > with extra care in the presence of reverse storage order. > > Tested on x86-64/Linux, OK for the mainline? > > > 2020-06-11 Eric Botcazou <ebotcazou@adacore.com> > > * tree-sra.c (propagate_subaccesses_from_rhs): When a non-scalar > access on the LHS is replaced with a scalar access, propagate the > TYPE_REVERSE_STORAGE_ORDER flag of the type of the original access. > > > 2020-06-11 Eric Botcazou <ebotcazou@adacore.com> > > * gnat.dg/opt85.ad[sb]: New test. > > -- > Eric Botcazou > diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c > index 4793b48f32c..fcba7fbdd31 100644 > --- a/gcc/tree-sra.c > +++ b/gcc/tree-sra.c > @@ -2758,6 +2758,9 @@ propagate_subaccesses_from_rhs (struct access *lacc, struct access *racc) > } > if (!lacc->first_child && !racc->first_child) > { > + /* We are about to change the access type from aggregate to scalar, > + so we need to put the reverse flag onto the access, if any. */ > + const bool reverse = TYPE_REVERSE_STORAGE_ORDER (lacc->type); I am not very good at reasoning about reverse storage order stuff but can it ever happen that this reverse is not the same as racc->reverse? In order for that to happen, we'd have to have an assignment (folded memcpy?) between two aggregates in the original code that, at the same offset within the aggregates, have single field structures with different storage orders. If that has undefined behavior (even for folded memcpy), I guess I am fine with the patch (but I cannot approve it). If not, I'd add a condition that racc->reverse is the same as TYPE_REVERSE_STORAGE_ORDER(lacc->type) to the if guarding all of this. I tried to come up with a (C) testcase for this but just could not trigger even this condition reasonably quickly. Thanks for looking into this, Martin
> I am not very good at reasoning about reverse storage order stuff but > can it ever happen that this reverse is not the same as racc->reverse? > > In order for that to happen, we'd have to have an assignment (folded > memcpy?) between two aggregates in the original code that, at the same > offset within the aggregates, have single field structures with > different storage orders. If that has undefined behavior (even for > folded memcpy), I guess I am fine with the patch (but I cannot approve > it). If not, I'd add a condition that racc->reverse is the same as > TYPE_REVERSE_STORAGE_ORDER(lacc->type) to the if guarding all of this. You cannot do an assignment between aggregates with opposite storage order, that's rejected by the compiler (both in C and Ada); generally speaking, such types are not compatible, even if they have the same underlying structure. We also block SRA when there is a VIEW_CONVERT_EXPR messing with the storage order, see calls to storage_order_barrier_p. And folding a memcpy between them should be invalid too (see e.g. the ongoing discussion with Richard in an earlier thread). So my understanding is that you don't need the condition.
On Thu, Jun 11, 2020 at 5:24 PM Eric Botcazou <botcazou@adacore.com> wrote: > > > I am not very good at reasoning about reverse storage order stuff but > > can it ever happen that this reverse is not the same as racc->reverse? > > > > In order for that to happen, we'd have to have an assignment (folded > > memcpy?) between two aggregates in the original code that, at the same > > offset within the aggregates, have single field structures with > > different storage orders. If that has undefined behavior (even for > > folded memcpy), I guess I am fine with the patch (but I cannot approve > > it). If not, I'd add a condition that racc->reverse is the same as > > TYPE_REVERSE_STORAGE_ORDER(lacc->type) to the if guarding all of this. > > You cannot do an assignment between aggregates with opposite storage order, > that's rejected by the compiler (both in C and Ada); generally speaking, such > types are not compatible, even if they have the same underlying structure. > We also block SRA when there is a VIEW_CONVERT_EXPR messing with the storage > order, see calls to storage_order_barrier_p. And folding a memcpy between > them should be invalid too (see e.g. the ongoing discussion with Richard in an > earlier thread). So my understanding is that you don't need the condition. The patch probably fixes only part of the issues with SRA and rev-storage. I see in build_ref_for_model: /* The flag will be set on the record type. */ REF_REVERSE_STORAGE_ORDER (t) = 0; for the case there was a COMPONENT_REF, so this is for the case where the model did not have that? Based on your assertion above can we derive TYPE_REVERSE_STORAGE_ORDER from the passed in racc and thus set the flag correctly in build_ref_for_model instead? Thanks, Richard. > -- > Eric Botcazou
> The patch probably fixes only part of the issues with SRA and rev-storage. Well, this issue (coming from a bypass in SRA) is the first issue uncovered with rev-storage in SRA for years. > I see in build_ref_for_model: > > /* The flag will be set on the record type. */ > REF_REVERSE_STORAGE_ORDER (t) = 0; > > for the case there was a COMPONENT_REF, so this is for the case where > the model did not have that? No, it's the other way around: model->reverse is true so the flag will be set on the MEM out of build_ref_for_offset. But the type of the MEM is aggregate, which means that the flag may not be set on the MEM; instead it should be set on the type itself, as indicated in the comment. > Based on your assertion above can we derive TYPE_REVERSE_STORAGE_ORDER from > the passed in racc and thus set the flag correctly in build_ref_for_model > instead? Well, the code is a bypass that builds an access on the LHS from a model of the RHS(!), so things need to be patched up and I'd rather not introduce bugs elsewhere if possible...
On Mon, Jun 15, 2020 at 10:23 AM Eric Botcazou <botcazou@adacore.com> wrote: > > > The patch probably fixes only part of the issues with SRA and rev-storage. > > Well, this issue (coming from a bypass in SRA) is the first issue uncovered > with rev-storage in SRA for years. > > > I see in build_ref_for_model: > > > > /* The flag will be set on the record type. */ > > REF_REVERSE_STORAGE_ORDER (t) = 0; > > > > for the case there was a COMPONENT_REF, so this is for the case where > > the model did not have that? > > No, it's the other way around: model->reverse is true so the flag will be set > on the MEM out of build_ref_for_offset. But the type of the MEM is aggregate, > which means that the flag may not be set on the MEM; instead it should be set > on the type itself, as indicated in the comment. Ah OK. > > Based on your assertion above can we derive TYPE_REVERSE_STORAGE_ORDER from > > the passed in racc and thus set the flag correctly in build_ref_for_model > > instead? > > Well, the code is a bypass that builds an access on the LHS from a model of > the RHS(!), so things need to be patched up and I'd rather not introduce bugs > elsewhere if possible... Hmm, true. So the patch is OK then. Thanks, Richard. > -- > Eric Botcazou
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 4793b48f32c..fcba7fbdd31 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -2758,6 +2758,9 @@ propagate_subaccesses_from_rhs (struct access *lacc, struct access *racc) } if (!lacc->first_child && !racc->first_child) { + /* We are about to change the access type from aggregate to scalar, + so we need to put the reverse flag onto the access, if any. */ + const bool reverse = TYPE_REVERSE_STORAGE_ORDER (lacc->type); tree t = lacc->base; lacc->type = racc->type; @@ -2772,9 +2775,12 @@ propagate_subaccesses_from_rhs (struct access *lacc, struct access *racc) lacc->expr = build_ref_for_model (EXPR_LOCATION (lacc->base), lacc->base, lacc->offset, racc, NULL, false); + if (TREE_CODE (lacc->expr) == MEM_REF) + REF_REVERSE_STORAGE_ORDER (lacc->expr) = reverse; lacc->grp_no_warning = true; lacc->grp_same_access_path = false; } + lacc->reverse = reverse; } return ret; }
Hi, this fixes an issue with reverse storage order in SRA, which is caught by the built-in verifier: ===========================GNAT BUG DETECTED==============================+ | 11.0.0 20200610 (experimental) (x86_64-suse-linux) GCC error: | | in verify_sra_access_forest, at tree-sra.c:2359 gcc_assert (reverse == access->reverse); The problem is that propagate_subaccesses_from_rhs changes the type of an access from aggregate to scalar and, as discussed elsewhere, this must be done with extra care in the presence of reverse storage order. Tested on x86-64/Linux, OK for the mainline? 2020-06-11 Eric Botcazou <ebotcazou@adacore.com> * tree-sra.c (propagate_subaccesses_from_rhs): When a non-scalar access on the LHS is replaced with a scalar access, propagate the TYPE_REVERSE_STORAGE_ORDER flag of the type of the original access. 2020-06-11 Eric Botcazou <ebotcazou@adacore.com> * gnat.dg/opt85.ad[sb]: New test.