[PR 89209] Avoid segfault in a peculiar corner case in SRA

[Richard Biener]

On February 16, 2019 11:56:13 AM GMT+01:00, Martin Jambor <mjambor@suse.cz> wrote:
>Hi,
>
>PR 89209 takes place because SRA on trunk encounters an assignment into
>an SSA_NAME from a V_C_E of a structure load which however cannot
>contain any useful data because (it is not addressable and) there is no
>store to that portion of the aggregate in the entire function.  In such
>circumstances, SRA conjures up a default-definition SSA name and
>replaces the RHS of the load with it so that an uninitialized warning
>is
>generated.  Unfortunately, the code digging through V_C_Es badly
>interacts with this and what happens is that first we create an
>aggregate type SSA name which the code avoiding creation of additional
>V_C_Es then tries to store "into" the SSA name on the LHS, which of
>course fails.  BTW, I was surprised no verifier caught the aggregate
>SSA
>name if I just avoided the segfaulting path.
>
>Fixed with the patch below which gives the code creating the
>default-definition SSA_NAME an alternative type which is used if the
>access type is not a gimple_register_typoe.  I have also added an
>additional test that lacc is not NULL to sra_modify_assign because the
>code path could trigger if the created default-def SSA_NAME happens to
>be loaded as two different types.  However, I have not managed to
>quickly create a testcase that would lead to it..
>
>Bootstrapped and tested on x86_64-linux.  OK for trunk?

OK. 

Richard. 

>Thanks,
>
>Martin
>
>
>2019-02-15  Martin Jambor  <mjambor@suse.cz>
>
>	PR tree-optimization/89209
>	* tree-sra.c (create_access_replacement): New optional parameter
>	reg_tree.  Use it as a type if non-NULL and access type is not of
>	a register type.
>	(get_repl_default_def_ssa_name): New parameter REG_TYPE, pass it
>	to create_access_replacement.
>	(sra_modify_assign): Pass LHS type to get_repl_default_def_ssa_name.
>	Check lacc is non-NULL before attempting to re-create it on the RHS.
>
>	testsuite/
>	* gcc.dg/tree-ssa/pr89209.c: New test.
>---
> gcc/testsuite/gcc.dg/tree-ssa/pr89209.c | 16 ++++++++++++
> gcc/tree-sra.c                          | 34 +++++++++++++++----------
> 2 files changed, 37 insertions(+), 13 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr89209.c
>
>diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr89209.c
>b/gcc/testsuite/gcc.dg/tree-ssa/pr89209.c
>new file mode 100644
>index 00000000000..f01bda9ae5c
>--- /dev/null
>+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr89209.c
>@@ -0,0 +1,16 @@
>+/* { dg-do compile } */
>+/* { dg-options "-O2" } */
>+
>+struct S {
>+  short a, b;
>+};
>+struct T {
>+  int c;
>+  struct S s;
>+};
>+int f ()
>+{
>+  struct T t;
>+  t.c = t.s.a || t.s.b;
>+  return t.c;
>+}
>diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>index e4851daaa3f..eeef31ba496 100644
>--- a/gcc/tree-sra.c
>+++ b/gcc/tree-sra.c
>@@ -2195,13 +2195,20 @@ sort_and_splice_var_accesses (tree var)
> 
>/* Create a variable for the given ACCESS which determines the type,
>name and a
>few other properties.  Return the variable declaration and store it
>also to
>-   ACCESS->replacement.  */
>+   ACCESS->replacement.  REG_TREE is used when creating a declaration
>to base a
>+   default-definition SSA name on on in order to facilitate an
>uninitialized
>+   warning.  It is used instead of the actual ACCESS type if that is
>not of a
>+   gimple register type.  */
> 
> static tree
>-create_access_replacement (struct access *access)
>+create_access_replacement (struct access *access, tree reg_type =
>NULL_TREE)
> {
>   tree repl;
> 
>+  tree type = access->type;
>+  if (reg_type && !is_gimple_reg_type (type))
>+    type = reg_type;
>+
>   if (access->grp_to_be_debug_replaced)
>     {
>       repl = create_tmp_var_raw (access->type);
>@@ -2210,17 +2217,16 @@ create_access_replacement (struct access
>*access)
>   else
>     /* Drop any special alignment on the type if it's not on the main
>        variant.  This avoids issues with weirdo ABIs like AAPCS.  */
>-    repl = create_tmp_var (build_qualified_type
>-			     (TYPE_MAIN_VARIANT (access->type),
>-			      TYPE_QUALS (access->type)), "SR");
>-  if (TREE_CODE (access->type) == COMPLEX_TYPE
>-      || TREE_CODE (access->type) == VECTOR_TYPE)
>+    repl = create_tmp_var (build_qualified_type (TYPE_MAIN_VARIANT
>(type),
>+						 TYPE_QUALS (type)), "SR");
>+  if (TREE_CODE (type) == COMPLEX_TYPE
>+      || TREE_CODE (type) == VECTOR_TYPE)
>     {
>       if (!access->grp_partial_lhs)
> 	DECL_GIMPLE_REG_P (repl) = 1;
>     }
>   else if (access->grp_partial_lhs
>-	   && is_gimple_reg_type (access->type))
>+	   && is_gimple_reg_type (type))
>     TREE_ADDRESSABLE (repl) = 1;
> 
>   DECL_SOURCE_LOCATION (repl) = DECL_SOURCE_LOCATION (access->base);
>@@ -3450,15 +3456,16 @@ sra_modify_constructor_assign (gimple *stmt,
>gimple_stmt_iterator *gsi)
> 
>/* Create and return a new suitable default definition SSA_NAME for
>RACC which
>is an access describing an uninitialized part of an aggregate that is
>being
>-   loaded.  */
>+   loaded.  REG_TREE is used instead of the actual RACC type if that
>is not of
>+   a gimple register type.  */
> 
> static tree
>-get_repl_default_def_ssa_name (struct access *racc)
>+get_repl_default_def_ssa_name (struct access *racc, tree reg_type)
> {
>   gcc_checking_assert (!racc->grp_to_be_replaced
> 		       && !racc->grp_to_be_debug_replaced);
>   if (!racc->replacement_decl)
>-    racc->replacement_decl = create_access_replacement (racc);
>+    racc->replacement_decl = create_access_replacement (racc,
>reg_type);
>   return get_or_create_ssa_default_def (cfun, racc->replacement_decl);
> }
> 
>@@ -3530,7 +3537,7 @@ sra_modify_assign (gimple *stmt,
>gimple_stmt_iterator *gsi)
> 	   && TREE_CODE (lhs) == SSA_NAME
> 	   && !access_has_replacements_p (racc))
>     {
>-      rhs = get_repl_default_def_ssa_name (racc);
>+      rhs = get_repl_default_def_ssa_name (racc, TREE_TYPE (lhs));
>       modify_this_stmt = true;
>       sra_stats.exprs++;
>     }
>@@ -3548,7 +3555,8 @@ sra_modify_assign (gimple *stmt,
>gimple_stmt_iterator *gsi)
> 	      lhs = build_ref_for_model (loc, lhs, 0, racc, gsi, false);
> 	      gimple_assign_set_lhs (stmt, lhs);
> 	    }
>-	  else if (AGGREGATE_TYPE_P (TREE_TYPE (rhs))
>+	  else if (lacc
>+		   && AGGREGATE_TYPE_P (TREE_TYPE (rhs))
> 		   && !contains_vce_or_bfcref_p (rhs))
> 	    rhs = build_ref_for_model (loc, rhs, 0, lacc, gsi, false);
>