[PATCH] Fix type field walking in gimplifier unsharing

[PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

Gimplification is done in-place and thus relies on all processed
trees being unshared.  This is achieved by unshare_body which
in the end uses walk_tree to get at all interesting trees that possibly
need unsharing.

Unfortunately it doesn't really work because walk_tree only walks
types and type-related fields (TYPE_SIZE, TYPE_MIN_VALUE, etc.) in
very narrow circumstances.

The symptom of failing to unshare trees used in those fields and
in the IL of a function body that is gimplified is that those
trees get modified by the gimplification process which basically
leaks temporary decls into them.

Note that only type sizes that are actually needed for the IL
of a function are gimplified and thus there are referenced
types with still save-expr TYPE_SIZE after gimplification.

Eventually dwarf2out knows to emit debug info for those by
generating dwarf expressions but will be surely not able to do
so if the expressions contain random local decls that may no
longer be there.

With my patch to make the gimplifier use SSA names for temporaries
those may now leak into the non-gimplified TYPE_SIZEs and if they
are later released SSA names with NULL_TREE type (or even garbage
memory if the SSA name is ggc freed) is in there.  This crashes
in various ways when those trees are accessed (in dwarf2out, in the
LTO streamer, in other places looking at pointed-to types).

Thus the following patch which makes the gimplifier unsharing
visit all types.

Alternative patches include unsharing trees when we build a
save_expr around them, doing that only when stor-layout does
this to TYPE_SIZE fields (and friends) or try to have an
extra pass over the GENERIC IL to just mark the expressions
in the type fields not walked by the current walking (might
turn out tricky but it would result in the "least" unsharing
thus only unshare the parts we eventually gimplify).

It might be that we just need to declare tree sharing that runs
into the above issue as invalid (they involve Fortran testcases
or ubsan testcases only as far as I can see - interestingly
no Ada testcases are affected).

So - any opinion on the "correct" way to fix this?  Clearly
the gimplifier running into shared trees is a bug.

Thanks,
Richard.

2016-04-27  Richard Biener  <rguenther@suse.de>

	* gimplify.c (copy_if_shared_r): Walk types, type sizes
	and bounds.
	(unmark_visited_r): Unmark them.

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c.orig	2016-04-27 10:29:54.784677194 +0200
+++ gcc/gimplify.c	2016-04-27 10:29:38.708496357 +0200
@@ -832,31 +832,61 @@ copy_if_shared_r (tree *tp, int *walk_su
   tree t = *tp;
   enum tree_code code = TREE_CODE (t);
 
-  /* Skip types, decls, and constants.  But we do want to look at their
-     types and the bounds of types.  Mark them as visited so we properly
-     unmark their subtrees on the unmark pass.  If we've already seen them,
-     don't look down further.  */
-  if (TREE_CODE_CLASS (code) == tcc_type
-      || TREE_CODE_CLASS (code) == tcc_declaration
+  bool was_visited = TREE_VISITED (t);
+
+  /* If the node wasn't visited already mark it so and recurse on its type.  */
+  if (! was_visited)
+    {
+      TREE_VISITED (t) = 1;
+
+      /* walk_tree does not descend into TREE_TYPE unless this is a
+	 DECL_EXPR of a TYPE_DECL.  */
+      if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
+	walk_tree (&TREE_TYPE (t), copy_if_shared_r, data, NULL);
+    }
+
+  /* Skip types, decls, and constants for copying.  But we do want to look
+     at their types and the bounds of types.  Mark them as visited so we
+     properly unmark their subtrees on the unmark pass.  If we've already
+     seen them, don't look down further.  */
+  if (TREE_CODE_CLASS (code) == tcc_declaration
       || TREE_CODE_CLASS (code) == tcc_constant)
     {
-      if (TREE_VISITED (t))
+      if (was_visited)
+	*walk_subtrees = 0;
+    }
+  else if (TREE_CODE_CLASS (code) == tcc_type)
+    {
+      if (was_visited)
 	*walk_subtrees = 0;
       else
-	TREE_VISITED (t) = 1;
+	{
+	  /* walk_type_fields does not walk type sizes or bounds if not
+	     coming directly from a DECL_EXPR context.  */
+	  /* ???  ideally we'd mark all exprs reached via types as visited
+	     before copy_if_shared so duplicates in types that do not matter
+	     for gimplification are never unshared.  */
+	  walk_tree (&TYPE_SIZE (t), copy_if_shared_r, data, NULL);
+	  walk_tree (&TYPE_SIZE_UNIT (t), copy_if_shared_r, data, NULL);
+	  if (INTEGRAL_TYPE_P (t)
+	      || TREE_CODE (t) == FIXED_POINT_TYPE
+	      || TREE_CODE (t) == REAL_TYPE)
+	    {
+	      walk_tree (&TYPE_MAX_VALUE (t), copy_if_shared_r, data, NULL);
+	      walk_tree (&TYPE_MIN_VALUE (t), copy_if_shared_r, data, NULL);
+	    }
+	}
     }
 
   /* If this node has been visited already, unshare it and don't look
      any deeper.  */
-  else if (TREE_VISITED (t))
+  else if (was_visited)
     {
       walk_tree (tp, mostly_copy_tree_r, data, NULL);
       *walk_subtrees = 0;
     }
 
-  /* Otherwise, mark the node as visited and keep looking.  */
-  else
-    TREE_VISITED (t) = 1;
+  /* Otherwise keep looking.  */
 
   return NULL_TREE;
 }
@@ -903,7 +933,25 @@ unmark_visited_r (tree *tp, int *walk_su
 
   /* If this node has been visited, unmark it and keep looking.  */
   if (TREE_VISITED (t))
-    TREE_VISITED (t) = 0;
+    {
+      TREE_VISITED (t) = 0;
+
+      if (TYPE_P (t))
+	{
+	  walk_tree (&TYPE_SIZE (t), unmark_visited_r, NULL, NULL);
+	  walk_tree (&TYPE_SIZE_UNIT (t), unmark_visited_r, NULL, NULL);
+	  if (INTEGRAL_TYPE_P (t)
+	      || TREE_CODE (t) == FIXED_POINT_TYPE
+	      || TREE_CODE (t) == REAL_TYPE)
+	    {
+	      walk_tree (&TYPE_MAX_VALUE (t), unmark_visited_r, NULL, NULL);
+	      walk_tree (&TYPE_MIN_VALUE (t), unmark_visited_r, NULL, NULL);
+	    }
+	}
+
+      if (CODE_CONTAINS_STRUCT (TREE_CODE (t), TS_TYPED))
+	walk_tree (&TREE_TYPE (t), unmark_visited_r, NULL, NULL);
+    }
 
   /* Otherwise, don't look any deeper.  */
   else

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Eric Botcazou]

> Gimplification is done in-place and thus relies on all processed
> trees being unshared.  This is achieved by unshare_body which
> in the end uses walk_tree to get at all interesting trees that possibly
> need unsharing.
> 
> Unfortunately it doesn't really work because walk_tree only walks
> types and type-related fields (TYPE_SIZE, TYPE_MIN_VALUE, etc.) in
> very narrow circumstances.

Right, but well defined and explained:

    case DECL_EXPR:
      /* If this is a TYPE_DECL, walk into the fields of the type that it's
	 defining.  We only want to walk into these fields of a type in this
	 case and not in the general case of a mere reference to the type.

	 The criterion is as follows: if the field can be an expression, it
	 must be walked only here.  This should be in keeping with the fields
	 that are directly gimplified in gimplify_type_sizes in order for the
	 mark/copy-if-shared/unmark machinery of the gimplifier to work with
	 variable-sized types.

	 Note that DECLs get walked as part of processing the BIND_EXPR.  */

> Thus the following patch which makes the gimplifier unsharing
> visit all types.

I think this will generate a lot of useless walking in Ada...

> So - any opinion on the "correct" way to fix this?

Add DECL_EXPRs for the types, that's what done in Ada.

-- 
Eric Botcazou

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

On Wed, 27 Apr 2016, Eric Botcazou wrote:

> > Gimplification is done in-place and thus relies on all processed
> > trees being unshared.  This is achieved by unshare_body which
> > in the end uses walk_tree to get at all interesting trees that possibly
> > need unsharing.
> > 
> > Unfortunately it doesn't really work because walk_tree only walks
> > types and type-related fields (TYPE_SIZE, TYPE_MIN_VALUE, etc.) in
> > very narrow circumstances.
> 
> Right, but well defined and explained:
> 
>     case DECL_EXPR:
>       /* If this is a TYPE_DECL, walk into the fields of the type that it's
> 	 defining.  We only want to walk into these fields of a type in this
> 	 case and not in the general case of a mere reference to the type.
> 
> 	 The criterion is as follows: if the field can be an expression, it
> 	 must be walked only here.  This should be in keeping with the fields
> 	 that are directly gimplified in gimplify_type_sizes in order for the
> 	 mark/copy-if-shared/unmark machinery of the gimplifier to work with
> 	 variable-sized types.
> 
> 	 Note that DECLs get walked as part of processing the BIND_EXPR.  */
> 
> > Thus the following patch which makes the gimplifier unsharing
> > visit all types.
> 
> I think this will generate a lot of useless walking in Ada...
> 
> > So - any opinion on the "correct" way to fix this?
> 
> Add DECL_EXPRs for the types, that's what done in Ada.

Aww, I was hoping for sth that would not require me to fix all
frontends ...

It seems the C frontend does it correctly already - I hit the
ubsan issue for c-c++-common/ubsan/pr59667.c and only for the C++ FE
for example.  Notice how only the pointed-to type is variable-size
here.

C produces

{
  unsigned int len = 1;
  typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> + 
-1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
  float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype) 
((long int) SAVE_EXPR <len> + -1)] * P = 0B;

    unsigned int len = 1;
    typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> + 
-1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
  SAVE_EXPR <len>;, (void) SAVE_EXPR <len>;;
    float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype) 
