* [PATCH] c++: Fix ICE with nsdmi [PR99705]
@ 2021-03-23 9:57 Jakub Jelinek
2021-03-23 20:51 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2021-03-23 9:57 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]
Hi!
When adding P0784R7 constexpr new support, we still didn't have
P1331R2 implemented and so I had to change also build_vec_delete_1
- instead of having uninitialized tbase temporary later initialized
by MODIFY_EXPR I've set the DECL_INITIAL for it - because otherwise
it would be rejected during constexpr evaluation which didn't like
uninitialized vars. Unfortunately, that change broke the following
testcase.
The problem is that these temporaries (not just tbase but tbase was
the only one with an initializer) are created during NSDMI parsing
and current_function_decl is NULL at that point. Later when we
clone body of constructors, auto_var_in_fn_p is false for those
(as they have NULL DECL_CONTEXT) and so they aren't duplicated,
and what is worse, the DECL_INITIAL isn't duplicated either nor processed,
and during expansion we ICE because the code from DECL_INITIAL of that
var refers to the abstract constructor's PARM_DECL (this) rather than
the actual constructor's one.
So, either we can just revert those build_vec_delete_1 changes (as done
in the second patch - in attachment), or, as the first patch does, we can
copy the temporaries during bot_manip like we copy the temporaries of
TARGET_EXPRs. To me that looks like a better fix because e.g. if
break_out_of_target_exprs is called for the same NSDMI multiple times,
sharing the temporaries looks just wrong to me. If the temporaries
are declared as BIND_EXPR_VARS of some BIND_EXPR (which is the case
of the tbase variable built by build_vec_delete_1 and is the only way
how the DECL_INITIAL can be walked by *walk_tree*), then we need to
copy it also in the BIND_EXPR BIND_EXPR_VARS chain, other temporaries
(those that don't need DECL_INITIAL) often have just DECL_EXPR and no
corresponding BIND_EXPR.
Note, ({ }) are rejected in nsdmis, so all we run into are temporaries
the FE creates artificially.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Or do you prefer the patch in attachment (or something else)?
2021-03-23 Jakub Jelinek <jakub@redhat.com>
PR c++/99705
* tree.c (bot_manip): Remap artificial automatic temporaries with
NULL DECL_CONTEXT mentioned in DECL_EXPR or in BIND_EXPR_VARS.
* g++.dg/cpp0x/new5.C: New test.
--- gcc/cp/tree.c.jj 2021-03-18 09:49:22.112712307 +0100
+++ gcc/cp/tree.c 2021-03-23 00:08:35.901724895 +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))
+ && DECL_CONTEXT (DECL_EXPR_DECL (*tp)) == NULL_TREE
+ && !splay_tree_lookup (target_remap,
+ (splay_tree_key) DECL_EXPR_DECL (*tp)))
+ {
+ tree t = create_temporary_var (TREE_TYPE (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);
--- gcc/testsuite/g++.dg/cpp0x/new5.C.jj 2021-03-22 14:08:29.168782588 +0100
+++ gcc/testsuite/g++.dg/cpp0x/new5.C 2021-03-22 14:07:35.253378808 +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
[-- Attachment #2: S875a --]
[-- Type: text/plain, Size: 1380 bytes --]
2021-03-22 Jakub Jelinek <jakub@redhat.com>
PR c++/99705
* init.c (build_vec_delete_1): Revert to using a MODIFY_EXPR for tbase
initialization instead of DECL_INITIAL.
* g++.dg/cpp0x/new5.C: New test.
--- gcc/cp/init.c.jj 2021-03-22 13:25:30.153349516 +0100
+++ gcc/cp/init.c 2021-03-22 13:35:34.206653080 +0100
@@ -3903,10 +3903,13 @@ build_vec_delete_1 (location_t loc, tree
fold_convert (sizetype, maxindex));
tbase = create_temporary_var (ptype);
- DECL_INITIAL (tbase)
+ tbase_init
= fold_build_pointer_plus_loc (loc, fold_convert (ptype, base),
virtual_size);
- tbase_init = build_stmt (loc, DECL_EXPR, tbase);
+ tbase_init = cp_build_modify_expr (loc, tbase, NOP_EXPR, tbase_init,
+ complain);
+ tbase_init = build_compound_expr (loc, build_stmt (loc, DECL_EXPR, tbase),
+ tbase_init);
controller = build3 (BIND_EXPR, void_type_node, tbase, NULL_TREE, NULL_TREE);
TREE_SIDE_EFFECTS (controller) = 1;
--- gcc/testsuite/g++.dg/cpp0x/new5.C.jj 2021-03-22 14:08:29.168782588 +0100
+++ gcc/testsuite/g++.dg/cpp0x/new5.C 2021-03-22 14:07:35.253378808 +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 ()
+{
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: Fix ICE with nsdmi [PR99705]
2021-03-23 9:57 [PATCH] c++: Fix ICE with nsdmi [PR99705] Jakub Jelinek
@ 2021-03-23 20:51 ` Jason Merrill
2021-03-23 20:59 ` Jakub Jelinek
2021-03-25 10:50 ` [PATCH] c++, v2: " Jakub Jelinek
0 siblings, 2 replies; 7+ messages in thread
From: Jason Merrill @ 2021-03-23 20:51 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 3/23/21 5:57 AM, Jakub Jelinek wrote:
> Hi!
>
> When adding P0784R7 constexpr new support, we still didn't have
> P1331R2 implemented and so I had to change also build_vec_delete_1
> - instead of having uninitialized tbase temporary later initialized
> by MODIFY_EXPR I've set the DECL_INITIAL for it - because otherwise
> it would be rejected during constexpr evaluation which didn't like
> uninitialized vars. Unfortunately, that change broke the following
> testcase.
> The problem is that these temporaries (not just tbase but tbase was
> the only one with an initializer) are created during NSDMI parsing
> and current_function_decl is NULL at that point. Later when we
> clone body of constructors, auto_var_in_fn_p is false for those
> (as they have NULL DECL_CONTEXT) and so they aren't duplicated,
> and what is worse, the DECL_INITIAL isn't duplicated either nor processed,
> and during expansion we ICE because the code from DECL_INITIAL of that
> var refers to the abstract constructor's PARM_DECL (this) rather than
> the actual constructor's one.
>
> So, either we can just revert those build_vec_delete_1 changes (as done
> in the second patch - in attachment), or, as the first patch does, we can
> copy the temporaries during bot_manip like we copy the temporaries of
> TARGET_EXPRs. To me that looks like a better fix because e.g. if
> break_out_of_target_exprs is called for the same NSDMI multiple times,
> sharing the temporaries looks just wrong to me. If the temporaries
> are declared as BIND_EXPR_VARS of some BIND_EXPR (which is the case
> of the tbase variable built by build_vec_delete_1 and is the only way
> how the DECL_INITIAL can be walked by *walk_tree*), then we need to
> copy it also in the BIND_EXPR BIND_EXPR_VARS chain, other temporaries
> (those that don't need DECL_INITIAL) often have just DECL_EXPR and no
> corresponding BIND_EXPR.
> Note, ({ }) are rejected in nsdmis, so all we run into are temporaries
> the FE creates artificially.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> Or do you prefer the patch in attachment (or something else)?
>
> 2021-03-23 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/99705
> * tree.c (bot_manip): Remap artificial automatic temporaries with
> NULL DECL_CONTEXT mentioned in DECL_EXPR or in BIND_EXPR_VARS.
>
> * g++.dg/cpp0x/new5.C: New test.
>
> --- gcc/cp/tree.c.jj 2021-03-18 09:49:22.112712307 +0100
> +++ gcc/cp/tree.c 2021-03-23 00:08:35.901724895 +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))
> + && 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?
> + 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);
> --- gcc/testsuite/g++.dg/cpp0x/new5.C.jj 2021-03-22 14:08:29.168782588 +0100
> +++ gcc/testsuite/g++.dg/cpp0x/new5.C 2021-03-22 14:07:35.253378808 +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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: Fix ICE with nsdmi [PR99705]
2021-03-23 20:51 ` Jason Merrill
@ 2021-03-23 20:59 ` Jakub Jelinek
2021-03-25 10:50 ` [PATCH] c++, v2: " Jakub Jelinek
1 sibling, 0 replies; 7+ messages in thread
From: Jakub Jelinek @ 2021-03-23 20:59 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
On Tue, Mar 23, 2021 at 04:51:52PM -0400, Jason Merrill wrote:
> > --- gcc/cp/tree.c.jj 2021-03-18 09:49:22.112712307 +0100
> > +++ gcc/cp/tree.c 2021-03-23 00:08:35.901724895 +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))
> > + && 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.
I had that first that way but it broke g++.dg/cpp0x/nsdmi12.C test.
I can have another look why tomorrow.
> > + && !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?
No, because all the temporaries that have DECL_INITIAL need to be manually
put into BIND_EXPR_VARS of some BIND_EXPR, otherwise they wouldn't be
handled properly elsewhere (e.g. walk_tree only walks DECL_INITIAL on
BIND_EXPR). I can add there
gcc_assert (DECL_INITIAL (DECL_EXPR_DECL (*tp)) == NULL_TREE);
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] c++, v2: Fix ICE with nsdmi [PR99705]
2021-03-23 20:51 ` Jason Merrill
2021-03-23 20:59 ` Jakub Jelinek
@ 2021-03-25 10:50 ` Jakub Jelinek
2021-03-25 20:20 ` Jason Merrill
1 sibling, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2021-03-25 10:50 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
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.
--- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++, v2: Fix ICE with nsdmi [PR99705]
2021-03-25 10:50 ` [PATCH] c++, v2: " Jakub Jelinek
@ 2021-03-25 20:20 ` Jason Merrill
2021-03-25 20:33 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2021-03-25 20:20 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++, v2: Fix ICE with nsdmi [PR99705]
2021-03-25 20:20 ` Jason Merrill
@ 2021-03-25 20:33 ` Jakub Jelinek
2021-03-25 20:36 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2021-03-25 20:33 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++, v2: Fix ICE with nsdmi [PR99705]
2021-03-25 20:33 ` Jakub Jelinek
@ 2021-03-25 20:36 ` Jason Merrill
0 siblings, 0 replies; 7+ messages in thread
From: Jason Merrill @ 2021-03-25 20:36 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-25 20:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 9:57 [PATCH] c++: Fix ICE with nsdmi [PR99705] Jakub Jelinek
2021-03-23 20:51 ` Jason Merrill
2021-03-23 20:59 ` Jakub Jelinek
2021-03-25 10:50 ` [PATCH] c++, v2: " Jakub Jelinek
2021-03-25 20:20 ` Jason Merrill
2021-03-25 20:33 ` Jakub Jelinek
2021-03-25 20:36 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).