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