| Message ID | 20230209173922.30789-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: ICE with -fno-elide-constructors and trivial fn [PR101073] | expand |
On 2/9/23 09:39, Marek Polacek wrote: > In constexpr-nsdmi3.C, with -fno-elide-constructors, we don't elide > the Y::Y(const Y&) call used to initialize o.c. So store_init_value > -> cxx_constant_init must constexpr-evaluate the call to Y::Y(const Y&) > in cxx_eval_call_expression. It's a trivial function, so we do the > "Shortcut trivial constructor/op=" code and rather than evaluating > the function, we just create an assignment > > o.c = *(const struct Y &) (const struct Y *) &(&<PLACEHOLDER_EXPR struct X>)->b > > which is a MODIFY_EXPR, so the preeval code in cxx_eval_store_expression > clears .ctor and .object, therefore we can't replace the PLACEHOLDER_EXPR > whereupon we crash at > > /* A placeholder without a referent. We can get here when > checking whether NSDMIs are noexcept, or in massage_init_elt; > just say it's non-constant for now. */ > gcc_assert (ctx->quiet); > > The PLACEHOLDER_EXPR can also be on the LHS as in constexpr-nsdmi10.C. > I don't think we can do much here, but I noticed that the whole > trivial_fn_p (fun) block is only entered when -fno-elide-constructors. > This is true since GCC 9; it wasn't easy to bisect what changes made it > so, but r240845 is probably one of them. -fno-elide-constructors is an > option for experiments only so it's not clear to me why we'd still want > to shortcut trivial constructor/op=. I propose to remove the code and > add a checking assert to make sure we're not getting a trivial_fn_p > unless -fno-elide-constructors. Hmm, trivial op= doesn't ever hit this code? > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? I don't > think I want to backport this. > > PR c++/101073 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_call_expression): Replace shortcutting trivial > constructor/op= with a checking assert. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/constexpr-nsdmi3.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi10.C: New test. > --- > gcc/cp/constexpr.cc | 25 +++---------------- > gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C | 17 +++++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi10.C | 18 +++++++++++++ > 3 files changed, 38 insertions(+), 22 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 564766c8a00..1d53dcf0f20 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -2865,28 +2865,9 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > ctx = &new_ctx; > } > > - /* Shortcut trivial constructor/op=. */ > - if (trivial_fn_p (fun)) > - { > - tree init = NULL_TREE; > - if (call_expr_nargs (t) == 2) > - init = convert_from_reference (get_nth_callarg (t, 1)); > - else if (TREE_CODE (t) == AGGR_INIT_EXPR > - && AGGR_INIT_ZERO_FIRST (t)) > - init = build_zero_init (DECL_CONTEXT (fun), NULL_TREE, false); > - if (init) > - { > - tree op = get_nth_callarg (t, 0); > - if (is_dummy_object (op)) > - op = ctx->object; > - else > - op = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (op)), op); > - tree set = build2 (MODIFY_EXPR, TREE_TYPE (op), op, init); I think the problem is using MODIFY_EXPR instead of INIT_EXPR to represent a constructor; that's why cxx_eval_store_expression thinks it's OK to preevaluate. This should properly use those two tree codes for op= and ctor, respectively. > - new_ctx.call = &new_call; > - return cxx_eval_constant_expression (&new_ctx, set, lval, > - non_constant_p, overflow_p); > - } > - } > + /* We used to shortcut trivial constructor/op= here, but nowadays > + we can only get a trivial function here with -fno-elide-constructors. */ > + gcc_checking_assert (!trivial_fn_p (fun) || !flag_elide_constructors); ...but if this optimization is so rarely triggered, this simplification is OK too. > bool non_constant_args = false; > new_call.bindings > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C > new file mode 100644 > index 00000000000..ec1c4e53387 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C > @@ -0,0 +1,17 @@ > +// PR c++/101073 > +// { dg-do compile { target c++11 } } > +// { dg-additional-options "-fno-elide-constructors" } > + > +struct Y > +{ > + int a; > +}; > + > +struct X > +{ > + Y b = Y{1}; > + Y c = this->b; > +}; > + > +constexpr X o = { }; > +static_assert(o.b.a == 1 && o.c.a == 1, ""); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C > new file mode 100644 > index 00000000000..35cb8acc15b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C > @@ -0,0 +1,18 @@ > +// PR c++/101073 > +// { dg-do compile { target c++14 } } > +// { dg-additional-options "-fno-elide-constructors" } > +// A copy of constexpr-nsdmi9.C. > + > +struct Y > +{ > + int a; > +}; > + > +struct X > +{ > + Y b = (c={5}); > + Y c = (b={1}); > +}; > + > +constexpr X o = { }; > +static_assert(o.b.a == 1 && o.c.a == 1, ""); > > base-commit: b24e9c083093a9e1b1007933a184c02f7ff058db
On Wed, Feb 15, 2023 at 02:39:16PM -0500, Jason Merrill wrote: > On 2/9/23 09:39, Marek Polacek wrote: > > In constexpr-nsdmi3.C, with -fno-elide-constructors, we don't elide > > the Y::Y(const Y&) call used to initialize o.c. So store_init_value > > -> cxx_constant_init must constexpr-evaluate the call to Y::Y(const Y&) > > in cxx_eval_call_expression. It's a trivial function, so we do the > > "Shortcut trivial constructor/op=" code and rather than evaluating > > the function, we just create an assignment > > > > o.c = *(const struct Y &) (const struct Y *) &(&<PLACEHOLDER_EXPR struct X>)->b > > > > which is a MODIFY_EXPR, so the preeval code in cxx_eval_store_expression > > clears .ctor and .object, therefore we can't replace the PLACEHOLDER_EXPR > > whereupon we crash at > > > > /* A placeholder without a referent. We can get here when > > checking whether NSDMIs are noexcept, or in massage_init_elt; > > just say it's non-constant for now. */ > > gcc_assert (ctx->quiet); > > > > The PLACEHOLDER_EXPR can also be on the LHS as in constexpr-nsdmi10.C. > > I don't think we can do much here, but I noticed that the whole > > trivial_fn_p (fun) block is only entered when -fno-elide-constructors. > > This is true since GCC 9; it wasn't easy to bisect what changes made it > > so, but r240845 is probably one of them. -fno-elide-constructors is an > > option for experiments only so it's not clear to me why we'd still want > > to shortcut trivial constructor/op=. I propose to remove the code and > > add a checking assert to make sure we're not getting a trivial_fn_p > > unless -fno-elide-constructors. > > Hmm, trivial op= doesn't ever hit this code? With -fno-elide-constructors we hit the trivial_fn_p block twice in constexpr-nsdmi9.C, once for "constexpr Y::Y(const Y&)" and then for "constexpr Y& Y::operator=(Y&&)". So it does hit the code, but only with -fno-elide-constructors. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? I don't > > think I want to backport this. > > > > PR c++/101073 > > > > gcc/cp/ChangeLog: > > > > * constexpr.cc (cxx_eval_call_expression): Replace shortcutting trivial > > constructor/op= with a checking assert. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp0x/constexpr-nsdmi3.C: New test. > > * g++.dg/cpp1y/constexpr-nsdmi10.C: New test. > > --- > > gcc/cp/constexpr.cc | 25 +++---------------- > > gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C | 17 +++++++++++++ > > .../g++.dg/cpp1y/constexpr-nsdmi10.C | 18 +++++++++++++ > > 3 files changed, 38 insertions(+), 22 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index 564766c8a00..1d53dcf0f20 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -2865,28 +2865,9 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > ctx = &new_ctx; > > } > > - /* Shortcut trivial constructor/op=. */ > > - if (trivial_fn_p (fun)) > > - { > > - tree init = NULL_TREE; > > - if (call_expr_nargs (t) == 2) > > - init = convert_from_reference (get_nth_callarg (t, 1)); > > - else if (TREE_CODE (t) == AGGR_INIT_EXPR > > - && AGGR_INIT_ZERO_FIRST (t)) > > - init = build_zero_init (DECL_CONTEXT (fun), NULL_TREE, false); > > - if (init) > > - { > > - tree op = get_nth_callarg (t, 0); > > - if (is_dummy_object (op)) > > - op = ctx->object; > > - else > > - op = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (op)), op); > > - tree set = build2 (MODIFY_EXPR, TREE_TYPE (op), op, init); > > I think the problem is using MODIFY_EXPR instead of INIT_EXPR to represent a > constructor; that's why cxx_eval_store_expression thinks it's OK to > preevaluate. This should properly use those two tree codes for op= and > ctor, respectively. Maybe it was so that the RHS in SET could refer to the op in the LHS? > > - new_ctx.call = &new_call; > > - return cxx_eval_constant_expression (&new_ctx, set, lval, > > - non_constant_p, overflow_p); > > - } > > - } > > + /* We used to shortcut trivial constructor/op= here, but nowadays > > + we can only get a trivial function here with -fno-elide-constructors. */ > > + gcc_checking_assert (!trivial_fn_p (fun) || !flag_elide_constructors); > > ...but if this optimization is so rarely triggered, this simplification is > OK too. I'd say that's better so that we don't have to update the code (like r234345 did). m~
On 2/15/23 13:37, Marek Polacek wrote: > On Wed, Feb 15, 2023 at 02:39:16PM -0500, Jason Merrill wrote: >> On 2/9/23 09:39, Marek Polacek wrote: >>> In constexpr-nsdmi3.C, with -fno-elide-constructors, we don't elide >>> the Y::Y(const Y&) call used to initialize o.c. So store_init_value >>> -> cxx_constant_init must constexpr-evaluate the call to Y::Y(const Y&) >>> in cxx_eval_call_expression. It's a trivial function, so we do the >>> "Shortcut trivial constructor/op=" code and rather than evaluating >>> the function, we just create an assignment >>> >>> o.c = *(const struct Y &) (const struct Y *) &(&<PLACEHOLDER_EXPR struct X>)->b >>> >>> which is a MODIFY_EXPR, so the preeval code in cxx_eval_store_expression >>> clears .ctor and .object, therefore we can't replace the PLACEHOLDER_EXPR >>> whereupon we crash at >>> >>> /* A placeholder without a referent. We can get here when >>> checking whether NSDMIs are noexcept, or in massage_init_elt; >>> just say it's non-constant for now. */ >>> gcc_assert (ctx->quiet); >>> >>> The PLACEHOLDER_EXPR can also be on the LHS as in constexpr-nsdmi10.C. >>> I don't think we can do much here, but I noticed that the whole >>> trivial_fn_p (fun) block is only entered when -fno-elide-constructors. >>> This is true since GCC 9; it wasn't easy to bisect what changes made it >>> so, but r240845 is probably one of them. -fno-elide-constructors is an >>> option for experiments only so it's not clear to me why we'd still want >>> to shortcut trivial constructor/op=. I propose to remove the code and >>> add a checking assert to make sure we're not getting a trivial_fn_p >>> unless -fno-elide-constructors. >> >> Hmm, trivial op= doesn't ever hit this code? > > With -fno-elide-constructors we hit the trivial_fn_p block twice in > constexpr-nsdmi9.C, once for "constexpr Y::Y(const Y&)" and then for > "constexpr Y& Y::operator=(Y&&)". So it does hit the code, but only > with -fno-elide-constructors. Odd, I'm not sure why that would make a difference for assignment. >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? I don't >>> think I want to backport this. >>> >>> PR c++/101073 >>> >>> gcc/cp/ChangeLog: >>> >>> * constexpr.cc (cxx_eval_call_expression): Replace shortcutting trivial >>> constructor/op= with a checking assert. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp0x/constexpr-nsdmi3.C: New test. >>> * g++.dg/cpp1y/constexpr-nsdmi10.C: New test. >>> --- >>> gcc/cp/constexpr.cc | 25 +++---------------- >>> gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C | 17 +++++++++++++ >>> .../g++.dg/cpp1y/constexpr-nsdmi10.C | 18 +++++++++++++ >>> 3 files changed, 38 insertions(+), 22 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C >>> >>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc >>> index 564766c8a00..1d53dcf0f20 100644 >>> --- a/gcc/cp/constexpr.cc >>> +++ b/gcc/cp/constexpr.cc >>> @@ -2865,28 +2865,9 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, >>> ctx = &new_ctx; >>> } >>> - /* Shortcut trivial constructor/op=. */ >>> - if (trivial_fn_p (fun)) >>> - { >>> - tree init = NULL_TREE; >>> - if (call_expr_nargs (t) == 2) >>> - init = convert_from_reference (get_nth_callarg (t, 1)); >>> - else if (TREE_CODE (t) == AGGR_INIT_EXPR >>> - && AGGR_INIT_ZERO_FIRST (t)) >>> - init = build_zero_init (DECL_CONTEXT (fun), NULL_TREE, false); >>> - if (init) >>> - { >>> - tree op = get_nth_callarg (t, 0); >>> - if (is_dummy_object (op)) >>> - op = ctx->object; >>> - else >>> - op = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (op)), op); >>> - tree set = build2 (MODIFY_EXPR, TREE_TYPE (op), op, init); >> >> I think the problem is using MODIFY_EXPR instead of INIT_EXPR to represent a >> constructor; that's why cxx_eval_store_expression thinks it's OK to >> preevaluate. This should properly use those two tree codes for op= and >> ctor, respectively. > > Maybe it was so that the RHS in SET could refer to the op in the LHS? I think it was just an oversight. You need INIT_EXPR for the rhs to refer to the lhs. >>> - new_ctx.call = &new_call; >>> - return cxx_eval_constant_expression (&new_ctx, set, lval, >>> - non_constant_p, overflow_p); >>> - } >>> - } >>> + /* We used to shortcut trivial constructor/op= here, but nowadays >>> + we can only get a trivial function here with -fno-elide-constructors. */ >>> + gcc_checking_assert (!trivial_fn_p (fun) || !flag_elide_constructors); >> >> ...but if this optimization is so rarely triggered, this simplification is >> OK too. > > I'd say that's better so that we don't have to update the code (like > r234345 did). Indeed, the patch is OK. Jason
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index 564766c8a00..1d53dcf0f20 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -2865,28 +2865,9 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, ctx = &new_ctx; } - /* Shortcut trivial constructor/op=. */ - if (trivial_fn_p (fun)) - { - tree init = NULL_TREE; - if (call_expr_nargs (t) == 2) - init = convert_from_reference (get_nth_callarg (t, 1)); - else if (TREE_CODE (t) == AGGR_INIT_EXPR - && AGGR_INIT_ZERO_FIRST (t)) - init = build_zero_init (DECL_CONTEXT (fun), NULL_TREE, false); - if (init) - { - tree op = get_nth_callarg (t, 0); - if (is_dummy_object (op)) - op = ctx->object; - else - op = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (op)), op); - tree set = build2 (MODIFY_EXPR, TREE_TYPE (op), op, init); - new_ctx.call = &new_call; - return cxx_eval_constant_expression (&new_ctx, set, lval, - non_constant_p, overflow_p); - } - } + /* We used to shortcut trivial constructor/op= here, but nowadays + we can only get a trivial function here with -fno-elide-constructors. */ + gcc_checking_assert (!trivial_fn_p (fun) || !flag_elide_constructors); bool non_constant_args = false; new_call.bindings diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C new file mode 100644 index 00000000000..ec1c4e53387 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C @@ -0,0 +1,17 @@ +// PR c++/101073 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fno-elide-constructors" } + +struct Y +{ + int a; +}; + +struct X +{ + Y b = Y{1}; + Y c = this->b; +}; + +constexpr X o = { }; +static_assert(o.b.a == 1 && o.c.a == 1, ""); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C new file mode 100644 index 00000000000..35cb8acc15b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C @@ -0,0 +1,18 @@ +// PR c++/101073 +// { dg-do compile { target c++14 } } +// { dg-additional-options "-fno-elide-constructors" } +// A copy of constexpr-nsdmi9.C. + +struct Y +{ + int a; +}; + +struct X +{ + Y b = (c={5}); + Y c = (b={1}); +}; + +constexpr X o = { }; +static_assert(o.b.a == 1 && o.c.a == 1, "");
In constexpr-nsdmi3.C, with -fno-elide-constructors, we don't elide the Y::Y(const Y&) call used to initialize o.c. So store_init_value -> cxx_constant_init must constexpr-evaluate the call to Y::Y(const Y&) in cxx_eval_call_expression. It's a trivial function, so we do the "Shortcut trivial constructor/op=" code and rather than evaluating the function, we just create an assignment o.c = *(const struct Y &) (const struct Y *) &(&<PLACEHOLDER_EXPR struct X>)->b which is a MODIFY_EXPR, so the preeval code in cxx_eval_store_expression clears .ctor and .object, therefore we can't replace the PLACEHOLDER_EXPR whereupon we crash at /* A placeholder without a referent. We can get here when checking whether NSDMIs are noexcept, or in massage_init_elt; just say it's non-constant for now. */ gcc_assert (ctx->quiet); The PLACEHOLDER_EXPR can also be on the LHS as in constexpr-nsdmi10.C. I don't think we can do much here, but I noticed that the whole trivial_fn_p (fun) block is only entered when -fno-elide-constructors. This is true since GCC 9; it wasn't easy to bisect what changes made it so, but r240845 is probably one of them. -fno-elide-constructors is an option for experiments only so it's not clear to me why we'd still want to shortcut trivial constructor/op=. I propose to remove the code and add a checking assert to make sure we're not getting a trivial_fn_p unless -fno-elide-constructors. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? I don't think I want to backport this. PR c++/101073 gcc/cp/ChangeLog: * constexpr.cc (cxx_eval_call_expression): Replace shortcutting trivial constructor/op= with a checking assert. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/constexpr-nsdmi3.C: New test. * g++.dg/cpp1y/constexpr-nsdmi10.C: New test. --- gcc/cp/constexpr.cc | 25 +++---------------- gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C | 17 +++++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi10.C | 18 +++++++++++++ 3 files changed, 38 insertions(+), 22 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-nsdmi3.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi10.C base-commit: b24e9c083093a9e1b1007933a184c02f7ff058db