Patchwork [C++] Reject variably modified types in operator new

login
register
mail settings
Submitter Florian Weimer
Date June 11, 2012, 4:11 p.m.
Message ID <4FD618B8.1020806@redhat.com>
Download mbox | patch
Permalink /patch/164225/
State New
Headers show

Comments

Florian Weimer - June 11, 2012, 4:11 p.m.
On 06/01/2012 02:09 PM, Florian Weimer wrote:
> On 06/01/2012 11:00 AM, Florian Weimer wrote:
>
>> I'll try to warn about this case and make the transformation to the
>> proper operator new[] call.
>
> Here's the version. I've added a warning for the ill-formed code.
>
> The only remaining glitch is in g++.dg/cpp0x/regress/debug-debug7.C,
> specifically (b is not a constant):
>
> int (*x)[b] = new int[a][b]; // { dg-error "not usable" }
>
> The new warning I've added fires on this line, too. How can I check for
> the pending error and suppress the warning?

As discussed in the other thread with Jason and Gaby, I have changed 
cp/parser.c to accept non-constant values and produce the error in 
build_new_1.  This is arguably the right place because this check must 
be performed after template instantiation.  (I will submit a follow-up 
patch for the remaining type check in the parser once this is accepted.)

testsuite/g++.dg/cpp0x/regress/debug-debug7.C remains problematic.  The 
diagnostic at parse time was definitely better, but I see no way to keep 
that.  I have reworded the diagnostics because the code no longer has 
access to the original syntax, so it cannot tell whether the operator 
new call involved a type-id in parens.

