| Message ID | ri6h8d4q9wy.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,89209] Avoid segfault in a peculiar corner case in SRA | expand |
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); >
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);