Patchwork Avoid invalid sharing of ADDR_EXPRs (PR fortran/55935)

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 11, 2013, 7:46 p.m.
Message ID <20130111194646.GH7269@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/211413/
State New
Headers show

Comments

Jakub Jelinek - Jan. 11, 2013, 7:46 p.m.
Hi!

As discussed in the PR, the extra verification of location blocks Richard
posted recently fails on some Fortran testcases.  The problem is that
ADDR_EXPRs in static const var initializers contain locations (fixed by the
trans-expr.c hunks), and that gimple-fold makes the ADDR_EXPRs shared, so
even with the fortran FE hunks alone when the gimplifier sets location
we get invalid blocks anyway.  In the PR we were discussing putting the
unshare_expr at the beginning of canonicalize_constructor_val, unfortunately
that breaks Ada bootstrap, because it is undesirable when record_reference
calls that function.  So this alternative patch moves the unshare_expr calls
to gimple-fold.c canonicalize_constructor_val callers.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Alternatively the get_symbol_constant_value unshare_expr call could move
from canonicalize_constructor_val argument to return val; line, both
locations have some advantages and disadvantages (the one in patch might
create unnecessary garbage if canonicalize_constructor_val returns NULL
or non-min_invariant, the other way would create garbage with the trees
created by canonicalize_constructor_val itself (they would be created
and immediately unshared).

2013-01-11  Jakub Jelinek  <jakub@redhat.com>

	PR fortran/55935
	* gimple-fold.c (get_symbol_constant_value): Call
	unshare_expr.
	(fold_gimple_assign): Don't call unshare_expr here.
	(fold_ctor_reference): Call unshare_expr.

	* trans-expr.c (gfc_conv_structure): Call
	unshare_expr_without_location on the ctor elements.


	Jakub
Richard Guenther - Jan. 12, 2013, 8:10 a.m.
Jakub Jelinek <jakub@redhat.com> wrote:

>Hi!
>
>As discussed in the PR, the extra verification of location blocks
>Richard
>posted recently fails on some Fortran testcases.  The problem is that
>ADDR_EXPRs in static const var initializers contain locations (fixed by
>the
>trans-expr.c hunks), and that gimple-fold makes the ADDR_EXPRs shared,
>so
>even with the fortran FE hunks alone when the gimplifier sets location
>we get invalid blocks anyway.  In the PR we were discussing putting the
>unshare_expr at the beginning of canonicalize_constructor_val,
>unfortunately
>that breaks Ada bootstrap, because it is undesirable when
>record_reference
>calls that function.  So this alternative patch moves the unshare_expr
>calls
>to gimple-fold.c canonicalize_constructor_val callers.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

>Alternatively the get_symbol_constant_value unshare_expr call could
>move
>from canonicalize_constructor_val argument to return val; line, both
>locations have some advantages and disadvantages (the one in patch
>might
>create unnecessary garbage if canonicalize_constructor_val returns NULL
>or non-min_invariant, the other way would create garbage with the trees
>created by canonicalize_constructor_val itself (they would be created
>and immediately unshared).
>
>2013-01-11  Jakub Jelinek  <jakub@redhat.com>
>
>	PR fortran/55935
>	* gimple-fold.c (get_symbol_constant_value): Call
>	unshare_expr.
>	(fold_gimple_assign): Don't call unshare_expr here.
>	(fold_ctor_reference): Call unshare_expr.
>
>	* trans-expr.c (gfc_conv_structure): Call
>	unshare_expr_without_location on the ctor elements.
>
>--- gcc/gimple-fold.c.jj	2013-01-11 09:02:55.000000000 +0100
>+++ gcc/gimple-fold.c	2013-01-11 15:42:52.485630537 +0100
>@@ -202,7 +202,7 @@ get_symbol_constant_value (tree sym)
>       tree val = DECL_INITIAL (sym);
>       if (val)
> 	{
>-	  val = canonicalize_constructor_val (val, sym);
>+	  val = canonicalize_constructor_val (unshare_expr (val), sym);
> 	  if (val && is_gimple_min_invariant (val))
> 	    return val;
> 	  else
>@@ -378,7 +378,7 @@ fold_gimple_assign (gimple_stmt_iterator
> 	  }
> 
> 	else if (DECL_P (rhs))
>-	  return unshare_expr (get_symbol_constant_value (rhs));
>+	  return get_symbol_constant_value (rhs);
> 
>         /* If we couldn't fold the RHS, hand over to the generic
>            fold routines.  */
>@@ -2941,7 +2941,7 @@ fold_ctor_reference (tree type, tree cto
>   /* We found the field with exact match.  */
>   if (useless_type_conversion_p (type, TREE_TYPE (ctor))
>       && !offset)
>-    return canonicalize_constructor_val (ctor, from_decl);
>+    return canonicalize_constructor_val (unshare_expr (ctor),
>from_decl);
> 
>   /* We are at the end of walk, see if we can view convert the
>      result.  */
>@@ -2950,7 +2950,7 @@ fold_ctor_reference (tree type, tree cto
>       && operand_equal_p (TYPE_SIZE (type),
> 			  TYPE_SIZE (TREE_TYPE (ctor)), 0))
>     {
>-      ret = canonicalize_constructor_val (ctor, from_decl);
>+      ret = canonicalize_constructor_val (unshare_expr (ctor),
>from_decl);
>       ret = fold_unary (VIEW_CONVERT_EXPR, type, ret);
>       if (ret)
> 	STRIP_NOPS (ret);
>--- gcc/fortran/trans-expr.c.jj	2013-01-11 09:02:50.000000000 +0100
>+++ gcc/fortran/trans-expr.c	2013-01-11 10:43:54.071921147 +0100
>@@ -6137,6 +6137,7 @@ gfc_conv_structure (gfc_se * se, gfc_exp
> 	  gfc_symbol *vtabs;
> 	  vtabs = cm->initializer->symtree->n.sym;
>	  vtab = gfc_build_addr_expr (NULL_TREE, gfc_get_symbol_decl (vtabs));
>+	  vtab = unshare_expr_without_location (vtab);
> 	  CONSTRUCTOR_APPEND_ELT (v, cm->backend_decl, vtab);
> 	}
>       else if (cm->ts.u.derived && strcmp (cm->name, "_size") == 0)
>@@ -6150,6 +6151,7 @@ gfc_conv_structure (gfc_se * se, gfc_exp
> 				      TREE_TYPE (cm->backend_decl),
> 				      cm->attr.dimension, cm->attr.pointer,
> 				      cm->attr.proc_pointer);
>+	  val = unshare_expr_without_location (val);
> 
> 	  /* Append it to the constructor list.  */
> 	  CONSTRUCTOR_APPEND_ELT (v, cm->backend_decl, val);
>
>	Jakub

Patch

--- gcc/gimple-fold.c.jj	2013-01-11 09:02:55.000000000 +0100
+++ gcc/gimple-fold.c	2013-01-11 15:42:52.485630537 +0100
@@ -202,7 +202,7 @@  get_symbol_constant_value (tree sym)
       tree val = DECL_INITIAL (sym);
       if (val)
 	{
-	  val = canonicalize_constructor_val (val, sym);
+	  val = canonicalize_constructor_val (unshare_expr (val), sym);
 	  if (val && is_gimple_min_invariant (val))
 	    return val;
 	  else
@@ -378,7 +378,7 @@  fold_gimple_assign (gimple_stmt_iterator
 	  }
 
 	else if (DECL_P (rhs))
-	  return unshare_expr (get_symbol_constant_value (rhs));
+	  return get_symbol_constant_value (rhs);
 
         /* If we couldn't fold the RHS, hand over to the generic
            fold routines.  */
@@ -2941,7 +2941,7 @@  fold_ctor_reference (tree type, tree cto
   /* We found the field with exact match.  */
   if (useless_type_conversion_p (type, TREE_TYPE (ctor))
       && !offset)
-    return canonicalize_constructor_val (ctor, from_decl);
+    return canonicalize_constructor_val (unshare_expr (ctor), from_decl);
 
   /* We are at the end of walk, see if we can view convert the
      result.  */
@@ -2950,7 +2950,7 @@  fold_ctor_reference (tree type, tree cto
       && operand_equal_p (TYPE_SIZE (type),
 			  TYPE_SIZE (TREE_TYPE (ctor)), 0))
     {
-      ret = canonicalize_constructor_val (ctor, from_decl);
+      ret = canonicalize_constructor_val (unshare_expr (ctor), from_decl);
       ret = fold_unary (VIEW_CONVERT_EXPR, type, ret);
       if (ret)
 	STRIP_NOPS (ret);
--- gcc/fortran/trans-expr.c.jj	2013-01-11 09:02:50.000000000 +0100
+++ gcc/fortran/trans-expr.c	2013-01-11 10:43:54.071921147 +0100
@@ -6137,6 +6137,7 @@  gfc_conv_structure (gfc_se * se, gfc_exp
 	  gfc_symbol *vtabs;
 	  vtabs = cm->initializer->symtree->n.sym;
 	  vtab = gfc_build_addr_expr (NULL_TREE, gfc_get_symbol_decl (vtabs));
+	  vtab = unshare_expr_without_location (vtab);
 	  CONSTRUCTOR_APPEND_ELT (v, cm->backend_decl, vtab);
 	}
       else if (cm->ts.u.derived && strcmp (cm->name, "_size") == 0)
@@ -6150,6 +6151,7 @@  gfc_conv_structure (gfc_se * se, gfc_exp
 				      TREE_TYPE (cm->backend_decl),
 				      cm->attr.dimension, cm->attr.pointer,
 				      cm->attr.proc_pointer);
+	  val = unshare_expr_without_location (val);
 
 	  /* Append it to the constructor list.  */
 	  CONSTRUCTOR_APPEND_ELT (v, cm->backend_decl, val);