Bootstrapped and tested on x86_64-linux-gnu, with no new regressions.
Jason Merrill - June 25, 2012, 3:25 a.m.
On 06/11/2012 12:11 PM, Florian Weimer wrote:
> +      tree inner_nelts_cst = maybe_constant_value (inner_nelts);
> +      if (!TREE_CONSTANT (inner_nelts_cst))
> +	{
> +	  if (complain & tf_error)
> +	    error_at (EXPR_LOC_OR_HERE (inner_nelts),
> +		      "array size in operator new must be constant");

Please use cxx_constant_value to give a more specific error about what 
is non-constant.

> +  /* Warn if we performed the (T[N]) to T[N] transformation and N is
> +     variable.  */
> +  if (outer_nelts_from_type
> +      && !TREE_CONSTANT (maybe_constant_value (outer_nelts))
> +      && (complain & tf_warning_or_error))
> +    pedwarn(EXPR_LOC_OR_HERE (outer_nelts), OPT_Wvla,
> +	    "ISO C++ does not support variable-length array types");

Here, if we aren't complaining we should return error_mark_node; we 
always need to act pedantic in SFINAE context.

Jason

Patch

2012-06-11  Florian Weimer  <fweimer@redhat.com>

	* init.c (build_new_1): Warn about (T[N]) for variable N, and
	reject T[M][N].

	* parser.c (cp_parser_direct_new_declarator): Accept non-constant
	expressions.  Handled now in build_new_1.

2012-06-11  Florian Weimer  <fweimer@redhat.com>

	* g++.dg/init/new35.C: New.
	* g++.dg/init/new36.C: New.
	* g++.dg/ext/vla5.C: New warning.
	* g++.dg/ext/vla8.C: New warning.
	* g++.dg/cpp0x/regress/debug-debug7.C: Update diagnostics.

Index: cp/init.c
===================================================================
--- cp/init.c	(revision 188384)
+++ cp/init.c	(working copy)
@@ -2175,6 +2175,7 @@ 
   tree pointer_type;
   tree non_const_pointer_type;
   tree outer_nelts = NULL_TREE;
+  bool outer_nelts_from_type = false;
   tree alloc_call, alloc_expr;
   /* The address returned by the call to "operator new".  This node is
      a VAR_DECL and is therefore reusable.  */
@@ -2209,10 +2210,14 @@ 
     }
   else if (TREE_CODE (type) == ARRAY_TYPE)
     {
+      /* Transforms new (T[N]) to new T[N].  The former is a GNU
+	 extension.  (This also covers new T where T is a VLA
+	 typedef.)  */
       array_p = true;
       nelts = array_type_nelts_top (type);
       outer_nelts = nelts;
       type = TREE_TYPE (type);
+      outer_nelts_from_type = true;
     }
 
   /* If our base type is an array, then make sure we know how many elements
@@ -2220,11 +2225,40 @@ 
   for (elt_type = type;
        TREE_CODE (elt_type) == ARRAY_TYPE;
        elt_type = TREE_TYPE (elt_type))
-    nelts = cp_build_binary_op (input_location,
-				MULT_EXPR, nelts,
-				array_type_nelts_top (elt_type),
-				complain);
+    {
+      tree inner_nelts = array_type_nelts_top (elt_type);
+      tree inner_nelts_cst = maybe_constant_value (inner_nelts);
+      if (!TREE_CONSTANT (inner_nelts_cst))
+	{
+	  if (complain & tf_error)
+	    error_at (EXPR_LOC_OR_HERE (inner_nelts),
+		      "array size in operator new must be constant");
+	  nelts = error_mark_node;
+	}
+      if (nelts != error_mark_node)
+	nelts = cp_build_binary_op (input_location,
+				    MULT_EXPR, nelts,
+				    inner_nelts_cst,
+				    complain);
+    }
 
+  if (variably_modified_type_p (elt_type, NULL_TREE) && (complain & tf_error))
+    {
+      error ("variably modified type not allowed in operator new");
+      return error_mark_node;
+    }
+
+  if (nelts == error_mark_node)
+    return error_mark_node;
+
+  /* Warn if we performed the (T[N]) to T[N] transformation and N is
+     variable.  */
+  if (outer_nelts_from_type
+      && !TREE_CONSTANT (maybe_constant_value (outer_nelts))
+      && (complain & tf_warning_or_error))
+    pedwarn(EXPR_LOC_OR_HERE (outer_nelts), OPT_Wvla,
+	    "ISO C++ does not support variable-length array types");
+
   if (TREE_CODE (elt_type) == VOID_TYPE)
     {
       if (complain & tf_error)
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 188384)
+++ cp/parser.c	(working copy)
@@ -6845,41 +6845,34 @@ 
   while (true)
     {
       tree expression;
+      cp_token *token;
 
       /* Look for the opening `['.  */
       cp_parser_require (parser, CPP_OPEN_SQUARE, RT_OPEN_SQUARE);
-      /* The first expression is not required to be constant.  */
-      if (!declarator)
+
+      token = cp_lexer_peek_token (parser->lexer);
+      expression = cp_parser_expression (parser, /*cast_p=*/false, NULL);
+      /* The standard requires that the expression have integral
+	 type.  DR 74 adds enumeration types.  We believe that the
+	 real intent is that these expressions be handled like the
+	 expression in a `switch' condition, which also allows
+	 classes with a single conversion to integral or
+	 enumeration type.  */
+      if (!processing_template_decl)
 	{
-	  cp_token *token = cp_lexer_peek_token (parser->lexer);
-	  expression = cp_parser_expression (parser, /*cast_p=*/false, NULL);
-	  /* The standard requires that the expression have integral
-	     type.  DR 74 adds enumeration types.  We believe that the
-	     real intent is that these expressions be handled like the
-	     expression in a `switch' condition, which also allows
-	     classes with a single conversion to integral or
-	     enumeration type.  */
-	  if (!processing_template_decl)
+	  expression
+	    = build_expr_type_conversion (WANT_INT | WANT_ENUM,
+					  expression,
+					  /*complain=*/true);
+	  if (!expression)
 	    {
-	      expression
-		= build_expr_type_conversion (WANT_INT | WANT_ENUM,
-					      expression,
-					      /*complain=*/true);
-	      if (!expression)
-		{
-		  error_at (token->location,
-			    "expression in new-declarator must have integral "
-			    "or enumeration type");
-		  expression = error_mark_node;
-		}
+	      error_at (token->location,
+			"expression in new-declarator must have integral "
+			"or enumeration type");
+	      expression = error_mark_node;
 	    }
 	}
-      /* But all the other expressions must be.  */
-      else
-	expression
-	  = cp_parser_constant_expression (parser,
-					   /*allow_non_constant=*/false,
-					   NULL);
+
       /* Look for the closing `]'.  */
       cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);
 
