Hi,
On Mon, Jul 04 2022, Richard Biener wrote:
> On Fri, 1 Jul 2022, Martin Jambor wrote:
>
>> Hi,
>>
>> As the testcase in PR 105860 shows, the code that tries to re-use the
>> handled_component chains in SRA can be horribly confused by unions,
>> where it thinks it has found a compatible structure under which it can
>> chain the references, but in fact it found the type it was looking
>> for elsewhere in a union and generated a write to a completely wrong
>> part of an aggregate.
>>
>> I don't remember whether the plan was to support unions at all in
>> build_reconstructed_reference but it can work, to an extent, if we
>> make sure that we start the search only outside the outermost union,
>> which is what the patch does (and the extra testcase verifies).
>>
>> Bootstrapped and tested on x86_64-linux. OK for trunk and then for
>> release branches?
>
> OK, but I'm wondering if the same problem can arise when the
> handled_component_p includes VIEW_CONVERTs or BIT_FIELD_REFs
> both of which may pun to a type already seen in a more inner
> referece.
SRA already refuses to operate at all on any anything that is accessed
with a reference where a V_C_E is not the outermost handled_component.
Outermost V_C_Es are skipped and the pass works with the underlying
expr. BIT_FIELD_REFs have to be outermost and they are treated
similarly. So that should be safe.
> Thus, is the actual problem that build_reconstructed_reference
> searches for the outermost match of the type rather than the
> innermost match? So should it search inner-to-outer instead
> (or for the last match in the current way of searching?)
I don't think so. In the testcase it found a match where there should
have been none (meaning a crude MEM_REF should be created), any
certainly correct match must be outside of a union COMPONENT_REF and
there should never be more than one.
Thanks,
Martin
>
> Thanks,
> Richard.
>
>> Thanks,
>>
>> Martin
>>
>>
>> gcc/ChangeLog:
>>
>> 2022-07-01 Martin Jambor <mjam...@suse.cz>
>>
>> PR tree-optimization/105860
>> * tree-sra.cc (build_reconstructed_reference): Start expr
>> traversal only just below the outermost union.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2022-07-01 Martin Jambor <mjam...@suse.cz>
>>
>> PR tree-optimization/105860
>> * gcc.dg/tree-ssa/alias-access-path-13.c: New test.
>> * gcc.dg/tree-ssa/pr105860.c: Likewise.
>> ---
>> .../gcc.dg/tree-ssa/alias-access-path-13.c | 31 +++++++++
>> gcc/testsuite/gcc.dg/tree-ssa/pr105860.c | 63 +++++++++++++++++++
>> gcc/tree-sra.cc | 13 +++-
>> 3 files changed, 106 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
>> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
>>
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
>> b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
>> new file mode 100644
>> index 00000000000..e502a97bc75
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
>> @@ -0,0 +1,31 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-fre1" } */
>> +
>> +struct inn
>> +{
>> + int val;
>> +};
>> +
>> +union foo
>> +{
>> + struct inn inn;
>> + long int baz;
>> +} *fooptr;
>> +
>> +struct bar
>> +{
>> + union foo foo;
>> + int val2;
>> +} *barptr;
>> +
>> +int
>> +test ()
>> +{
>> + union foo foo;
>> + foo.inn.val = 0;
>> + barptr->val2 = 123;
>> + *fooptr = foo;
>> + return barptr->val2;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-times "return 123" 1 "fre1"} } */
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
>> b/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
>> new file mode 100644
>> index 00000000000..77bcb4a6739
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
>> @@ -0,0 +1,63 @@
>> +/* { dg-do run } */
>> +/* { dg-options "-O1" } */
>> +
>> +struct S1 {
>> + unsigned int _0;
>> + unsigned int _1;
>> +} ;
>> +struct S2 {
>> + struct S1 _s1;
>> + unsigned long _x2;
>> +} ;
>> +
>> +struct ufld_type1 {
>> + unsigned int _u1t;
>> + struct S2 _s2;
>> +} ;
>> +
>> +struct ufld_type2 {
>> + unsigned int _u2t;
>> + struct S1 _s1;
>> +} ;
>> +struct parm_type {
>> + union {
>> + struct ufld_type1 var_1;
>> + struct ufld_type2 var_2;
>> + } U;
>> +};
>> +
>> +struct parm_type bad_function( struct parm_type arg0 )
>> +{
>> + struct parm_type rv;
>> + struct S2 var4;
>> + switch( arg0.U.var_2._u2t ) {
>> + case 4294967041:
>> + var4._s1 = arg0.U.var_1._s2._s1;
>> + rv.U.var_1._u1t = 4294967041;
>> + rv.U.var_1._s2 = var4;
>> + break;
>> + case 4294967043:
>> + rv.U.var_2._u2t = 4294967043;
>> + rv.U.var_2._s1 = arg0.U.var_2._s1;
>> + break;
>> + default:
>> + break;
>> + }
>> + return rv;
>> +}
>> +
>> +int main() {
>> + struct parm_type val;
>> + struct parm_type out;
>> + val.U.var_2._u2t = 4294967043;
>> + val.U.var_2._s1._0 = 0x01010101;
>> + val.U.var_2._s1._1 = 0x02020202;
>> + out = bad_function(val);
>> + if (val.U.var_2._u2t != 4294967043)
>> + __builtin_abort ();
>> + if (out.U.var_2._s1._0 != 0x01010101)
>> + __builtin_abort ();
>> + if (val.U.var_2._s1._1 != 0x02020202 )
>> + __builtin_abort ();
>> + return 0;
>> +}
>> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
>> index 081c51b58a4..099e8dbe873 100644
>> --- a/gcc/tree-sra.cc
>> +++ b/gcc/tree-sra.cc
>> @@ -1667,7 +1667,18 @@ build_ref_for_offset (location_t loc, tree base,
>> poly_int64 offset,
>> static tree
>> build_reconstructed_reference (location_t, tree base, struct access *model)
>> {
>> - tree expr = model->expr, prev_expr = NULL;
>> + tree expr = model->expr;
>> + /* We have to make sure to start just below the outermost union. */
>> + tree start_expr = expr;
>> + while (handled_component_p (expr))
>> + {
>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (expr, 0))) == UNION_TYPE)
>> + start_expr = expr;
>> + expr = TREE_OPERAND (expr, 0);
>> + }
>> +
>> + expr = start_expr;
>> + tree prev_expr = NULL_TREE;
>> while (!types_compatible_p (TREE_TYPE (expr), TREE_TYPE (base)))
>> {
>> if (!handled_component_p (expr))
>>
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstra
