OK, thanks.
On Fri, Feb 23, 2018 at 9:29 AM, Marek Polacek <pola...@redhat.com> wrote: > On Fri, Feb 16, 2018 at 04:10:20PM -0500, Jason Merrill wrote: >> On Mon, Feb 5, 2018 at 1:45 PM, Jason Merrill <ja...@redhat.com> wrote: >> > On Mon, Feb 5, 2018 at 8:37 AM, Marek Polacek <pola...@redhat.com> wrote: >> >> On Fri, Feb 02, 2018 at 02:11:27PM -0500, Jason Merrill wrote: >> >>> On Thu, Jan 25, 2018 at 4:16 PM, Marek Polacek <pola...@redhat.com> >> >>> wrote: >> >>> > This is a similar problem to 83116: we'd cached a constexpr call, but >> >>> > after a >> >>> > store the result had become invalid, yet we used the wrong result >> >>> > again when >> >>> > encountering the same call later. This resulted in evaluating a >> >>> > THROW_EXPR >> >>> > which doesn't work. Details in >> >>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83692#c5 >> >>> > >> >>> > The fix for 83116 didn't work here, because when evaluating the body >> >>> > of the >> >>> > ctor via store_init_value -> cxx_constant_value we are in STRICT, so >> >>> > we do >> >>> > cache. >> >>> >> >>> > It seems that we may no longer rely on the constexpr call table when we >> >>> > do cxx_eval_store_expression, because that just rewrites *valp, i.e. >> >>> > the >> >>> > value of an object. Might be too big a hammer again, but I couldn't >> >>> > think >> >>> > of how I could guard the caching of a constexpr call. >> >>> >> >>> > This doesn't manifest in C++14 because build_special_member_call in >> >>> > C++17 is >> >>> > more aggressive with copy elisions (as required by P0135 which changed >> >>> > how we >> >>> > view prvalues). In C++14 build_special_member_call produces a >> >>> > CALL_EXPR, so >> >>> > expand_default_init calls maybe_constant_init, for which STRICT is >> >>> > false, so >> >>> > we avoid caching as per 83116. >> >>> >> >>> So it sounds like the problem is using cxx_constant_value for the >> >>> diagnostic when it has different semantics from the >> >>> maybe_constant_init that follows right after. I guess we want a >> >>> cxx_constant_init function that is a hybrid of the two. >> >> >> >> So like the following? Thanks, >> >> >> >> Bootstrapped/regtested on x86_64-linux, ok for trunk? >> >> >> >> 2018-02-04 Marek Polacek <pola...@redhat.com> >> >> >> >> PR c++/83692 >> >> * constexpr.c (cxx_constant_init): New function. >> >> * cp-tree.h (cxx_constant_init): Declare. >> >> * typeck2.c (store_init_value): Call cxx_constant_init instead of >> >> cxx_constant_value. >> >> >> >> +/* Like cxx_constant_value, but non-strict mode. */ >> >> + >> >> +tree >> >> +cxx_constant_init (tree t, tree decl) >> >> +{ >> >> + return cxx_eval_outermost_constant_expr (t, false, false, decl); >> >> +} >> > >> > Hmm, that doesn't do the TARGET_EXPR stripping that >> > maybe_constant_init does. I was thinking of a version of >> > maybe_constant_init that passes false to allow_non_constant. Probably >> > by making "maybe_constant_init" and cxx_constant_init both call the >> > current function with an additional parameter. And then the existing >> > call to maybe_constant_init can move under an 'else' to avoid >> > redundant constexpr evaluation. >> >> Want me to take this over? > > Sorry again for the delay. > > I tried to do what you suggested. There was one twist: it regressed > constexpr-nullptr-2.C, in particular we lost diagnostics for > > constexpr int* pj0 = &((S*)0)->j; // { dg-error "not a constant > expression" } > constexpr int* pj1 = &((S*)nullptr)->j; // { dg-error "not a constant > expression" } > > because when maybe_constant_init_1 saw a constant: > > 5142 else if (CONSTANT_CLASS_P (t)) > 5143 /* No evaluation needed. */; > > so it didn't call cxx_eval_outermost_constant_expr which is supposed to give > the error. I fixed it by adding "&& allow_non_constant" so now it gives the > proper diagnostics. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2018-02-23 Marek Polacek <pola...@redhat.com> > > PR c++/83692 > * constexpr.c (maybe_constant_init_1): New function. > (maybe_constant_init): Make it a wrapper around maybe_constant_init_1. > (cxx_constant_init): New function. > * cp-tree.h (cxx_constant_init): Declare. > * typeck2.c (store_init_value): Call cxx_constant_init instead of > cxx_constant_value. Move the maybe_constant_init call under an > 'else'. > > * g++.dg/cpp1z/constexpr-83692.C: New test. > > diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c > index 47ff90cb055..26d0d099a05 100644 > --- gcc/cp/constexpr.c > +++ gcc/cp/constexpr.c > @@ -5123,8 +5123,8 @@ fold_non_dependent_expr (tree t) > /* Like maybe_constant_value, but returns a CONSTRUCTOR directly, rather > than wrapped in a TARGET_EXPR. */ > > -tree > -maybe_constant_init (tree t, tree decl) > +static tree > +maybe_constant_init_1 (tree t, tree decl, bool allow_non_constant) > { > if (!t) > return t; > @@ -5139,10 +5139,10 @@ maybe_constant_init (tree t, tree decl) > t = TARGET_EXPR_INITIAL (t); > if (!is_nondependent_static_init_expression (t)) > /* Don't try to evaluate it. */; > - else if (CONSTANT_CLASS_P (t)) > + else if (CONSTANT_CLASS_P (t) && allow_non_constant) > /* No evaluation needed. */; > else > - t = cxx_eval_outermost_constant_expr (t, true, false, decl); > + t = cxx_eval_outermost_constant_expr (t, allow_non_constant, false, > decl); > if (TREE_CODE (t) == TARGET_EXPR) > { > tree init = TARGET_EXPR_INITIAL (t); > @@ -5152,6 +5152,22 @@ maybe_constant_init (tree t, tree decl) > return t; > } > > +/* Wrapper for maybe_constant_init_1 which permits non constants. */ > + > +tree > +maybe_constant_init (tree t, tree decl) > +{ > + return maybe_constant_init_1 (t, decl, true); > +} > + > +/* Wrapper for maybe_constant_init_1 which does not permit non constants. */ > + > +tree > +cxx_constant_init (tree t, tree decl) > +{ > + return maybe_constant_init_1 (t, decl, false); > +} > + > #if 0 > /* FIXME see ADDR_EXPR section in potential_constant_expression_1. */ > /* Return true if the object referred to by REF has automatic or thread > diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h > index 9038d677b2d..04c7b7ce3a9 100644 > --- gcc/cp/cp-tree.h > +++ gcc/cp/cp-tree.h > @@ -7411,6 +7411,7 @@ extern bool require_potential_constant_expression > (tree); > extern bool require_constant_expression (tree); > extern bool require_potential_rvalue_constant_expression (tree); > extern tree cxx_constant_value (tree, tree = NULL_TREE); > +extern tree cxx_constant_init (tree, tree = NULL_TREE); > extern tree maybe_constant_value (tree, tree = NULL_TREE); > extern tree maybe_constant_init (tree, tree = > NULL_TREE); > extern tree fold_non_dependent_expr (tree); > diff --git gcc/cp/typeck2.c gcc/cp/typeck2.c > index 899d60e8535..153b46cca77 100644 > --- gcc/cp/typeck2.c > +++ gcc/cp/typeck2.c > @@ -830,9 +830,10 @@ store_init_value (tree decl, tree init, vec<tree, > va_gc>** cleanups, int flags) > if (!require_constant_expression (value)) > value = error_mark_node; > else > - value = cxx_constant_value (value, decl); > + value = cxx_constant_init (value, decl); > } > - value = maybe_constant_init (value, decl); > + else > + value = maybe_constant_init (value, decl); > if (TREE_CODE (value) == CONSTRUCTOR && cp_has_mutable_p (type)) > /* Poison this CONSTRUCTOR so it can't be copied to another > constexpr variable. */ > diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > index e69de29bb2d..f6b61eeab85 100644 > --- gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > +++ gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > @@ -0,0 +1,21 @@ > +// PR c++/83692 > +// { dg-options -std=c++17 } > + > +struct integer { > + constexpr int value() const { return m_value; } > + int m_value; > +}; > + > +struct outer { > + integer m_x{0}; > + constexpr outer() > + { > + if (m_x.value() != 0) > + throw 0; > + m_x.m_value = integer{1}.value(); > + if (m_x.value() != 1) > + throw 0; > + } > +}; > + > +constexpr outer o{}; > > Marek
