From patchwork Mon Jun 25 12:17:24 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 167063 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id BD8E1B6F9F for ; Mon, 25 Jun 2012 22:18:06 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1341231489; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject: References:In-Reply-To:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=1vVH8pkRhtLkXDQNknbQoOTBF0M=; b=G07k2AEXyRjPFTf WtZrpIOs/JfZWGJGznBKb/BdnwbECC2/sSuSQu9XhX4mhh7cS9THGrvB8T4ff8VC eQMSGOlK6REk0OEDA3ZWbTMzF7ATJ6HZoBgZShu9C6W8i0u5IKj608/Liktfcbji kRiZe1+x9NLg4MOe7JIXFRcbAa7s= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=RmKOEqaexUPYQqcakSnO+8M0vK/qLe10iCRem5jK6Qsf2nORZOpjXm5VelvAa8 X04eff60bXTT9yC9tClzb7vOd372AQx+Dn0P0hiy32EvPAOZF9ZhrWErYl4KpP92 DZwc/dRxSleZNtTNAiTZzr4GzazgNt51WnpVgZe71EuzQ=; Received: (qmail 31449 invoked by alias); 25 Jun 2012 12:17:59 -0000 Received: (qmail 31436 invoked by uid 22791); 25 Jun 2012 12:17:56 -0000 X-SWARE-Spam-Status: No, hits=-7.6 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_THREADED, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, SPF_HELO_PASS, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 25 Jun 2012 12:17:40 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q5PCHRWY013784 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Jun 2012 08:17:28 -0400 Received: from dhcp-5-241.str.redhat.com (dhcp-5-241.str.redhat.com [10.32.5.241]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q5PCHOEh012674 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 25 Jun 2012 08:17:26 -0400 Message-ID: <4FE856D4.90804@redhat.com> Date: Mon, 25 Jun 2012 14:17:24 +0200 From: Florian Weimer User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Jason Merrill CC: gcc-patches@gcc.gnu.org, Gabriel Dos Reis Subject: Re: [C++] Reject variably modified types in operator new References: <4FC4F2AE.2090106@redhat.com> <4FC88495.60607@redhat.com> <4FC8B116.6000607@redhat.com> <4FD618B8.1020806@redhat.com> <4FE7DA1D.4020205@redhat.com> In-Reply-To: <4FE7DA1D.4020205@redhat.com> X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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? 2012-06-25 Florian Weimer * 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 * 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 a; +AvlTreeIter 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 + +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 +void * +callnew(int n) +{ + return new long[n][T::n]; +} + +template +void * +callnew_fail_1(int n) +{ + return new long[n][T::n]; // { dg-error "variable length|array size|usable in a constant" } +} + +template +void * +callnew_fail_2() +{ + return new long[T::n]; // { dg-error "size in array new" } +} + +template +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(n); + callnew_fail_2(); + callnew_fail_3(); + callnew(n); + callnew(n); +}