((long int) SAVE_EXPR <len> + -1)] * P = 0B;
  (*P)[0][0] = 1.0e+0;
  return 0;
}

the decl-expr is the 'typedef' line.  The C++ FE produces

{
  unsigned int len = 1;
  float[0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)][0:(sizetype) 
(SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;

  <<cleanup_point   unsigned int len = 1;>>;
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) (((bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) + 
1) * (bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) + 1)) * 
32) >>>>>;
  <<cleanup_point   float[0:(sizetype) (SAVE_EXPR <(ssizetype) len + 
-1>)][0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;>>;
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) ((*P)[0][0] = 1.0e+0) >>>>>;
  return <retval> = 0;
}

notice the lack of a decl-expr here.  It has some weird expr_stmt
here covering the sizes though. Possibly because VLA arrays are a GNU 
extension.

Didn't look into the fortran FE issue but I expect it's similar
(it only occurs for pointers to VLAs as well).

I'll try to come up with patches.

Thanks for the hint,
Richard.

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Eric Botcazou]

> Aww, I was hoping for sth that would not require me to fix all
> frontends ...

I don't really see how this can work without DECL_EXPR though.  You need to 
define when the variable-sized expressions are evaluated to lay out the type, 
otherwise it will be laid out on the first use, which may see a different 
value of the expressions than the definition point.  The only way to do that 
for a locally-defined type is to add a DECL_EXPR in GENERIC, so that the 
gimplifier evaluates the expressions at the right spot.

