[RFA] tree-inline: Fix VLA handling [PR95552]

Message ID 20200606041749.5300-1-jason@redhat.com
State New
Headers show
Series
  • [RFA] tree-inline: Fix VLA handling [PR95552]
Related show

Commit Message

Jose E. Marchesi via Gcc-patches June 6, 2020, 4:17 a.m.
The problem in this testcase comes from cloning the constructor into
complete and base variants.  When we clone the body the first time,
walk_tree_1 calls copy_tree_body_r on the type of the artificial TYPE_DECL
we made for the VLA type without calling it on the decl itself, so we
overwrite the type of the TYPE_DECL without copying the decl first.

This has been broken since we started inserting a TYPE_DECL for anonymous
VLAs in r7-457.

This patch fixes walk_tree_1 to call the function on the TYPE_DECL, as we do
for other decls of a DECL_EXPR.

Tested x86_64-pc-linux-gnu.  OK for trunk?  Release branches?

gcc/ChangeLog:

	PR c++/95552
	* tree.c (walk_tree_1): Call func on the TYPE_DECL of a DECL_EXPR.

gcc/testsuite/ChangeLog:

	PR c++/95552
	* g++.dg/ext/vla23.C: New test.
---
 gcc/testsuite/g++.dg/ext/vla23.C | 14 ++++++++++++++
 gcc/tree.c                       |  5 +++++
 2 files changed, 19 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ext/vla23.C


base-commit: 5bc13e5217f687f5d08a7022b4c6081befc54402
-- 
2.18.1

Comments

Jose E. Marchesi via Gcc-patches June 6, 2020, 6:15 a.m. | #1
On June 6, 2020 6:17:49 AM GMT+02:00, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>The problem in this testcase comes from cloning the constructor into

>complete and base variants.  When we clone the body the first time,

>walk_tree_1 calls copy_tree_body_r on the type of the artificial

>TYPE_DECL

>we made for the VLA type without calling it on the decl itself, so we

>overwrite the type of the TYPE_DECL without copying the decl first.

>

>This has been broken since we started inserting a TYPE_DECL for

>anonymous

>VLAs in r7-457.

>

>This patch fixes walk_tree_1 to call the function on the TYPE_DECL, as

>we do

>for other decls of a DECL_EXPR.

>

>Tested x86_64-pc-linux-gnu.  OK for trunk?  Release branches?


Looks reasonable. Please make sure to bootstrap and test with Ada enabled. Eric may also have comments here. 

In any case please wait a while before backporting. 

Thanks, 
Richard. 

>gcc/ChangeLog:

>

>	PR c++/95552

>	* tree.c (walk_tree_1): Call func on the TYPE_DECL of a DECL_EXPR.

>

>gcc/testsuite/ChangeLog:

>

>	PR c++/95552

>	* g++.dg/ext/vla23.C: New test.

>---

> gcc/testsuite/g++.dg/ext/vla23.C | 14 ++++++++++++++

> gcc/tree.c                       |  5 +++++

> 2 files changed, 19 insertions(+)

> create mode 100644 gcc/testsuite/g++.dg/ext/vla23.C

>

>diff --git a/gcc/testsuite/g++.dg/ext/vla23.C

>b/gcc/testsuite/g++.dg/ext/vla23.C

>new file mode 100644

>index 00000000000..317a824b2f3

>--- /dev/null

>+++ b/gcc/testsuite/g++.dg/ext/vla23.C

>@@ -0,0 +1,14 @@

>+// PR c++/95552

>+// Test for VLA and cloned constructor.

>+// { dg-additional-options -Wno-vla }

>+// { dg-require-effective-target alloca }

>+

>+struct VB { };

>+struct ViewDom: virtual VB

>+{

>+  ViewDom(int i) { char (*a)[i]; }

>+};

>+void element( )

>+{

>+  ViewDom a(2);

>+}

>diff --git a/gcc/tree.c b/gcc/tree.c

>index 7197b4720ce..c8e9680b06c 100644

>--- a/gcc/tree.c

>+++ b/gcc/tree.c

>@@ -12212,6 +12212,11 @@ walk_tree_1 (tree *tp, walk_tree_fn func, void

>*data,

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

>       if (TREE_CODE (DECL_EXPR_DECL (*tp)) == TYPE_DECL)

> 	{

>+	  /* Call the function for the decl so e.g. inlining can remap it. 

>*/

>+	  result = (*func) (&DECL_EXPR_DECL (*tp), &walk_subtrees, data);

>+	  if (result || !walk_subtrees)

>+	    return result;

>+

> 	  tree *type_p = &TREE_TYPE (DECL_EXPR_DECL (*tp));

> 	  if (TREE_CODE (*type_p) == ERROR_MARK)

> 	    return NULL_TREE;

>

>base-commit: 5bc13e5217f687f5d08a7022b4c6081befc54402
Eric Botcazou June 6, 2020, 8:56 a.m. | #2
> This patch fixes walk_tree_1 to call the function on the TYPE_DECL, as we do

> for other decls of a DECL_EXPR.


Where is that done exactly?  The only case handled by DECL_EXPR is TYPE_DECL.

The correct thing to do is clearly implied by the first line in your patch.

-- 
Eric Botcazou
Jose E. Marchesi via Gcc-patches June 8, 2020, 2:08 a.m. | #3
On 6/6/20 4:56 AM, Eric Botcazou wrote:
>> This patch fixes walk_tree_1 to call the function on the TYPE_DECL, as we do

>> for other decls of a DECL_EXPR.

> 

> Where is that done exactly?  The only case handled by DECL_EXPR is TYPE_DECL.


The only case handled specially is TYPE_DECL; other DECL_EXPRs fall 
through to the default case, where we WALK_SUBTREE over all the operands 
of the expression, which for DECL_EXPR is the decl.

I suppose it might be clearer to walk the decl for all DECL_EXPRs, then 
have the special TYPE_DECL handling, then break instead of falling 
through, though that doesn't get the tail recursion of the default case. 
  Thoughts?

Jason
Eric Botcazou June 8, 2020, 9:46 a.m. | #4
> The only case handled specially is TYPE_DECL; other DECL_EXPRs fall

> through to the default case, where we WALK_SUBTREE over all the operands

> of the expression, which for DECL_EXPR is the decl.


It seems hard to believe that the inliner relies on this to copy DECLs though, 
see the calls to remap_decl[s].  And note that remap_decl has had a special 
handling for DECL_ORIGINAL_TYPE of TYPE_DECLs since 2003.  Are you sure the 
problem is not that the TYPE_DECL is not attached to the enclosing BIND_EXPR?

-- 
Eric Botcazou
Jose E. Marchesi via Gcc-patches June 8, 2020, 8:19 p.m. | #5
On 6/8/20 5:46 AM, Eric Botcazou wrote:
>> The only case handled specially is TYPE_DECL; other DECL_EXPRs fall

>> through to the default case, where we WALK_SUBTREE over all the operands

>> of the expression, which for DECL_EXPR is the decl.

> 

> It seems hard to believe that the inliner relies on this to copy DECLs though,


I agree!  I imagine the actual inliner is unaffected because that 
happens in GIMPLE now, so this only affects FE cloning.  I don't know if 
any other front ends use a walk_tree callback like copy_tree_body_r that 
would replace a type in the same way.

> see the calls to remap_decl[s].


Which ones?

> And note that remap_decl has had a special

> handling for DECL_ORIGINAL_TYPE of TYPE_DECLs since 2003.

Yes, but the problem is that remap_decl isn't getting called.

> Are you sure the

> problem is not that the TYPE_DECL is not attached to the enclosing BIND_EXPR?


Attaching it to the BIND_EXPR doesn't help walk_tree_1 do the right 
thing with the DECL_EXPR.

Jason
Jose E. Marchesi via Gcc-patches June 8, 2020, 8:51 p.m. | #6
On 6/8/20 4:19 PM, Jason Merrill wrote:
> On 6/8/20 5:46 AM, Eric Botcazou wrote:

>>> The only case handled specially is TYPE_DECL; other DECL_EXPRs fall

>>> through to the default case, where we WALK_SUBTREE over all the operands

>>> of the expression, which for DECL_EXPR is the decl.

>>

>> It seems hard to believe that the inliner relies on this to copy DECLs 

>> though,

> 

> I agree!  I imagine the actual inliner is unaffected because that 

> happens in GIMPLE now, so this only affects FE cloning.  I don't know if 

> any other front ends use a walk_tree callback like copy_tree_body_r that 

> would replace a type in the same way.

> 

>> see the calls to remap_decl[s].

> 

> Which ones?

> 

>> And note that remap_decl has had a special

>> handling for DECL_ORIGINAL_TYPE of TYPE_DECLs since 2003.

> Yes, but the problem is that remap_decl isn't getting called.

> 

>> Are you sure the

>> problem is not that the TYPE_DECL is not attached to the enclosing 

>> BIND_EXPR?

> 

> Attaching it to the BIND_EXPR doesn't help walk_tree_1 do the right 

> thing with the DECL_EXPR.


We could also work around this in copy_tree_body_r as follows, but I 
imagined that other tree walking functions might also be affected by the 
missing call for the TYPE_DECL of a DECL_EXPR.

Jason
commit 4320af50f15c11f6d6a7ddcf0fac51e752c0906f
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Jun 5 22:51:18 2020 -0400

    copy-body

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 3160ca3f88a..2899bcf13e3 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -1434,6 +1434,12 @@ copy_tree_body_r (tree *tp, int *walk_subtrees, void *data)
 	  TREE_SET_BLOCK (*tp, new_block);
 	}
 
+      if (TREE_CODE (*tp) == DECL_EXPR
+	  && TREE_CODE (DECL_EXPR_DECL (*tp)) == TYPE_DECL)
+	/* walk_tree_1 walks the type of a type DECL_EXPR but not the decl, so
+	   we clobber the type of the source decl unless we remap it now.  */
+	DECL_EXPR_DECL (*tp) = remap_decl (DECL_EXPR_DECL (*tp), id);
+
       if (TREE_CODE (*tp) != OMP_CLAUSE)
 	TREE_TYPE (*tp) = remap_type (TREE_TYPE (*tp), id);
Eric Botcazou June 9, 2020, 12:41 p.m. | #7
> Yes, but the problem is that remap_decl isn't getting called.


Right, I can get it to be called by adding a pushdecl to grokdeclarator...

> Attaching it to the BIND_EXPR doesn't help walk_tree_1 do the right

> thing with the DECL_EXPR.


... but, indeed, this still ICEs.  So the key is that the DECL_EXPR_DECL of 
the copied DECL_EXPR points to the remapped TYPE_DECL before the type is 
copied?  If so, then your original patch is probably the way to go, but the 
comment would need to be slightly adjusted.

In Ada, where we attach the TYPE_DECL to the BIND_EXPR, this will mean that
remap_decl is invoked 3 times per TYPE_DECL: first twice from copy_bind_expr 
and then once again for the DECL_EXPR.  But probably no big deal in the end.

-- 
Eric Botcazou
Jose E. Marchesi via Gcc-patches June 9, 2020, 4:08 p.m. | #8
On 6/9/20 8:41 AM, Eric Botcazou wrote:
>> Yes, but the problem is that remap_decl isn't getting called.

> 

> Right, I can get it to be called by adding a pushdecl to grokdeclarator...

> 

>> Attaching it to the BIND_EXPR doesn't help walk_tree_1 do the right

>> thing with the DECL_EXPR.

> 

> ... but, indeed, this still ICEs.  So the key is that the DECL_EXPR_DECL of

> the copied DECL_EXPR points to the remapped TYPE_DECL before the type is

> copied?  If so, then your original patch is probably the way to go, but the

> comment would need to be slightly adjusted.


Like this?

> In Ada, where we attach the TYPE_DECL to the BIND_EXPR, this will mean that

> remap_decl is invoked 3 times per TYPE_DECL: first twice from copy_bind_expr

> and then once again for the DECL_EXPR.  But probably no big deal in the end.


Yes, we want to remap every occurrence of the decl so they all get replaced.

Jason
commit 7b7c1b07dc32cb3cb6dc9b97d516a7240c825cf9
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Jun 5 16:36:27 2020 -0400

    tree-inline: Fix VLA handling [PR95552]
    
    The problem in this testcase comes from cloning the constructor into
    complete and base variants.  When we clone the body the first time,
    walk_tree_1 calls copy_tree_body_r on the type of the artificial TYPE_DECL
    we made for the VLA type without calling it on the decl itself, so we
    overwrite the type of the TYPE_DECL without copying the decl first.
    
    This has been broken since we started inserting a TYPE_DECL for anonymous
    VLAs in r7-457.
    
    This patch fixes walk_tree_1 to call the function on the TYPE_DECL, as we do
    for other decls of a DECL_EXPR.
    
    gcc/ChangeLog:
    
            PR c++/95552
            * tree.c (walk_tree_1): Call func on the TYPE_DECL of a DECL_EXPR.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/95552
            * g++.dg/ext/vla23.C: New test.

diff --git a/gcc/testsuite/g++.dg/ext/vla23.C b/gcc/testsuite/g++.dg/ext/vla23.C
new file mode 100644
index 00000000000..317a824b2f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/vla23.C
@@ -0,0 +1,14 @@
+// PR c++/95552
+// Test for VLA and cloned constructor.
+// { dg-additional-options -Wno-vla }
+// { dg-require-effective-target alloca }
+
+struct VB { };
+struct ViewDom: virtual VB
+{
+  ViewDom(int i) { char (*a)[i]; }
+};
+void element( )
+{
+  ViewDom a(2);
+}
diff --git a/gcc/tree.c b/gcc/tree.c
index 7197b4720ce..805f669a945 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -12212,6 +12212,12 @@ walk_tree_1 (tree *tp, walk_tree_fn func, void *data,
 	 Note that DECLs get walked as part of processing the BIND_EXPR.  */
       if (TREE_CODE (DECL_EXPR_DECL (*tp)) == TYPE_DECL)
 	{
+	  /* Call the function for the decl so e.g. copy_tree_body_r can
+	     replace it with the remapped one.  */
+	  result = (*func) (&DECL_EXPR_DECL (*tp), &walk_subtrees, data);
+	  if (result || !walk_subtrees)
+	    return result;
+
 	  tree *type_p = &TREE_TYPE (DECL_EXPR_DECL (*tp));
 	  if (TREE_CODE (*type_p) == ERROR_MARK)
 	    return NULL_TREE;
Eric Botcazou June 9, 2020, 5:12 p.m. | #9
> Like this?


Fine with me, thanks!

-- 
Eric Botcazou

Patch

diff --git a/gcc/testsuite/g++.dg/ext/vla23.C b/gcc/testsuite/g++.dg/ext/vla23.C
new file mode 100644
index 00000000000..317a824b2f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/vla23.C
@@ -0,0 +1,14 @@ 
+// PR c++/95552
+// Test for VLA and cloned constructor.
+// { dg-additional-options -Wno-vla }
+// { dg-require-effective-target alloca }
+
+struct VB { };
+struct ViewDom: virtual VB
+{
+  ViewDom(int i) { char (*a)[i]; }
+};
+void element( )
+{
+  ViewDom a(2);
+}
diff --git a/gcc/tree.c b/gcc/tree.c
index 7197b4720ce..c8e9680b06c 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -12212,6 +12212,11 @@  walk_tree_1 (tree *tp, walk_tree_fn func, void *data,
 	 Note that DECLs get walked as part of processing the BIND_EXPR.  */
       if (TREE_CODE (DECL_EXPR_DECL (*tp)) == TYPE_DECL)
 	{
+	  /* Call the function for the decl so e.g. inlining can remap it.  */
+	  result = (*func) (&DECL_EXPR_DECL (*tp), &walk_subtrees, data);
+	  if (result || !walk_subtrees)
+	    return result;
+
 	  tree *type_p = &TREE_TYPE (DECL_EXPR_DECL (*tp));
 	  if (TREE_CODE (*type_p) == ERROR_MARK)
 	    return NULL_TREE;