Index: testsuite/g++.dg/cpp0x/regress/debug-debug7.C
===================================================================
--- testsuite/g++.dg/cpp0x/regress/debug-debug7.C	(revision 188384)
+++ testsuite/g++.dg/cpp0x/regress/debug-debug7.C	(working copy)
@@ -7,8 +7,8 @@ 
 main() {
 
   int a = 4;
-  int b = 5;			// { dg-message "not const" }
-  int (*x)[b] = new int[a][b];	// { dg-error "not usable" }
+  int b = 5;
+  int (*x)[b] = new int[a][b]; // { dg-error "array size.*must be constant" }
 
   x[2][1] = 7;
 
Index: testsuite/g++.dg/ext/vla5.C
===================================================================
--- testsuite/g++.dg/ext/vla5.C	(revision 188384)
+++ testsuite/g++.dg/ext/vla5.C	(working copy)
@@ -6,5 +6,5 @@ 
 void
 test (int a)
 {
-  new (char[a]);
+  new (char[a]); // { dg-warning "variable-length array" }
 }
Index: testsuite/g++.dg/ext/vla8.C
===================================================================
--- testsuite/g++.dg/ext/vla8.C	(revision 188384)
+++ testsuite/g++.dg/ext/vla8.C	(working copy)
@@ -8,8 +8,8 @@ 
 
   AvlTreeIter()
   {
-    new (void* [Num()]);
+    new (void* [Num()]); // { dg-warning "variable-length array" }
   }
 };
 
-AvlTreeIter<int> a;
+AvlTreeIter<int> a; // { dg-message "from here" }
Index: testsuite/g++.dg/init/new35.C
===================================================================
--- testsuite/g++.dg/init/new35.C	(revision 0)
+++ testsuite/g++.dg/init/new35.C	(revision 0)
@@ -0,0 +1,13 @@ 
+// { dg-do compile }
+// { dg-options "" }
+
+int
+main (int argc, char **argv)
+{
+  typedef char A[argc];
+  new A; // { dg-warning "variable-length array types" }
+  new A[0]; // { dg-error "must be constant" }
+  new A[5]; // { dg-error "must be constant" }
+  new (A[0]); // { dg-error "must be constant" }
+  new (A[5]); // { dg-error "must be constant" }
+}
Index: testsuite/g++.dg/init/new36.C
===================================================================
--- testsuite/g++.dg/init/new36.C	(revision 0)
+++ testsuite/g++.dg/init/new36.C	(revision 0)
@@ -0,0 +1,153 @@ 
+// Testcase for invocation of constructors/destructors in operator new[].
+// { dg-do run }
+
+#include <stdlib.h>
+
+struct E {
+  virtual ~E() { }
+};
+
+struct S {
+  S();
+  ~S();
+};
+
+static int count;
+static int max;
+static int throwAfter = -1;
+static S *pS;
+
+S::S()
+{
+  if (throwAfter >= 0 && count >= throwAfter)
+    throw E();
+  if (pS)
+    {
+      ++pS;
+      if (this != pS)
+	abort();
+    }
+  else
+    pS = this;
+  ++count;
+  max = count;
+}
+
+S::~S()
+{
+  if (count > 1)
+    {
+      if (this != pS)
+	abort();
+      --pS;
+    }
+  else
+    pS = 0;
+  --count;
+}
+
+void __attribute__((noinline)) doit(int n)
+{
+  {
+    S *s = new S[n];
+    if (count != n)
+      abort();
+    if (pS != s + n - 1)
+      abort();
+    delete [] s;
+    if (count != 0)
+      abort();
+  }
+  throwAfter = 2;
+  max = 0;
+  try
+    {
+      new S[n];
+      abort();
+    }
+  catch (E)
+    {
+      if (max != 2)
+	abort();
+    }
+  throwAfter = -1;
+}
+
+int main()
+{
+  {
+    S s;
+    if (count != 1)
+      abort();
+    if (pS != &s)
+      abort();
+  }
+  if (count != 0)
+    abort();
+  {
+    S *s = new S;
+    if (count != 1)
+      abort();
+    if (pS != s)
+      abort();
+    delete s;
+    if (count != 0)
+      abort();
+  }
+  {
+    S *s = new S[1];
+    if (count != 1)
+      abort();
+    if (pS != s)
+      abort();
+    delete [] s;
+    if (count != 0)
+      abort();
+  }
+  {
+    S *s = new S[5];
+    if (count != 5)
+      abort();
+    if (pS != s + 4)
+      abort();
+    delete [] s;
+    if (count != 0)
+      abort();
+  }
+  typedef S A[5];
+  {
+    S *s = new A;
+    if (count != 5)
+      abort();
+    if (pS != s + 4)
+      abort();
+    delete [] s;
+    if (count != 0)
+      abort();
+  }
+  throwAfter = 2;
+  max = 0;
+  try
+    {
+      new S[5];
+      abort();
+    }
+  catch (E)
+    {
+      if (max != 2)
+	abort();
+    }
+  max = 0;
+  try
+    {
+      new A;
+      abort();
+    }
+  catch (E)
+    {
+      if (max != 2)
+	abort();
+    }
+  throwAfter = -1;
+  doit(5);
+}