Hi!
Thanks for the review, let me start with the unrelated bugfixes then and
deal with the rest later.
On Tue, Oct 01, 2019 at 05:56:00PM -0400, Jason Merrill wrote:
> > @@ -3905,7 +4033,7 @@ cxx_eval_store_expression (const constex
> > bool no_zero_init = true;
> > releasing_vec ctors;
> > - while (!refs->is_empty())
> > + while (!refs->is_empty ())
> > {
> > if (*valp == NULL_TREE)
> > {
> > @@ -4046,7 +4174,9 @@ cxx_eval_store_expression (const constex
> > if (const_object_being_modified)
> > {
> > bool fail = false;
> > - if (!CLASS_TYPE_P (TREE_TYPE (const_object_being_modified)))
> > + tree const_objtype
> > + = strip_array_types (TREE_TYPE (const_object_being_modified));
> > + if (!CLASS_TYPE_P (const_objtype))
>
> This looks like an unrelated bugfix; you might commit it (and the others)
> separately if that's convenient.
>
> > @@ -4907,14 +5043,21 @@ cxx_eval_constant_expression (const cons
> > break;
> > case CLEANUP_STMT:
> > - r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval,
> > + {
> > + tree initial_jump_target = jump_target ? *jump_target : NULL_TREE;
> > + r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval,
> > + non_constant_p, overflow_p,
> > + jump_target);
> > + if (!CLEANUP_EH_ONLY (t) && !*non_constant_p)
> > + /* Also evaluate the cleanup. If we weren't skipping at the
> > + start of the CLEANUP_BODY, change jump_target temporarily
> > + to &initial_jump_target, so that even a return or break or
> > + continue in the body doesn't skip the cleanup. */
>
> This also looks like an unrelated bugfix.
Both are only partially related, I think they can go in separately, but at
least for the first one I haven't managed to come up with a testcase where
it would matter and nothing in e.g. check-c++-all comes there with ARRAY_TYPE,
is it ok for trunk without a testcase (first attached patch)?
As for the second bugfix, I think it should be accompanied with the
potential_constant_expression_1 change, and there I've actually managed to
come up with a testcase where it matters, though it is using a GCC extension
(generally, CLEANUP_STMTs won't appear in constexpr functions that often
because of the requirement that variables in them have literal type and
literal types have trivial destructors, so really no cleanups in that case).
The testcase where it makes a difference is:
constexpr void
cleanup (int *x)
{
if (x)
asm ("");
}
constexpr void
cleanup2 (int *x)
{
}
constexpr bool
foo ()
{
int a __attribute__((cleanup (cleanup))) = 1;
return true;
}
constexpr bool
bar ()
{
int a __attribute__((cleanup (cleanup2))) = 1;
return true;
}
constexpr auto x = foo ();
constexpr auto y = bar ();
With vanilla trunk, one gets a weird message:
test.C:27:24: error: ‘constexpr bool foo()’ called in a constant expression
27 | constexpr auto x = foo ();
| ~~~~^~
test.C:28:24: error: ‘constexpr bool bar()’ called in a constant expression
28 | constexpr auto y = bar ();
| ~~~~^~
That is because we call potential_constant_expression_1 on the foo (or
bar) body, see CLEANUP_STMT in there and punt. With just the
potential_constant_expression_1 change to handle CLEANUP_STMT, we actually
accept it, which is wrong.
Finally, with the whole patch attached below, we reject foo () call and
accept bar ():
test.C:27:24: in ‘constexpr’ expansion of ‘foo()’
test.C:27:25: in ‘constexpr’ expansion of ‘cleanup((& a))’
test.C:5:5: error: inline assembly is not a constant expression
5 | asm ("");
| ^~~
test.C:5:5: note: only unevaluated inline assembly is allowed in a ‘constexpr’
function in C++2a
Is the testcase ok in the second patch? Are those patches ok for trunk?
Jakub
2019-10-02 Jakub Jelinek <ja...@redhat.com>
* constexpr.c (cxx_eval_store_expression): Formatting fix. Handle
const_object_being_modified with array type.
--- gcc/cp/constexpr.c.jj 2019-09-27 20:33:37.600208356 +0200
+++ gcc/cp/constexpr.c 2019-09-27 20:38:38.203710246 +0200
@@ -3905,7 +4033,7 @@ cxx_eval_store_expression (const constex
bool no_zero_init = true;
releasing_vec ctors;
- while (!refs->is_empty())
+ while (!refs->is_empty ())
{
if (*valp == NULL_TREE)
{
@@ -4046,7 +4174,9 @@ cxx_eval_store_expression (const constex
if (const_object_being_modified)
{
bool fail = false;
- if (!CLASS_TYPE_P (TREE_TYPE (const_object_being_modified)))
+ tree const_objtype
+ = strip_array_types (TREE_TYPE (const_object_being_modified));
+ if (!CLASS_TYPE_P (const_objtype))
fail = true;
else
{
2019-10-02 Jakub Jelinek <ja...@redhat.com>
* constexpr.c (cxx_eval_constant_expression) <case CLEANUP_STMT>: If
not skipping upon entry to body, run cleanup with the same *jump_target
as it started to run the cleanup even if the body returns, breaks or
continues.
(potential_constant_expression_1): Allow CLEANUP_STMT.
* g++.dg/ext/constexpr-attr-cleanup1.C: New test.
--- gcc/cp/constexpr.c.jj 2019-10-02 13:15:21.822352849 +0200
+++ gcc/cp/constexpr.c 2019-10-02 13:15:53.527879667 +0200
@@ -4907,14 +4907,21 @@ cxx_eval_constant_expression (const cons
break;
case CLEANUP_STMT:
- r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval,
+ {
+ tree initial_jump_target = jump_target ? *jump_target : NULL_TREE;
+ r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval,
+ non_constant_p, overflow_p,
+ jump_target);
+ if (!CLEANUP_EH_ONLY (t) && !*non_constant_p)
+ /* Also evaluate the cleanup. If we weren't skipping at the
+ start of the CLEANUP_BODY, change jump_target temporarily
+ to &initial_jump_target, so that even a return or break or
+ continue in the body doesn't skip the cleanup. */
+ cxx_eval_constant_expression (ctx, CLEANUP_EXPR (t), true,
non_constant_p, overflow_p,
- jump_target);
- if (!CLEANUP_EH_ONLY (t) && !*non_constant_p)
- /* Also evaluate the cleanup. */
- cxx_eval_constant_expression (ctx, CLEANUP_EXPR (t), true,
- non_constant_p, overflow_p,
- jump_target);
+ jump_target ? &initial_jump_target
+ : NULL);
+ }
break;
/* These differ from cxx_eval_unary_expression in that this doesn't
@@ -6983,6 +6990,12 @@ potential_constant_expression_1 (tree t,
return true;
case CLEANUP_STMT:
+ if (!RECUR (CLEANUP_BODY (t), any))
+ return false;
+ if (!CLEANUP_EH_ONLY (t) && !RECUR (CLEANUP_EXPR (t), any))
+ return false;
+ return true;
+
case EMPTY_CLASS_EXPR:
case PREDICT_EXPR:
return false;
--- gcc/testsuite/g++.dg/ext/constexpr-attr-cleanup1.C.jj 2019-10-02
13:32:11.973282291 +0200
+++ gcc/testsuite/g++.dg/ext/constexpr-attr-cleanup1.C 2019-10-02
13:32:56.192624120 +0200
@@ -0,0 +1,30 @@
+// { dg-do compile { target c++2a } }
+
+constexpr void
+cleanup (int *x)
+{
+ if (x)
+ asm (""); // { dg-error "inline assembly is not a constant
expression" }
+} // { dg-message "only unevaluated inline assembly is
allowed in a 'constexpr' function" "" { target *-*-* } .-1 }
+
+constexpr void
+cleanup2 (int *x)
+{
+}
+
+constexpr bool
+foo ()
+{
+ int a __attribute__((cleanup (cleanup))) = 1;
+ return true;
+}
+
+constexpr bool
+bar ()
+{
+ int a __attribute__((cleanup (cleanup2))) = 1;
+ return true;
+}
+
+constexpr auto x = foo (); // { dg-message "in 'constexpr' expansion of" }
+constexpr auto y = bar ();