Of course in Ada we have the ACATS testsuite which tests for this kind of 
things, this explains why it already works.

> It seems the C frontend does it correctly already - I hit the
> ubsan issue for c-c++-common/ubsan/pr59667.c and only for the C++ FE
> for example.  Notice how only the pointed-to type is variable-size
> here.
> 
> C produces
> 
> {
>   unsigned int len = 1;
>   typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> +
> -1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
>   float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype)
> ((long int) SAVE_EXPR <len> + -1)] * P = 0B;
> 
>     unsigned int len = 1;
>     typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> +
> -1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
>   SAVE_EXPR <len>;, (void) SAVE_EXPR <len>;;
>     float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype)
> ((long int) SAVE_EXPR <len> + -1)] * P = 0B;
>   (*P)[0][0] = 1.0e+0;
>   return 0;
> }
> 
> the decl-expr is the 'typedef' line.  The C++ FE produces
> 
> {
>   unsigned int len = 1;
>   float[0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)][0:(sizetype)
> (SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;
> 
>   <<cleanup_point   unsigned int len = 1;>>;
>   <<cleanup_point <<< Unknown tree: expr_stmt
>   (void) (((bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) +
> 1) * (bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) + 1)) *
> 32) >>>>>;
>   <<cleanup_point   float[0:(sizetype) (SAVE_EXPR <(ssizetype) len +
> -1>)][0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;>>;
>   <<cleanup_point <<< Unknown tree: expr_stmt
>   (void) ((*P)[0][0] = 1.0e+0) >>>>>;
>   return <retval> = 0;
> }
> 
> notice the lack of a decl-expr here.  It has some weird expr_stmt
> here covering the sizes though. Possibly because VLA arrays are a GNU
> extension.

Indeed.

> Didn't look into the fortran FE issue but I expect it's similar
> (it only occurs for pointers to VLAs as well).
> 
> I'll try to come up with patches.
> 
> Thanks for the hint,

You're welcome.

-- 
Eric Botcazou

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

On Thu, 28 Apr 2016, Eric Botcazou wrote:

> > Aww, I was hoping for sth that would not require me to fix all
> > frontends ...
> 
> I don't really see how this can work without DECL_EXPR though.  You need to 
> define when the variable-sized expressions are evaluated to lay out the type, 
> otherwise it will be laid out on the first use, which may see a different 
> value of the expressions than the definition point.  The only way to do that 
> for a locally-defined type is to add a DECL_EXPR in GENERIC, so that the 
> gimplifier evaluates the expressions at the right spot.

Ah, so the C++ FE does this correctly but in addition to that it has

          /* When the pointed-to type involves components of variable 
size,
             care must be taken to ensure that the size evaluation code is
             emitted early enough to dominate all the possible later uses
             and late enough for the variables on which it depends to have
             been assigned.

             This is expected to happen automatically when the pointed-to
             type has a name/declaration of it's own, but special 
attention
             is required if the type is anonymous.
...
          if (!TYPE_NAME (type)
              && (decl_context == NORMAL || decl_context == FIELD)
              && at_function_scope_p ()
              && variably_modified_type_p (type, NULL_TREE))
            /* Force evaluation of the SAVE_EXPR.  */
            finish_expr_stmt (TYPE_SIZE (type));

so in this case the type doesn't have an associated TYPE_DECL and thus
we can't build a DECL_EXPR.  To me the correct fix is then to
always force a TYPE_DECL for variable-modified types.

Jason?

Now digging into the Fortran FE equivalent case...

Thanks,
Richard.

> Of course in Ada we have the ACATS testsuite which tests for this kind of 
> things, this explains why it already works.
> 
> > It seems the C frontend does it correctly already - I hit the
> > ubsan issue for c-c++-common/ubsan/pr59667.c and only for the C++ FE
> > for example.  Notice how only the pointed-to type is variable-size
> > here.
> > 
> > C produces
> > 
> > {
> >   unsigned int len = 1;
> >   typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> +
> > -1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
> >   float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype)
> > ((long int) SAVE_EXPR <len> + -1)] * P = 0B;
> > 
> >     unsigned int len = 1;
> >     typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> +
> > -1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
> >   SAVE_EXPR <len>;, (void) SAVE_EXPR <len>;;
> >     float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype)
> > ((long int) SAVE_EXPR <len> + -1)] * P = 0B;
> >   (*P)[0][0] = 1.0e+0;
> >   return 0;
> > }
> > 
> > the decl-expr is the 'typedef' line.  The C++ FE produces
> > 
> > {
> >   unsigned int len = 1;
> >   float[0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)][0:(sizetype)
> > (SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;
> > 
> >   <<cleanup_point   unsigned int len = 1;>>;
> >   <<cleanup_point <<< Unknown tree: expr_stmt
> >   (void) (((bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) +
> > 1) * (bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) + 1)) *
> > 32) >>>>>;
> >   <<cleanup_point   float[0:(sizetype) (SAVE_EXPR <(ssizetype) len +
> > -1>)][0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;>>;
> >   <<cleanup_point <<< Unknown tree: expr_stmt
> >   (void) ((*P)[0][0] = 1.0e+0) >>>>>;
> >   return <retval> = 0;
> > }
> > 
> > notice the lack of a decl-expr here.  It has some weird expr_stmt
> > here covering the sizes though. Possibly because VLA arrays are a GNU
> > extension.
> 
> Indeed.
> 
> > Didn't look into the fortran FE issue but I expect it's similar
> > (it only occurs for pointers to VLAs as well).
> > 
> > I'll try to come up with patches.
> > 
> > Thanks for the hint,
> 
> You're welcome.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

On Thu, 28 Apr 2016, Richard Biener wrote:

> On Thu, 28 Apr 2016, Eric Botcazou wrote:
> 
> > > Aww, I was hoping for sth that would not require me to fix all
> > > frontends ...
> > 
> > I don't really see how this can work without DECL_EXPR though.  You need to 
> > define when the variable-sized expressions are evaluated to lay out the type, 
> > otherwise it will be laid out on the first use, which may see a different 
> > value of the expressions than the definition point.  The only way to do that 
> > for a locally-defined type is to add a DECL_EXPR in GENERIC, so that the 
> > gimplifier evaluates the expressions at the right spot.
> 
> Ah, so the C++ FE does this correctly but in addition to that it has
> 
>           /* When the pointed-to type involves components of variable 
> size,
>              care must be taken to ensure that the size evaluation code is
>              emitted early enough to dominate all the possible later uses
>              and late enough for the variables on which it depends to have
>              been assigned.
> 
>              This is expected to happen automatically when the pointed-to
>              type has a name/declaration of it's own, but special 
> attention
>              is required if the type is anonymous.
> ...
>           if (!TYPE_NAME (type)
>               && (decl_context == NORMAL || decl_context == FIELD)
>               && at_function_scope_p ()
>               && variably_modified_type_p (type, NULL_TREE))
>             /* Force evaluation of the SAVE_EXPR.  */
>             finish_expr_stmt (TYPE_SIZE (type));
> 
> so in this case the type doesn't have an associated TYPE_DECL and thus
> we can't build a DECL_EXPR.  To me the correct fix is then to
> always force a TYPE_DECL for variable-modified types.
> 
> Jason?

The following works (for the testcase):

Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c       (revision 235547)
+++ gcc/cp/decl.c       (working copy)
@@ -10393,8 +10393,11 @@ grokdeclarator (const cp_declarator *dec
              && (decl_context == NORMAL || decl_context == FIELD)
              && at_function_scope_p ()
              && variably_modified_type_p (type, NULL_TREE))
-           /* Force evaluation of the SAVE_EXPR.  */
-           finish_expr_stmt (TYPE_SIZE (type));
+           {
+             TYPE_NAME (type) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
+                                            NULL_TREE, type);
+             add_decl_expr (TYPE_NAME (type));
+           }
 
          if (declarator->kind == cdk_reference)
            {

and I have a similar fix for the Fortran FE for one testcase I
reduced to

  character(10), dimension (2) :: implicit_result
  character(10), dimension (2) :: source
  implicit_result = reallocate_hnv (LEN (source))
contains
  FUNCTION reallocate_hnv(LEN)
    CHARACTER(LEN=LEN), DIMENSION(:), POINTER :: reallocate_hnv
  END FUNCTION reallocate_hnv
end

Index: fortran/trans-array.c
===================================================================
--- fortran/trans-array.c       (revision 235547)
+++ fortran/trans-array.c       (working copy)
@@ -1094,6 +1094,16 @@ gfc_trans_create_temp_array (stmtblock_t
   info->descriptor = desc;
   size = gfc_index_one_node;
 
+  /* Emit a DECL_EXPR for the variable sized array type in
+     GFC_TYPE_ARRAY_DATAPTR_TYPE so the gimplification of its type
+     sizes works correctly.  */
+  tree arraytype = TREE_TYPE (GFC_TYPE_ARRAY_DATAPTR_TYPE (type));
+  if (! TYPE_NAME (arraytype))
+    TYPE_NAME (arraytype) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
+                                       NULL_TREE, arraytype);
+  gfc_add_expr_to_block (pre, build1 (DECL_EXPR,
+                                     arraytype, TYPE_NAME (arraytype)));
+
   /* Fill in the array dtype.  */
   tmp = gfc_conv_descriptor_dtype (desc);
   gfc_add_modify (pre, tmp, gfc_get_dtype (TREE_TYPE (desc)));


I wonder if we can avoid allocating the TYPE_DECL by simply also
allowing TREE_TYPE as operand of a DECL_EXPR (to avoid adding
a 'TYPE_EXPR').

Richard.

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Eric Botcazou]

> The following works (for the testcase):
> 
> Index: gcc/cp/decl.c
> ===================================================================
> --- gcc/cp/decl.c       (revision 235547)
> +++ gcc/cp/decl.c       (working copy)
> @@ -10393,8 +10393,11 @@ grokdeclarator (const cp_declarator *dec
>               && (decl_context == NORMAL || decl_context == FIELD)
>               && at_function_scope_p ()
>               && variably_modified_type_p (type, NULL_TREE))
> -           /* Force evaluation of the SAVE_EXPR.  */
> -           finish_expr_stmt (TYPE_SIZE (type));
> +           {
> +             TYPE_NAME (type) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> +                                            NULL_TREE, type);
> +             add_decl_expr (TYPE_NAME (type));
> +           }
> 
>           if (declarator->kind == cdk_reference)
>             {
> 
> and I have a similar fix for the Fortran FE for one testcase I
> reduced to
> 
>   character(10), dimension (2) :: implicit_result
>   character(10), dimension (2) :: source
>   implicit_result = reallocate_hnv (LEN (source))
> contains
>   FUNCTION reallocate_hnv(LEN)
>     CHARACTER(LEN=LEN), DIMENSION(:), POINTER :: reallocate_hnv
>   END FUNCTION reallocate_hnv
> end
> 
> Index: fortran/trans-array.c
> ===================================================================
> --- fortran/trans-array.c       (revision 235547)
> +++ fortran/trans-array.c       (working copy)
> @@ -1094,6 +1094,16 @@ gfc_trans_create_temp_array (stmtblock_t
>    info->descriptor = desc;
>    size = gfc_index_one_node;
> 
> +  /* Emit a DECL_EXPR for the variable sized array type in
> +     GFC_TYPE_ARRAY_DATAPTR_TYPE so the gimplification of its type
> +     sizes works correctly.  */
> +  tree arraytype = TREE_TYPE (GFC_TYPE_ARRAY_DATAPTR_TYPE (type));
> +  if (! TYPE_NAME (arraytype))
> +    TYPE_NAME (arraytype) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> +                                       NULL_TREE, arraytype);
> +  gfc_add_expr_to_block (pre, build1 (DECL_EXPR,
> +                                     arraytype, TYPE_NAME (arraytype)));
> +
>    /* Fill in the array dtype.  */
>    tmp = gfc_conv_descriptor_dtype (desc);
>    gfc_add_modify (pre, tmp, gfc_get_dtype (TREE_TYPE (desc)));

Great.  We do exactly that in the Ada compiler (but of course the number of 
places where we need to do it is an order of magnitude larger).

> I wonder if we can avoid allocating the TYPE_DECL by simply also
> allowing TREE_TYPE as operand of a DECL_EXPR (to avoid adding
> a 'TYPE_EXPR').

I agree that DECL_EXPR + TYPE_DECL is a bit heavy, but I'm not sure that the 
benefit would be worth introducing the irregularity in the IL.

-- 
Eric Botcazou

Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener]

On Fri, 29 Apr 2016, Eric Botcazou wrote:

> > The following works (for the testcase):
> > 
> > Index: gcc/cp/decl.c
> > ===================================================================
> > --- gcc/cp/decl.c       (revision 235547)
> > +++ gcc/cp/decl.c       (working copy)
> > @@ -10393,8 +10393,11 @@ grokdeclarator (const cp_declarator *dec
> >               && (decl_context == NORMAL || decl_context == FIELD)
> >               && at_function_scope_p ()
> >               && variably_modified_type_p (type, NULL_TREE))
> > -           /* Force evaluation of the SAVE_EXPR.  */
> > -           finish_expr_stmt (TYPE_SIZE (type));
> > +           {
> > +             TYPE_NAME (type) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> > +                                            NULL_TREE, type);
> > +             add_decl_expr (TYPE_NAME (type));
> > +           }
> > 
> >           if (declarator->kind == cdk_reference)
> >             {
> > 
> > and I have a similar fix for the Fortran FE for one testcase I
> > reduced to
> > 
> >   character(10), dimension (2) :: implicit_result
> >   character(10), dimension (2) :: source
> >   implicit_result = reallocate_hnv (LEN (source))
> > contains
> >   FUNCTION reallocate_hnv(LEN)
> >     CHARACTER(LEN=LEN), DIMENSION(:), POINTER :: reallocate_hnv
> >   END FUNCTION reallocate_hnv
> > end
> > 
> > Index: fortran/trans-array.c
> > ===================================================================
> > --- fortran/trans-array.c       (revision 235547)
> > +++ fortran/trans-array.c       (working copy)
> > @@ -1094,6 +1094,16 @@ gfc_trans_create_temp_array (stmtblock_t
> >    info->descriptor = desc;
> >    size = gfc_index_one_node;
> > 
> > +  /* Emit a DECL_EXPR for the variable sized array type in
> > +     GFC_TYPE_ARRAY_DATAPTR_TYPE so the gimplification of its type
> > +     sizes works correctly.  */
> > +  tree arraytype = TREE_TYPE (GFC_TYPE_ARRAY_DATAPTR_TYPE (type));
> > +  if (! TYPE_NAME (arraytype))
> > +    TYPE_NAME (arraytype) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
> > +                                       NULL_TREE, arraytype);
> > +  gfc_add_expr_to_block (pre, build1 (DECL_EXPR,
> > +                                     arraytype, TYPE_NAME (arraytype)));
> > +
> >    /* Fill in the array dtype.  */
> >    tmp = gfc_conv_descriptor_dtype (desc);
> >    gfc_add_modify (pre, tmp, gfc_get_dtype (TREE_TYPE (desc)));
> 
> Great.  We do exactly that in the Ada compiler (but of course the number of 
> places where we need to do it is an order of magnitude larger).
> 
> > I wonder if we can avoid allocating the TYPE_DECL by simply also
> > allowing TREE_TYPE as operand of a DECL_EXPR (to avoid adding
> > a 'TYPE_EXPR').
> 
> I agree that DECL_EXPR + TYPE_DECL is a bit heavy, but I'm not sure that the 
> benefit would be worth introducing the irregularity in the IL.

Not sure either.  I'll add a helper like build_decl_expr_for_type
that does the magic (if TYPE_NAME is NULL).  That at least reduces
the amount of code duplication.

Richard.