Patchwork C++ PATCHes relating to c++/48834, c++/40975 (array new)

login
register
mail settings
Submitter Jason Merrill
Date May 2, 2011, 9:58 p.m.
Message ID <4DBF2909.4050207@redhat.com>
Download mbox | patch
Permalink /patch/93720/
State New
Headers show

Comments

Jason Merrill - May 2, 2011, 9:58 p.m.
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
Eric Botcazou - May 2, 2011, 10:23 p.m.
> 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.
Jason Merrill - May 3, 2011, 5:05 a.m.
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
Eric Botcazou - May 3, 2011, 7 a.m.
> 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.
Jason Merrill - May 3, 2011, 3:27 p.m.
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
Eric Botcazou - May 3, 2011, 3:52 p.m.
> 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.
Jason Merrill - May 3, 2011, 5:28 p.m.
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
Eric Botcazou - May 4, 2011, 8:12 a.m.
> 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.
Jason Merrill - May 4, 2011, 3:45 p.m.
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
Eric Botcazou - May 4, 2011, 10:57 p.m.
> 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.
Jason Merrill - May 5, 2011, 2:09 p.m.
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
Richard Henderson - May 5, 2011, 4:35 p.m.
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~
Eric Botcazou - May 7, 2011, 7:41 p.m.
> 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.

Patch

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;
+}