diff mbox

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

Message ID 4FE856D4.90804@redhat.com
State New
Headers show

Commit Message

Florian Weimer June 25, 2012, 12:17 p.m. UTC
On 06/25/2012 05:25 AM, Jason Merrill wrote:
> 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.

Thanks for your review.

I tried this, but for this example program,

/* 1 */ void f(int n, int m)
/* 2 */ {
/* 3 */   typedef char T[n];
/* 4 */   new T[m];
/* 5 */ }

GCC reports these errors:

/tmp/t.C: In function ‘void f(int, int)’:
/tmp/t.C:4:18: error: array size in operator new must be constant
    /* 4 */   new T[m];
                     ^
/tmp/t.C:4:18: error: ‘n’ is not a constant expression

The message should point to the typedef, but instead, it references the 
line with operator new (which doesn't even contain the variable n).

For the non-VLA typedef case, it is an improvement.  But I would like to 
leave in both errors, as in the attached patch.

If you have suggestions how to improve cxx_constant_value error 
reporting, I can look into that in a separate patch.

>> + /* 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.

The new version should do that.

I've added a new test case about error reporting in templates (where we 
previously accepted ill-formed code such as new long[n][T::n] with n, 
T::n both variables).

Bootstrapped and tested on x86_64-unknown-linux-gnu, with no new 
regressions.  OK for trunk?

Comments

Jason Merrill June 25, 2012, 2:50 p.m. UTC | #1
On 06/25/2012 08:17 AM, Florian Weimer wrote:
> The message should point to the typedef, but instead, it references the
> line with operator new (which doesn't even contain the variable n).

Yep, this is another example of how we don't track/use expression 
locations well enough.

> For the non-VLA typedef case, it is an improvement. But I would like to
> leave in both errors, as in the attached patch.

OK.

> If you have suggestions how to improve cxx_constant_value error
> reporting, I can look into that in a separate patch.

One way would be to pass around a location_t loc in the cxx_eval_* 
functions, starting with input_location, but then at the top of 
cxx_eval_constant_expression have

if (EXPR_HAS_LOCATION (t))
   loc = EXPR_LOCATION (t);

and use error_at (loc in all error messages.

And perhaps also change array_type_nelts_top to give the PLUS_EXPR (if 
any) the location of the array typedef (if any).

> Bootstrapped and tested on x86_64-unknown-linux-gnu, with no new
> regressions. OK for trunk?

Yes.

Jason
diff mbox

Patch

2012-06-25  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-25  Florian Weimer  <fweimer@redhat.com>

	* g++.dg/init/new35.C: New.
	* g++.dg/init/new36.C: New.
	* g++.dg/init/new37.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.

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 419c13f..5a81643 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2175,6 +2175,7 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
   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 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
     }
   else if (TREE_CODE (type) == ARRAY_TYPE)
     {
+      /* Transforms new (T[N]) to new T[N].  The former is a GNU
+	 extension for variable N.  (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,10 +2225,46 @@  build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts,
   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");
+	      cxx_constant_value(inner_nelts);
+	    }
+	  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)))
+    {
+      if (complain & tf_warning_or_error)
+	pedwarn(EXPR_LOC_OR_HERE (outer_nelts), OPT_Wvla,
+		"ISO C++ does not support variable-length array types");
+      else
+	return error_mark_node;
+    }
 
   if (TREE_CODE (elt_type) == VOID_TYPE)
     {
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 6bc6877c..46f1401 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6883,41 +6883,34 @@  cp_parser_direct_new_declarator (cp_parser* parser)
   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);
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/regress/debug-debug7.C b/gcc/testsuite/g++.dg/cpp0x/regress/debug-debug7.C
index ea8f1eb..d3f14f4 100644
--- a/gcc/testsuite/g++.dg/cpp0x/regress/debug-debug7.C
+++ b/gcc/testsuite/g++.dg/cpp0x/regress/debug-debug7.C
@@ -7,8 +7,8 @@  int
 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|usable in a constant" }
 
   x[2][1] = 7;
 
diff --git a/gcc/testsuite/g++.dg/ext/vla5.C b/gcc/testsuite/g++.dg/ext/vla5.C
index 021d484..2457e34 100644
--- a/gcc/testsuite/g++.dg/ext/vla5.C
+++ b/gcc/testsuite/g++.dg/ext/vla5.C
@@ -6,5 +6,5 @@ 
 void
 test (int a)
 {
-  new (char[a]);
+  new (char[a]); // { dg-warning "variable-length array" }
 }
diff --git a/gcc/testsuite/g++.dg/ext/vla8.C b/gcc/testsuite/g++.dg/ext/vla8.C
index 7b7428d..1c6000f 100644
--- a/gcc/testsuite/g++.dg/ext/vla8.C
+++ b/gcc/testsuite/g++.dg/ext/vla8.C
@@ -8,8 +8,8 @@  struct AvlTreeIter
 
   AvlTreeIter()
   {
-    new (void* [Num()]);
+    new (void* [Num()]); // { dg-warning "variable-length array" }
   }
 };
 
-AvlTreeIter<int> a;
+AvlTreeIter<int> a; // { dg-message "from here" }
diff --git a/gcc/testsuite/g++.dg/init/new35.C b/gcc/testsuite/g++.dg/init/new35.C
new file mode 100644
index 0000000..c5f79aa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new35.C
@@ -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|not a constant" }
+  new A[0]; // { dg-error "must be constant|not a constant" }
+  new A[5]; // { dg-error "must be constant|not a constant" }
+  new (A[0]); // { dg-error "must be constant|not a constant" }
+  new (A[5]); // { dg-error "must be constant|not a constant" }
+}
diff --git a/gcc/testsuite/g++.dg/init/new36.C b/gcc/testsuite/g++.dg/init/new36.C
new file mode 100644
index 0000000..c9b7af2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new36.C
@@ -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);
+}
diff --git a/gcc/testsuite/g++.dg/init/new37.C b/gcc/testsuite/g++.dg/init/new37.C
new file mode 100644
index 0000000..82ca18b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/new37.C
@@ -0,0 +1,63 @@ 
+// { dg-do compile }
+
+void
+nonconst(int n)
+{
+  new (long[n][n]); // { dg-error "variable length|array size|not a constant" }
+  new long[n][n]; // { dg-error "variable length|array size|not a constant" }
+}
+
+template <typename T>
+void *
+callnew(int n)
+{
+  return new long[n][T::n];
+}
+
+template <typename T>
+void *
+callnew_fail_1(int n)
+{
+  return new long[n][T::n]; // { dg-error "variable length|array size|usable in a constant" }
+}
+
+template <typename T>
+void *
+callnew_fail_2()
+{
+  return new long[T::n]; // { dg-error "size in array new" }
+}
+
+template <typename T>
+void *
+callnew_fail_3()
+{
+  return new T[2][T::n]; // { dg-error "size of array has non-integral type" }
+}
+
+struct T1 {
+  static int n;
+};
+
+struct T2 {
+  static const double n = 2; // { dg-error "non-integral type" }
+};
+
+struct T3 {
+  static const int n = 2;
+};
+
+struct T4 {
+  enum { n = 3 };
+};
+
+void
+test_callnew(int n)
+{
+  new long[0.2]; // { dg-error "integral or enumeration type" }
+  callnew_fail_1<T1>(n);
+  callnew_fail_2<T2>();
+  callnew_fail_3<T2>();
+  callnew<T3>(n);
+  callnew<T4>(n);
+}