| Message ID | 4DBF2909.4050207@redhat.com |
|---|---|
| State | New |
| Headers | show |
> But Diego doesn't think there was any real reason to abort on trying to > copy a STATEMENT_LIST, so it seems to me that we could revert my earlier > patch for 40975 and just add support for copying STATEMENT_LIST. So > 40975-2.patch adds that support. FWIW this assertion caught an impressive number of bugs in Ada over the years related to COND_EXPR: when they are incorrectly shared, gimplifying them on one side creates a STATEMENT_LIST and stopped the copying on the other side. I'm not sure you can copy statements if they have any side-effects; this looks quite dangerous to me. Instead statement-expressions should be wrapped up in a SAVE_EXPR/TARGET_EXPR to protect them and prevent copying.
On 05/02/2011 06:23 PM, Eric Botcazou wrote: > I'm not sure you can copy statements if they have any side-effects; this looks > quite dangerous to me. Instead statement-expressions should be wrapped up in > a SAVE_EXPR/TARGET_EXPR to protect them and prevent copying. It sounds like Ada and C++ are using copy_tree_r in very different ways. The use in C++ has to do with default arguments: we parse the expression at the point of the function declaration, but whenever we want to use the expression in a function call we need to make a deep copy of the expression. In this case, we want to copy everything. How is it used in Ada? Jason
> How is it used in Ada?
The front-end doesn't use it directly, it's only used through the gimplifier
by the unsharing phase (unshare_body). We also have statement expressions.
On 05/03/2011 03:00 AM, Eric Botcazou wrote: >> How is it used in Ada? > > The front-end doesn't use it directly, it's only used through the gimplifier > by the unsharing phase (unshare_body). We also have statement expressions. In that case you wouldn't be affected by this patch; unshare_body uses mostly_copy_tree_r, which has its own special case for STATEMENT_LIST. Jason
> In that case you wouldn't be affected by this patch; unshare_body uses > mostly_copy_tree_r, which has its own special case for STATEMENT_LIST. Right, I added it precisely to support statement expressions in Ada (instead of changing copy_tree_r) since we never want to copy them in the unsharing phase. That's why I find the change to copy_tree_r questionable.
On 05/03/2011 11:52 AM, Eric Botcazou wrote: >> In that case you wouldn't be affected by this patch; unshare_body uses >> mostly_copy_tree_r, which has its own special case for STATEMENT_LIST. > > Right, I added it precisely to support statement expressions in Ada (instead > of changing copy_tree_r) since we never want to copy them in the unsharing > phase. That's why I find the change to copy_tree_r questionable. Well, let's look at the users of copy_tree_r. cp/tree.c (bot_manip): The case I want to fix. gimplify.c (mostly_copy_tree_r): Handles STATEMENT_LIST itself. stor-layout.c (copy_self_referential_tree_r): Affected by the change. Would you like me to add a gcc_unreachable() here? tree-inline.c (copy_tree_body_r): already copies STATEMENT_LIST itself (with a copy_statement_list function which I should use instead of open-coding it again). (copy_bind_expr): subroutine of copy_tree_body_r. (unsave_r, remap_gimple_op_r): Handle STATEMENT_LIST themselves. So, looks like one place that could have an undesired change in behavior. I'll go ahead and add that assert. Jason
> Well, let's look at the users of copy_tree_r. > > cp/tree.c (bot_manip): The case I want to fix. Then I'd put the fix there. The old behaviour of copy_tree_r is IMO the most sensible one and its callers should cope, as almost all already do it seems.
On 05/04/2011 04:12 AM, Eric Botcazou wrote: >> Well, let's look at the users of copy_tree_r. >> >> cp/tree.c (bot_manip): The case I want to fix. > > Then I'd put the fix there. The old behaviour of copy_tree_r is IMO the most > sensible one and its callers should cope, as almost all already do it seems. Well, I disagree. STATEMENT_LISTs are just another kind of thing you encounter in an expression; if a caller wants special handling, they can arrange for it. This is how things used to work before, but it broke when the tree-ssa merge switched from using TREE_CHAIN on statements to a separate STATEMENT_LIST. Jason
> Well, I disagree. STATEMENT_LISTs are just another kind of thing you > encounter in an expression; if a caller wants special handling, they can > arrange for it. But you're unilaterally choosing one special handling (copying) among several ones (copying, not copying, aborting) just because of one caller, for no good reason IMO. > This is how things used to work before, but it broke when the tree-ssa > merge switched from using TREE_CHAIN on statements to a separate > STATEMENT_LIST. Well, the assertion certainly wasn't put there by accident.
On 05/04/2011 06:57 PM, Eric Botcazou wrote: > But you're unilaterally choosing one special handling (copying) among several > ones (copying, not copying, aborting) just because of one caller, for no good > reason IMO. It seems pretty straightforward to me that a function named copy_tree_r should copy everything that isn't always shared (like decls). It already copies SAVE_EXPR, after all; how is copying STATEMENT_LIST going to cause trouble in a context where copying SAVE_EXPR isn't? >> This is how things used to work before, but it broke when the tree-ssa >> merge switched from using TREE_CHAIN on statements to a separate >> STATEMENT_LIST. > > Well, the assertion certainly wasn't put there by accident. No, but the rationale isn't documented. It was added by the patch that introduced STATEMENT_LIST, http://gcc.gnu.org/ml/gcc-patches/2003-11/msg00879.html but doesn't appear in the ChangeLog entry. I notice that in that patch unsave_r copied STATEMENT_LIST, but now it doesn't. rth, do you happen to remember why you put it there? Jason
On 05/05/2011 07:09 AM, Jason Merrill wrote: > No, but the rationale isn't documented. It was added by the patch > that introduced STATEMENT_LIST, > > http://gcc.gnu.org/ml/gcc-patches/2003-11/msg00879.html > > but doesn't appear in the ChangeLog entry. I notice that in that > patch unsave_r copied STATEMENT_LIST, but now it doesn't. rth, do you > happen to remember why you put it there? No, I don't recall. I suspect that I put that in there while testing in order to determine if I needed to support copying statement lists, and it turned out that I didn't. r~
> It seems pretty straightforward to me that a function named copy_tree_r > should copy everything that isn't always shared (like decls). It > already copies SAVE_EXPR, after all; how is copying STATEMENT_LIST going > to cause trouble in a context where copying SAVE_EXPR isn't? OK, this can make sense, callers should handle special nodes like SAVE_EXPR, TARGET_EXPR, STATEMENT_LIST, etc themselves. In light of this, they need to be audited and adjusted, as you did already a few days ago. So I think I can live with your 40975-3.patch in the end. Thanks for your patience.
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 2eaa1db..fff3fbf 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -564,6 +564,7 @@ build_vec_init_expr (tree target, tree init, tree nelts, elt_init = build_vec_init_elt (type, init, complain); init = build3 (VEC_INIT_EXPR, type, slot, init, nelts); + TREE_SIDE_EFFECTS (init) = true; SET_EXPR_LOCATION (init, input_location); if (cxx_dialect >= cxx0x @@ -571,7 +572,11 @@ build_vec_init_expr (tree target, tree init, tree nelts, VEC_INIT_EXPR_IS_CONSTEXPR (init) = true; VEC_INIT_EXPR_VALUE_INIT (init) = value_init; - if (slot != target) + if (slot == target) + /* If we specified what array we're initializing, make sure + we don't override that in cp_gimplify_init_expr. */ + init = cp_build_compound_expr (init, slot, complain); + else { init = build_target_expr (slot, init, complain); TARGET_EXPR_IMPLICIT_P (init) = 1; diff --git a/gcc/testsuite/g++.dg/init/new31.C b/gcc/testsuite/g++.dg/init/new31.C new file mode 100644 index 0000000..33c94aa --- /dev/null +++ b/gcc/testsuite/g++.dg/init/new31.C @@ -0,0 +1,18 @@ +// PR c++/48834 +// { dg-options -Wuninitialized } +// { dg-do run } + +struct S +{ + S ():i (0) + { + } + int i; +}; + +int +main () +{ + S *s = new S[2]; + return 0; +}
So, the problem in 48834 was that we had specified a particular target for the vec initialization, but it was getting clobbered by ending up on the rhs of an INIT_EXPR. So 48834.patch avoids that. But Diego doesn't think there was any real reason to abort on trying to copy a STATEMENT_LIST, so it seems to me that we could revert my earlier patch for 40975 and just add support for copying STATEMENT_LIST. So 40975-2.patch adds that support. I haven't attached a patch to revert my earlier 40975 patch. Some of the incidental changes from my earlier patch seem independently useful, so I'm reapplying those in vec-sfinae.patch. Tested x86_64-pc-linux-gnu, applying to trunk. Also applying 40975-2.patch to 4.4, 4.5 and 4.6; it doesn't fix the bug in 4.3 so I'm not applying it there. commit 41d391e1683e6ed7e28ad31f41732b3f4b691baa Author: Jason Merrill <jason@redhat.com> Date: Mon May 2 01:36:01 2011 -0400 PR c++/48834 * tree.c (build_vec_init_expr): Set TREE_SIDE_EFFECTS. Protect an explicit target. commit 98011319607130da27331a5f1b763ba8ce741734 Author: Jason Merrill <jason@redhat.com> Date: Mon May 2 16:02:29 2011 -0400 PR c++/40975 * tree-inline.c (copy_tree_r): Handle STATEMENT_LIST. diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 5da4a12..3777675 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -4271,14 +4271,26 @@ copy_tree_r (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED) CONSTRUCTOR_ELTS (*tp)); *tp = new_tree; } + else if (code == STATEMENT_LIST) + { + /* We used to just abort on STATEMENT_LIST, but we can run into them + with statement-expressions (c++/40975). */ + tree new_list = alloc_stmt_list (); + tree_stmt_iterator i = tsi_start (*tp); + tree_stmt_iterator j = tsi_last (new_list); + for (; !tsi_end_p (i); tsi_next (&i)) + { + tree stmt = tsi_stmt (i); + tsi_link_after (&j, stmt, TSI_CONTINUE_LINKING); + } + *tp = new_list; + } else if (TREE_CODE_CLASS (code) == tcc_type) *walk_subtrees = 0; else if (TREE_CODE_CLASS (code) == tcc_declaration) *walk_subtrees = 0; else if (TREE_CODE_CLASS (code) == tcc_constant) *walk_subtrees = 0; - else - gcc_assert (code != STATEMENT_LIST); return NULL_TREE; } commit 6f60af2d7324b81f4b524c34c321280e4874c2ee Author: Jason Merrill <jason@redhat.com> Date: Mon May 2 17:02:10 2011 -0400 * tree.c (build_vec_init_expr): Take complain parm. (build_vec_init_elt): Likewise. Free arg vector. (diagnose_non_constexpr_vec_init, build_array_copy): Adjust. * cp-tree.h (VEC_INIT_EXPR_SLOT): Use VEC_INIT_EXPR_CHECK. (VEC_INIT_EXPR_INIT): Likewise. Adjust build_vec_init_expr declaration. * init.c (perform_member_init): Adjust. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 9bad404..961581e 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -2896,8 +2896,8 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter) (arg) = next_aggr_init_expr_arg (&(iter))) /* VEC_INIT_EXPR accessors. */ -#define VEC_INIT_EXPR_SLOT(NODE) TREE_OPERAND (NODE, 0) -#define VEC_INIT_EXPR_INIT(NODE) TREE_OPERAND (NODE, 1) +#define VEC_INIT_EXPR_SLOT(NODE) TREE_OPERAND (VEC_INIT_EXPR_CHECK (NODE), 0) +#define VEC_INIT_EXPR_INIT(NODE) TREE_OPERAND (VEC_INIT_EXPR_CHECK (NODE), 1) /* Indicates that a VEC_INIT_EXPR is a potential constant expression. Only set when the current function is constexpr. */ @@ -5420,7 +5420,7 @@ extern tree get_target_expr_sfinae (tree, tsubst_flags_t); extern tree build_cplus_array_type (tree, tree); extern tree build_array_of_n_type (tree, int); extern tree build_array_copy (tree); -extern tree build_vec_init_expr (tree, tree); +extern tree build_vec_init_expr (tree, tree, tsubst_flags_t); extern void diagnose_non_constexpr_vec_init (tree); extern tree hash_tree_cons (tree, tree, tree); extern tree hash_tree_chain (tree, tree); diff --git a/gcc/cp/init.c b/gcc/cp/init.c index 50dbcc9..7a7379e 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -506,7 +506,7 @@ perform_member_init (tree member, tree init) /* mem() means value-initialization. */ if (TREE_CODE (type) == ARRAY_TYPE) { - init = build_vec_init_expr (type, init); + init = build_vec_init_expr (type, init, tf_warning_or_error); init = build2 (INIT_EXPR, type, decl, init); finish_expr_stmt (init); } @@ -543,7 +543,7 @@ perform_member_init (tree member, tree init) || same_type_ignoring_top_level_qualifiers_p (type, TREE_TYPE (init))) { - init = build_vec_init_expr (type, init); + init = build_vec_init_expr (type, init, tf_warning_or_error); init = build2 (INIT_EXPR, type, decl, init); finish_expr_stmt (init); } diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index dfd11ad..0f2f86c 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -475,7 +475,7 @@ build_cplus_new (tree type, tree init, tsubst_flags_t complain) another array to copy. */ static tree -build_vec_init_elt (tree type, tree init) +build_vec_init_elt (tree type, tree init, tsubst_flags_t complain) { tree inner_type = strip_array_types (type); VEC(tree,gc) *argvec; @@ -485,7 +485,7 @@ build_vec_init_elt (tree type, tree init) /* No interesting initialization to do. */ return integer_zero_node; else if (init == void_type_node) - return build_value_init (inner_type, tf_warning_or_error); + return build_value_init (inner_type, complain); gcc_assert (init == NULL_TREE || (same_type_ignoring_top_level_qualifiers_p @@ -499,9 +499,12 @@ build_vec_init_elt (tree type, tree init) dummy = move (dummy); VEC_quick_push (tree, argvec, dummy); } - return build_special_member_call (NULL_TREE, complete_ctor_identifier, + init = build_special_member_call (NULL_TREE, complete_ctor_identifier, &argvec, inner_type, LOOKUP_NORMAL, - tf_warning_or_error); + complain); + release_tree_vector (argvec); + + return init; } /* Return a TARGET_EXPR which expresses the initialization of an array to @@ -509,11 +512,11 @@ build_vec_init_elt (tree type, tree init) from another array of the same type. */ tree -build_vec_init_expr (tree type, tree init) +build_vec_init_expr (tree type, tree init, tsubst_flags_t complain) { tree slot; bool value_init = false; - tree elt_init = build_vec_init_elt (type, init); + tree elt_init = build_vec_init_elt (type, init, complain); if (init == void_type_node) { @@ -550,14 +553,14 @@ diagnose_non_constexpr_vec_init (tree expr) else init = VEC_INIT_EXPR_INIT (expr); - elt_init = build_vec_init_elt (type, init); + elt_init = build_vec_init_elt (type, init, tf_warning_or_error); require_potential_constant_expression (elt_init); } tree build_array_copy (tree init) { - return build_vec_init_expr (TREE_TYPE (init), init); + return build_vec_init_expr (TREE_TYPE (init), init, tf_warning_or_error); } /* Build a TARGET_EXPR using INIT to initialize a new temporary of the