Re: [PATCH] tree-sra: Fix union handling in build_reconstructed_reference (PR 105860)

[Richard Biener <rguenther@suse.de>]

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

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> gcc/ChangeLog:
> 
> 2022-07-01  Martin Jambor  <mjambor@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  <mjambor@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstra