diff mbox series

c++, v2: Fix ICE with nsdmi [PR99705]

Message ID 20210325105023.GE1179226@tucnak
State New
Headers show
Series c++, v2: Fix ICE with nsdmi [PR99705] | expand

Commit Message

Jakub Jelinek March 25, 2021, 10:50 a.m. UTC 
On Tue, Mar 23, 2021 at 04:51:52PM -0400, Jason Merrill via Gcc-patches wrote:
> > +  if (TREE_CODE (*tp) == DECL_EXPR
> > +      && VAR_P (DECL_EXPR_DECL (*tp))
> > +      && DECL_ARTIFICIAL (DECL_EXPR_DECL (*tp))
> > +      && !TREE_STATIC (DECL_EXPR_DECL (*tp))
> > +      && DECL_CONTEXT (DECL_EXPR_DECL (*tp)) == NULL_TREE
> 
> I might drop the DECL_CONTEXT check; I'd think any embedded temporaries that
> happen to have it set would also need this treatment.
> 
> > +      && !splay_tree_lookup (target_remap,
> > +			     (splay_tree_key) DECL_EXPR_DECL (*tp)))
> > +    {
> > +      tree t = create_temporary_var (TREE_TYPE (DECL_EXPR_DECL (*tp)));
> 
> You don't need to copy DECL_INITIAL here?

Ok, I had another look at both of these issues.
The reason for the nsdmi12.C failures with the removed DECL_CONTEXT ... === NULL_TREE
check was that apparently it is not just walk_tree_1 BIND_EXPR handling that
walks DECL_INITIAL:
    case BIND_EXPR:
      {
        tree decl;
        for (decl = BIND_EXPR_VARS (*tp); decl; decl = DECL_CHAIN (decl))
          {
            /* Walk the DECL_INITIAL and DECL_SIZE.  We don't want to walk
               into declarations that are just mentioned, rather than
               declared; they don't really belong to this part of the tree.
               And, we can see cycles: the initializer for a declaration
               can refer to the declaration itself.  */
            WALK_SUBTREE (DECL_INITIAL (decl));
            WALK_SUBTREE (DECL_SIZE (decl));
            WALK_SUBTREE (DECL_SIZE_UNIT (decl));
          }
        WALK_SUBTREE_TAIL (BIND_EXPR_BODY (*tp));
      }
but it is also cp_walk_subtrees DECL_EXPR handling:
    case DECL_EXPR:
      /* User variables should be mentioned in BIND_EXPR_VARS
         and their initializers and sizes walked when walking
         the containing BIND_EXPR.  Compiler temporaries are
         handled here.  And also normal variables in templates,
         since do_poplevel doesn't build a BIND_EXPR then.  */
      if (VAR_P (TREE_OPERAND (*tp, 0))
          && (processing_template_decl
              || (DECL_ARTIFICIAL (TREE_OPERAND (*tp, 0))
                  && !TREE_STATIC (TREE_OPERAND (*tp, 0)))))
        {
          tree decl = TREE_OPERAND (*tp, 0);
          WALK_SUBTREE (DECL_INITIAL (decl));
          WALK_SUBTREE (DECL_SIZE (decl));
          WALK_SUBTREE (DECL_SIZE_UNIT (decl));
        }
      break;
(though e.g. cp/optimize.c (clone_body) uses plain walk_tree and therefore
won't walk the DECL_INITIAL of the temporaries, so why it is better for them
to be in BIND_EXPRs).
Anyway, what happens in the nsdmi12.C case is that in the newly added
bot_manip code BIND_EXPR handling we decide to clone a temporary with
DECL_INITIAL, replace it in the new BIND_EXPR and by walking the subtrees
we duplicate its DECL_INITIAL.  But then when walking with bot_replace, due
to the above mentioned cp_walk_subtrees code we first bot_replace the
DECL_INITIAL of the original temporary rather than its copy when we
encounter DECL_EXPR for it, and only afterwards when walking the subtrees
of the DECL_EXPR replace the temporary with its clone, which means that
both original temporary and copy's DECL_INITIAL will now contain other
temporary's copies and of course that is fatal.
I have fixed that by the bot_replace hunk, that the code changes DECL_EXPR
operand if needed already when walking the DECL_EXPR.  That means that
cp_walk_subtrees called right after that will already see the new decl
and DTRT.
And, given the cp_walk_subtrees DECL_INITIAL handling, I'm copying
DECL_INITIAL also for temporaries that have just DECL_EXPR and aren't
mentioned in BIND_EXPR, because there is some chance it could work.

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

2021-03-25  Jakub Jelinek  <jakub@redhat.com>

	PR c++/99705
	* tree.c (bot_manip): Remap artificial automatic temporaries mentioned
	in DECL_EXPR or in BIND_EXPR_VARS.
	(bot_replace): For DECL_EXPR, replace DECL_EXPR_DECL if needed before
	walking subtrees.

	* g++.dg/cpp0x/new5.C: New test.



	Jakub

Comments

Jason Merrill March 25, 2021, 8:20 p.m. UTC | #1 
On 3/25/21 6:50 AM, Jakub Jelinek wrote:
> On Tue, Mar 23, 2021 at 04:51:52PM -0400, Jason Merrill via Gcc-patches wrote:
>>> +  if (TREE_CODE (*tp) == DECL_EXPR
>>> +      && VAR_P (DECL_EXPR_DECL (*tp))
>>> +      && DECL_ARTIFICIAL (DECL_EXPR_DECL (*tp))
>>> +      && !TREE_STATIC (DECL_EXPR_DECL (*tp))
>>> +      && DECL_CONTEXT (DECL_EXPR_DECL (*tp)) == NULL_TREE
>>
>> I might drop the DECL_CONTEXT check; I'd think any embedded temporaries that
>> happen to have it set would also need this treatment.
>>
>>> +      && !splay_tree_lookup (target_remap,
>>> +			     (splay_tree_key) DECL_EXPR_DECL (*tp)))
>>> +    {
>>> +      tree t = create_temporary_var (TREE_TYPE (DECL_EXPR_DECL (*tp)));
>>
>> You don't need to copy DECL_INITIAL here?
> 
> Ok, I had another look at both of these issues.
> The reason for the nsdmi12.C failures with the removed DECL_CONTEXT ... === NULL_TREE
> check was that apparently it is not just walk_tree_1 BIND_EXPR handling that
> walks DECL_INITIAL:
>      case BIND_EXPR:
>        {
>          tree decl;
>          for (decl = BIND_EXPR_VARS (*tp); decl; decl = DECL_CHAIN (decl))
>            {
>              /* Walk the DECL_INITIAL and DECL_SIZE.  We don't want to walk
>                 into declarations that are just mentioned, rather than
>                 declared; they don't really belong to this part of the tree.
>                 And, we can see cycles: the initializer for a declaration
>                 can refer to the declaration itself.  */
>              WALK_SUBTREE (DECL_INITIAL (decl));
>              WALK_SUBTREE (DECL_SIZE (decl));
>              WALK_SUBTREE (DECL_SIZE_UNIT (decl));
>            }
>          WALK_SUBTREE_TAIL (BIND_EXPR_BODY (*tp));
>        }
> but it is also cp_walk_subtrees DECL_EXPR handling:
>      case DECL_EXPR:
>        /* User variables should be mentioned in BIND_EXPR_VARS
>           and their initializers and sizes walked when walking
>           the containing BIND_EXPR.  Compiler temporaries are
>           handled here.  And also normal variables in templates,
>           since do_poplevel doesn't build a BIND_EXPR then.  */
>        if (VAR_P (TREE_OPERAND (*tp, 0))
>            && (processing_template_decl
>                || (DECL_ARTIFICIAL (TREE_OPERAND (*tp, 0))
>                    && !TREE_STATIC (TREE_OPERAND (*tp, 0)))))
>          {

What if we WALK_SUBTREE (DECL_EXPR_DECL (*tp)) here, instead of the 
bot_replace hunk?  OK either way.

>            tree decl = TREE_OPERAND (*tp, 0);
>            WALK_SUBTREE (DECL_INITIAL (decl));
>            WALK_SUBTREE (DECL_SIZE (decl));
>            WALK_SUBTREE (DECL_SIZE_UNIT (decl));
>          }
>        break;
> (though e.g. cp/optimize.c (clone_body) uses plain walk_tree and therefore
> won't walk the DECL_INITIAL of the temporaries, so why it is better for them
> to be in BIND_EXPRs).
> Anyway, what happens in the nsdmi12.C case is that in the newly added
> bot_manip code BIND_EXPR handling we decide to clone a temporary with
> DECL_INITIAL, replace it in the new BIND_EXPR and by walking the subtrees
> we duplicate its DECL_INITIAL.  But then when walking with bot_replace, due
> to the above mentioned cp_walk_subtrees code we first bot_replace the
> DECL_INITIAL of the original temporary rather than its copy when we
> encounter DECL_EXPR for it, and only afterwards when walking the subtrees
> of the DECL_EXPR replace the temporary with its clone, which means that
> both original temporary and copy's DECL_INITIAL will now contain other
> temporary's copies and of course that is fatal.
> I have fixed that by the bot_replace hunk, that the code changes DECL_EXPR
> operand if needed already when walking the DECL_EXPR.  That means that
> cp_walk_subtrees called right after that will already see the new decl
> and DTRT.
> And, given the cp_walk_subtrees DECL_INITIAL handling, I'm copying
> DECL_INITIAL also for temporaries that have just DECL_EXPR and aren't
> mentioned in BIND_EXPR, because there is some chance it could work.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2021-03-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/99705
> 	* tree.c (bot_manip): Remap artificial automatic temporaries mentioned
> 	in DECL_EXPR or in BIND_EXPR_VARS.
> 	(bot_replace): For DECL_EXPR, replace DECL_EXPR_DECL if needed before
> 	walking subtrees.
> 
> 	* g++.dg/cpp0x/new5.C: New test.
> 
> --- gcc/cp/tree.c.jj	2021-03-23 10:20:42.823717414 +0100
> +++ gcc/cp/tree.c	2021-03-24 20:06:40.385176204 +0100
> @@ -3128,6 +3128,35 @@ bot_manip (tree* tp, int* walk_subtrees,
>   	}
>         return NULL_TREE;
>       }
> +  if (TREE_CODE (*tp) == DECL_EXPR
> +      && VAR_P (DECL_EXPR_DECL (*tp))
> +      && DECL_ARTIFICIAL (DECL_EXPR_DECL (*tp))
> +      && !TREE_STATIC (DECL_EXPR_DECL (*tp))
> +      && !splay_tree_lookup (target_remap,
> +			     (splay_tree_key) DECL_EXPR_DECL (*tp)))
> +    {
> +      tree t = create_temporary_var (TREE_TYPE (DECL_EXPR_DECL (*tp)));
> +      DECL_INITIAL (t) = DECL_INITIAL (DECL_EXPR_DECL (*tp));
> +      splay_tree_insert (target_remap, (splay_tree_key) DECL_EXPR_DECL (*tp),
> +			 (splay_tree_value) t);
> +    }
> +  if (TREE_CODE (*tp) == BIND_EXPR && BIND_EXPR_VARS (*tp))
> +    {
> +      copy_tree_r (tp, walk_subtrees, NULL);
> +      for (tree *p = &BIND_EXPR_VARS (*tp); *p; p = &DECL_CHAIN (*p))
> +	{
> +	  gcc_assert (VAR_P (*p) && DECL_ARTIFICIAL (*p) && !TREE_STATIC (*p));
> +	  tree t = create_temporary_var (TREE_TYPE (*p));
> +	  DECL_INITIAL (t) = DECL_INITIAL (*p);
> +	  DECL_CHAIN (t) = DECL_CHAIN (*p);
> +	  splay_tree_insert (target_remap, (splay_tree_key) *p,
> +			     (splay_tree_value) t);
> +	  *p = t;
> +	}
> +      if (data.clear_location && EXPR_HAS_LOCATION (*tp))
> +	SET_EXPR_LOCATION (*tp, input_location);
> +      return NULL_TREE;
> +    }
>   
>     /* Make a copy of this node.  */
>     t = copy_tree_r (tp, walk_subtrees, NULL);
> @@ -3178,6 +3207,18 @@ bot_replace (tree* t, int* /*walk_subtre
>         *t = build_base_path (PLUS_EXPR, TREE_OPERAND (*t, 0), binfo, true,
>   			    tf_warning_or_error);
>       }
> +  else if (TREE_CODE (*t) == DECL_EXPR && VAR_P (DECL_EXPR_DECL (*t)))
> +    {
> +      /* For DECL_EXPRs where the variable needs to be remapped,
> +	 remap it before recursing on subtrees, because cp_walk_subtrees
> +	 might walk DECL_INITIAL etc. and we must not clobber DECL_INITIAL
> +	 etc. of the original decl.  */
> +      splay_tree_node n
> +	= splay_tree_lookup (target_remap,
> +			     (splay_tree_key) DECL_EXPR_DECL (*t));
> +      if (n)
> +	DECL_EXPR_DECL (*t) = (tree) n->value;
> +    }
>   
>     return NULL_TREE;
>   }
> --- gcc/testsuite/g++.dg/cpp0x/new5.C.jj	2021-03-24 17:44:02.629963259 +0100
> +++ gcc/testsuite/g++.dg/cpp0x/new5.C	2021-03-24 17:44:02.629963259 +0100
> @@ -0,0 +1,21 @@
> +// PR c++/99705
> +// { dg-do compile { target c++11 } }
> +
> +template <typename T>
> +struct C
> +{
> +  C () { f (); }
> +  ~C () {}
> +  static void f () {}
> +};
> +
> +struct X
> +{
> +  X ();
> +  int n = 10;
> +  C<int> *p = new C<int>[n];
> +};
> +
> +X::X ()
> +{
> +}
> 
> 
> 	Jakub
>
Jakub Jelinek March 25, 2021, 8:33 p.m. UTC | #2 
On Thu, Mar 25, 2021 at 04:20:54PM -0400, Jason Merrill wrote:
> > but it is also cp_walk_subtrees DECL_EXPR handling:
> >      case DECL_EXPR:
> >        /* User variables should be mentioned in BIND_EXPR_VARS
> >           and their initializers and sizes walked when walking
> >           the containing BIND_EXPR.  Compiler temporaries are
> >           handled here.  And also normal variables in templates,
> >           since do_poplevel doesn't build a BIND_EXPR then.  */
> >        if (VAR_P (TREE_OPERAND (*tp, 0))
> >            && (processing_template_decl
> >                || (DECL_ARTIFICIAL (TREE_OPERAND (*tp, 0))
> >                    && !TREE_STATIC (TREE_OPERAND (*tp, 0)))))
> >          {
> 
> What if we WALK_SUBTREE (DECL_EXPR_DECL (*tp)) here, instead of the
> bot_replace hunk?  OK either way.

The above hunk is cp_walk_subtrees, we shouldn't change that IMO.
We could do it in bot_manip instead of bot_replace by doing roughly:
  if (TREE_CODE (*tp) == DECL_EXPR
      && VAR_P (DECL_EXPR_DECL (*tp))
      && DECL_ARTIFICIAL (DECL_EXPR_DECL (*tp))
      && !TREE_STATIC (DECL_EXPR_DECL (*tp)))
    {
      tree t;
      splay_tree_node n
	= splay_tree_lookup (target_remap,
			     (splay_tree_key) DECL_EXPR_DECL (*tp));
      if (n)
	t = (tree) n->value;
      else
	{
	  t = create_temporary_var (TREE_TYPE (DECL_EXPR_DECL (*tp)));
	  DECL_INITIAL (t) = DECL_INITIAL (DECL_EXPR_DECL (*tp));
	  splay_tree_insert (target_remap,
			     (splay_tree_key) DECL_EXPR_DECL (*tp),
			     (splay_tree_value) t);
	}
      copy_tree_r (tp, walk_subtrees, NULL);
      DECL_EXPR_DECL (*tp) = t;
      if (data.clear_location && EXPR_HAS_LOCATION (*tp))
	SET_EXPR_LOCATION (*tp, input_location);
      return NULL_TREE;
    }
plus the current BIND_EXPR handling afterwards.  I.e. update
DECL_EXPR_DECL of the DECL_EXPR right away in bot_manip rather than
waiting until bot_replace with that.

If you prefer that, I can test it tonight.

	Jakub
Jason Merrill March 25, 2021, 8:36 p.m. UTC | #3 
On 3/25/21 4:33 PM, Jakub Jelinek wrote:
> On Thu, Mar 25, 2021 at 04:20:54PM -0400, Jason Merrill wrote:
>>> but it is also cp_walk_subtrees DECL_EXPR handling:
>>>       case DECL_EXPR:
>>>         /* User variables should be mentioned in BIND_EXPR_VARS
>>>            and their initializers and sizes walked when walking
>>>            the containing BIND_EXPR.  Compiler temporaries are
>>>            handled here.  And also normal variables in templates,
>>>            since do_poplevel doesn't build a BIND_EXPR then.  */
>>>         if (VAR_P (TREE_OPERAND (*tp, 0))
>>>             && (processing_template_decl
>>>                 || (DECL_ARTIFICIAL (TREE_OPERAND (*tp, 0))
>>>                     && !TREE_STATIC (TREE_OPERAND (*tp, 0)))))
>>>           {
>>
>> What if we WALK_SUBTREE (DECL_EXPR_DECL (*tp)) here, instead of the
>> bot_replace hunk?  OK either way.
> 
> The above hunk is cp_walk_subtrees, we shouldn't change that IMO.
> We could do it in bot_manip instead of bot_replace by doing roughly:
>    if (TREE_CODE (*tp) == DECL_EXPR
>        && VAR_P (DECL_EXPR_DECL (*tp))
>        && DECL_ARTIFICIAL (DECL_EXPR_DECL (*tp))
>        && !TREE_STATIC (DECL_EXPR_DECL (*tp)))
>      {
>        tree t;
>        splay_tree_node n
> 	= splay_tree_lookup (target_remap,
> 			     (splay_tree_key) DECL_EXPR_DECL (*tp));
>        if (n)
> 	t = (tree) n->value;
>        else
> 	{
> 	  t = create_temporary_var (TREE_TYPE (DECL_EXPR_DECL (*tp)));
> 	  DECL_INITIAL (t) = DECL_INITIAL (DECL_EXPR_DECL (*tp));
> 	  splay_tree_insert (target_remap,
> 			     (splay_tree_key) DECL_EXPR_DECL (*tp),
> 			     (splay_tree_value) t);
> 	}
>        copy_tree_r (tp, walk_subtrees, NULL);
>        DECL_EXPR_DECL (*tp) = t;
>        if (data.clear_location && EXPR_HAS_LOCATION (*tp))
> 	SET_EXPR_LOCATION (*tp, input_location);
>        return NULL_TREE;
>      }
> plus the current BIND_EXPR handling afterwards.  I.e. update
> DECL_EXPR_DECL of the DECL_EXPR right away in bot_manip rather than
> waiting until bot_replace with that.
> 
> If you prefer that, I can test it tonight.

Sure, let's go with this version.

Jason
diff mbox series

Patch

--- gcc/cp/tree.c.jj	2021-03-23 10:20:42.823717414 +0100
+++ gcc/cp/tree.c	2021-03-24 20:06:40.385176204 +0100
@@ -3128,6 +3128,35 @@  bot_manip (tree* tp, int* walk_subtrees,
 	}
       return NULL_TREE;
     }
+  if (TREE_CODE (*tp) == DECL_EXPR
+      && VAR_P (DECL_EXPR_DECL (*tp))
+      && DECL_ARTIFICIAL (DECL_EXPR_DECL (*tp))
+      && !TREE_STATIC (DECL_EXPR_DECL (*tp))
+      && !splay_tree_lookup (target_remap,
+			     (splay_tree_key) DECL_EXPR_DECL (*tp)))
+    {
+      tree t = create_temporary_var (TREE_TYPE (DECL_EXPR_DECL (*tp)));
+      DECL_INITIAL (t) = DECL_INITIAL (DECL_EXPR_DECL (*tp));
+      splay_tree_insert (target_remap, (splay_tree_key) DECL_EXPR_DECL (*tp),
+			 (splay_tree_value) t);
+    }
+  if (TREE_CODE (*tp) == BIND_EXPR && BIND_EXPR_VARS (*tp))
+    {
+      copy_tree_r (tp, walk_subtrees, NULL);
+      for (tree *p = &BIND_EXPR_VARS (*tp); *p; p = &DECL_CHAIN (*p))
+	{
+	  gcc_assert (VAR_P (*p) && DECL_ARTIFICIAL (*p) && !TREE_STATIC (*p));
+	  tree t = create_temporary_var (TREE_TYPE (*p));
+	  DECL_INITIAL (t) = DECL_INITIAL (*p);
+	  DECL_CHAIN (t) = DECL_CHAIN (*p);
+	  splay_tree_insert (target_remap, (splay_tree_key) *p,
+			     (splay_tree_value) t);
+	  *p = t;
+	}
+      if (data.clear_location && EXPR_HAS_LOCATION (*tp))
+	SET_EXPR_LOCATION (*tp, input_location);
+      return NULL_TREE;
+    }
 
   /* Make a copy of this node.  */
   t = copy_tree_r (tp, walk_subtrees, NULL);
@@ -3178,6 +3207,18 @@  bot_replace (tree* t, int* /*walk_subtre
       *t = build_base_path (PLUS_EXPR, TREE_OPERAND (*t, 0), binfo, true,
 			    tf_warning_or_error);
     }
+  else if (TREE_CODE (*t) == DECL_EXPR && VAR_P (DECL_EXPR_DECL (*t)))
+    {
+      /* For DECL_EXPRs where the variable needs to be remapped,
+	 remap it before recursing on subtrees, because cp_walk_subtrees
+	 might walk DECL_INITIAL etc. and we must not clobber DECL_INITIAL
+	 etc. of the original decl.  */
+      splay_tree_node n
+	= splay_tree_lookup (target_remap,
+			     (splay_tree_key) DECL_EXPR_DECL (*t));
+      if (n)
+	DECL_EXPR_DECL (*t) = (tree) n->value;
+    }
 
   return NULL_TREE;
 }
--- gcc/testsuite/g++.dg/cpp0x/new5.C.jj	2021-03-24 17:44:02.629963259 +0100
+++ gcc/testsuite/g++.dg/cpp0x/new5.C	2021-03-24 17:44:02.629963259 +0100
@@ -0,0 +1,21 @@ 
+// PR c++/99705
+// { dg-do compile { target c++11 } }
+
+template <typename T>
+struct C
+{
+  C () { f (); }
+  ~C () {}
+  static void f () {}
+};
+
+struct X
+{
+  X ();
+  int n = 10;
+  C<int> *p = new C<int>[n];
+};
+
+X::X ()
+{
+}