* [PATCH] c++: get_nsdmi in template context [PR108116] @ 2022-12-21 14:52 Patrick Palka 2022-12-21 14:56 ` Patrick Palka 2022-12-21 21:48 ` Jason Merrill 0 siblings, 2 replies; 10+ messages in thread From: Patrick Palka @ 2022-12-21 14:52 UTC (permalink / raw) To: gcc-patches; +Cc: jason, Patrick Palka Here during ahead of time checking of C{}, we indirectly call get_nsdmi for C::m from finish_compound_literal, which in turn calls break_out_target_exprs for C::m's (non-templated) initializer, during which we end up building a call to A::~A and checking expr_noexcept_p for it (from build_vec_delete_1). But this is all done with processing_template_decl set, so the built A::~A call is templated (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which expr_noexcept_p doesn't expect and we crash. In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a expr_noexcept_p call with !processing_template_decl, which works here too. But it seems to me since the initializer we obtain in get_nsdmi is always non-templated, it should be calling break_out_target_exprs with processing_template_decl cleared since otherwise the function might end up mixing templated and non-templated trees. I'm not sure about this though, perhaps this is not the best fix here. Alternatively, when processing_template_decl we could make get_nsdmi avoid calling break_out_target_exprs at all or something. Additionally, perhaps break_out_target_exprs should be a no-op more generally when processing_template_decl since we shouldn't see any TARGET_EXPRs inside a template? Bootstrapped and regtested on x86_64-pc-linux-gnu. PR c++/108116 gcc/cp/ChangeLog: * init.cc (get_nsdmi): Clear processing_template_decl before processing the non-templated initializer. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/nsdmi-template24.C: New test. --- gcc/cp/init.cc | 8 ++++++- gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc index 73e6547c076..c4345ebdaea 100644 --- a/gcc/cp/init.cc +++ b/gcc/cp/init.cc @@ -561,7 +561,8 @@ perform_target_ctor (tree init) return init; } -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ +/* Return the non-static data initializer for FIELD_DECL MEMBER. + The initializer returned is always non-templated. */ static GTY((cache)) decl_tree_cache_map *nsdmi_inst; @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain) current_class_ptr = build_address (current_class_ref); } + /* Since INIT is always non-templated clear processing_template_decl + before processing it so that we don't interleave templated and + non-templated trees. */ + processing_template_decl_sentinel ptds; + /* Strip redundant TARGET_EXPR so we don't need to remap it, and so the aggregate init code below will see a CONSTRUCTOR. */ bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C new file mode 100644 index 00000000000..202c67d7321 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C @@ -0,0 +1,22 @@ +// PR c++/108116 +// { dg-do compile { target c++11 } } + +#include <initializer_list> + +struct A { + A(int); + ~A(); +}; + +struct B { + B(std::initializer_list<A>); +}; + +struct C { + B m{0}; +}; + +template<class> +void f() { + C c = C{}; +}; -- 2.39.0.95.g7c2ef319c5 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-21 14:52 [PATCH] c++: get_nsdmi in template context [PR108116] Patrick Palka @ 2022-12-21 14:56 ` Patrick Palka 2022-12-21 21:48 ` Jason Merrill 1 sibling, 0 replies; 10+ messages in thread From: Patrick Palka @ 2022-12-21 14:56 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, jason On Wed, 21 Dec 2022, Patrick Palka wrote: > Here during ahead of time checking of C{}, we indirectly call get_nsdmi > for C::m from finish_compound_literal, which in turn calls > break_out_target_exprs for C::m's (non-templated) initializer, during > which we end up building a call to A::~A and checking expr_noexcept_p > for it (from build_vec_delete_1). But this is all done with > processing_template_decl set, so the built A::~A call is templated > (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which > expr_noexcept_p doesn't expect and we crash. > > In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a > expr_noexcept_p call with !processing_template_decl, which works here > too. But it seems to me since the initializer we obtain in get_nsdmi is > always non-templated, it should be calling break_out_target_exprs with > processing_template_decl cleared since otherwise the function might end > up mixing templated and non-templated trees. > > I'm not sure about this though, perhaps this is not the best fix here. > Alternatively, when processing_template_decl we could make get_nsdmi > avoid calling break_out_target_exprs at all or something. Additionally, > perhaps break_out_target_exprs should be a no-op more generally when > processing_template_decl since we shouldn't see any TARGET_EXPRs inside > a template? > > Bootstrapped and regtested on x86_64-pc-linux-gnu. Note this is a 12 regression so I suppose there's also the question of what's safest to backport vs what's the best fix.. > > PR c++/108116 > > gcc/cp/ChangeLog: > > * init.cc (get_nsdmi): Clear processing_template_decl before > processing the non-templated initializer. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/nsdmi-template24.C: New test. > --- > gcc/cp/init.cc | 8 ++++++- > gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > index 73e6547c076..c4345ebdaea 100644 > --- a/gcc/cp/init.cc > +++ b/gcc/cp/init.cc > @@ -561,7 +561,8 @@ perform_target_ctor (tree init) > return init; > } > > -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ > +/* Return the non-static data initializer for FIELD_DECL MEMBER. > + The initializer returned is always non-templated. */ > > static GTY((cache)) decl_tree_cache_map *nsdmi_inst; > > @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain) > current_class_ptr = build_address (current_class_ref); > } > > + /* Since INIT is always non-templated clear processing_template_decl > + before processing it so that we don't interleave templated and > + non-templated trees. */ > + processing_template_decl_sentinel ptds; > + > /* Strip redundant TARGET_EXPR so we don't need to remap it, and > so the aggregate init code below will see a CONSTRUCTOR. */ > bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); > diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > new file mode 100644 > index 00000000000..202c67d7321 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > @@ -0,0 +1,22 @@ > +// PR c++/108116 > +// { dg-do compile { target c++11 } } > + > +#include <initializer_list> > + > +struct A { > + A(int); > + ~A(); > +}; > + > +struct B { > + B(std::initializer_list<A>); > +}; > + > +struct C { > + B m{0}; > +}; > + > +template<class> > +void f() { > + C c = C{}; > +}; > -- > 2.39.0.95.g7c2ef319c5 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-21 14:52 [PATCH] c++: get_nsdmi in template context [PR108116] Patrick Palka 2022-12-21 14:56 ` Patrick Palka @ 2022-12-21 21:48 ` Jason Merrill 2022-12-22 16:31 ` Patrick Palka 1 sibling, 1 reply; 10+ messages in thread From: Jason Merrill @ 2022-12-21 21:48 UTC (permalink / raw) To: Patrick Palka, gcc-patches On 12/21/22 09:52, Patrick Palka wrote: > Here during ahead of time checking of C{}, we indirectly call get_nsdmi > for C::m from finish_compound_literal, which in turn calls > break_out_target_exprs for C::m's (non-templated) initializer, during > which we end up building a call to A::~A and checking expr_noexcept_p > for it (from build_vec_delete_1). But this is all done with > processing_template_decl set, so the built A::~A call is templated > (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which > expr_noexcept_p doesn't expect and we crash. > > In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a > expr_noexcept_p call with !processing_template_decl, which works here > too. But it seems to me since the initializer we obtain in get_nsdmi is > always non-templated, it should be calling break_out_target_exprs with > processing_template_decl cleared since otherwise the function might end > up mixing templated and non-templated trees. > > I'm not sure about this though, perhaps this is not the best fix here. > Alternatively, when processing_template_decl we could make get_nsdmi > avoid calling break_out_target_exprs at all or something. Additionally, > perhaps break_out_target_exprs should be a no-op more generally when > processing_template_decl since we shouldn't see any TARGET_EXPRs inside > a template? Hmm. Any time we would call break_out_target_exprs we're dealing with non-dependent expressions; if we're in a template, we're building up an initializer or a call that we'll soon throw away, just for the purpose of checking or type computation. Furthermore, as you say, the argument is always a non-template tree, whether in get_nsdmi or convert_default_arg. So having processing_template_decl cleared would be correct. I don't think we can get away with not calling break_out_target_exprs at all in a template; if nothing else, we would lose immediate invocation expansion. However, we could probably skip the bot_manip tree walk, which should avoid the problem. Either way we end up returning non-template trees, as we do now, and callers have to deal with transient CONSTRUCTORs containing such (as we do in massage_init_elt). Does convert_default_arg not run into the same problem, e.g. when calling void g(B = {0}); ? > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > PR c++/108116 > > gcc/cp/ChangeLog: > > * init.cc (get_nsdmi): Clear processing_template_decl before > processing the non-templated initializer. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/nsdmi-template24.C: New test. > --- > gcc/cp/init.cc | 8 ++++++- > gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > index 73e6547c076..c4345ebdaea 100644 > --- a/gcc/cp/init.cc > +++ b/gcc/cp/init.cc > @@ -561,7 +561,8 @@ perform_target_ctor (tree init) > return init; > } > > -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ > +/* Return the non-static data initializer for FIELD_DECL MEMBER. > + The initializer returned is always non-templated. */ > > static GTY((cache)) decl_tree_cache_map *nsdmi_inst; > > @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain) > current_class_ptr = build_address (current_class_ref); > } > > + /* Since INIT is always non-templated clear processing_template_decl > + before processing it so that we don't interleave templated and > + non-templated trees. */ > + processing_template_decl_sentinel ptds; > + > /* Strip redundant TARGET_EXPR so we don't need to remap it, and > so the aggregate init code below will see a CONSTRUCTOR. */ > bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); > diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > new file mode 100644 > index 00000000000..202c67d7321 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > @@ -0,0 +1,22 @@ > +// PR c++/108116 > +// { dg-do compile { target c++11 } } > + > +#include <initializer_list> > + > +struct A { > + A(int); > + ~A(); > +}; > + > +struct B { > + B(std::initializer_list<A>); > +}; > + > +struct C { > + B m{0}; > +}; > + > +template<class> > +void f() { > + C c = C{}; > +}; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-21 21:48 ` Jason Merrill @ 2022-12-22 16:31 ` Patrick Palka 2022-12-22 21:33 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Patrick Palka @ 2022-12-22 16:31 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Wed, 21 Dec 2022, Jason Merrill wrote: > On 12/21/22 09:52, Patrick Palka wrote: > > Here during ahead of time checking of C{}, we indirectly call get_nsdmi > > for C::m from finish_compound_literal, which in turn calls > > break_out_target_exprs for C::m's (non-templated) initializer, during > > which we end up building a call to A::~A and checking expr_noexcept_p > > for it (from build_vec_delete_1). But this is all done with > > processing_template_decl set, so the built A::~A call is templated > > (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which > > expr_noexcept_p doesn't expect and we crash. > > > > In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a > > expr_noexcept_p call with !processing_template_decl, which works here > > too. But it seems to me since the initializer we obtain in get_nsdmi is > > always non-templated, it should be calling break_out_target_exprs with > > processing_template_decl cleared since otherwise the function might end > > up mixing templated and non-templated trees. > > > > I'm not sure about this though, perhaps this is not the best fix here. > > Alternatively, when processing_template_decl we could make get_nsdmi > > avoid calling break_out_target_exprs at all or something. Additionally, > > perhaps break_out_target_exprs should be a no-op more generally when > > processing_template_decl since we shouldn't see any TARGET_EXPRs inside > > a template? > > Hmm. > > Any time we would call break_out_target_exprs we're dealing with non-dependent > expressions; if we're in a template, we're building up an initializer or a > call that we'll soon throw away, just for the purpose of checking or type > computation. > > Furthermore, as you say, the argument is always a non-template tree, whether > in get_nsdmi or convert_default_arg. So having processing_template_decl > cleared would be correct. > > I don't think we can get away with not calling break_out_target_exprs at all > in a template; if nothing else, we would lose immediate invocation expansion. > However, we could probably skip the bot_manip tree walk, which should avoid > the problem. > > Either way we end up returning non-template trees, as we do now, and callers > have to deal with transient CONSTRUCTORs containing such (as we do in > massage_init_elt). Ah I see, makes sense. > > Does convert_default_arg not run into the same problem, e.g. when calling > > void g(B = {0}); In practice it seems not, because we don't call convert_default_arg when processing_template_decl is set (verified with an assert to that effect). In build_over_call for example we exit early when processing_template_decl is set, and return a templated CALL_EXPR that doesn't include default arguments at all. A consequence of this is that we don't reject ahead of time a call that would use an ill-formed dependent default argument, e.g. template<class T> void g(B = T{0}); template<class> void f() { g<void>(); } since the default argument instantiation would be the responsibility of convert_default_arg. Thinking hypothetically here, if we do in the future want to include default arguments in the templated form of a CALL_EXPR, we'd probably have to instantiate them with processing_template_decl set so that the result is templated. And we'd subsequently want to call break_out_target_exprs on the result also with processing_template_decl set IIUC, to perform immediate invocation expansion. This seems to be a potential use case for being able to call break_out_target_exprs on templated trees, and so unconditionally clearing p_t_d from break_out_target_exprs might not be future proof. In light of this, shall we go with the original approach to clear processing_template_decl directly from get_nsdmi? > > ? > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > PR c++/108116 > > > > gcc/cp/ChangeLog: > > > > * init.cc (get_nsdmi): Clear processing_template_decl before > > processing the non-templated initializer. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp0x/nsdmi-template24.C: New test. > > --- > > gcc/cp/init.cc | 8 ++++++- > > gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ > > 2 files changed, 29 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > > index 73e6547c076..c4345ebdaea 100644 > > --- a/gcc/cp/init.cc > > +++ b/gcc/cp/init.cc > > @@ -561,7 +561,8 @@ perform_target_ctor (tree init) > > return init; > > } > > -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ > > +/* Return the non-static data initializer for FIELD_DECL MEMBER. > > + The initializer returned is always non-templated. */ > > static GTY((cache)) decl_tree_cache_map *nsdmi_inst; > > @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t > > complain) > > current_class_ptr = build_address (current_class_ref); > > } > > + /* Since INIT is always non-templated clear processing_template_decl > > + before processing it so that we don't interleave templated and > > + non-templated trees. */ > > + processing_template_decl_sentinel ptds; > > + > > /* Strip redundant TARGET_EXPR so we don't need to remap it, and > > so the aggregate init code below will see a CONSTRUCTOR. */ > > bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); > > diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > new file mode 100644 > > index 00000000000..202c67d7321 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > @@ -0,0 +1,22 @@ > > +// PR c++/108116 > > +// { dg-do compile { target c++11 } } > > + > > +#include <initializer_list> > > + > > +struct A { > > + A(int); > > + ~A(); > > +}; > > + > > +struct B { > > + B(std::initializer_list<A>); > > +}; > > + > > +struct C { > > + B m{0}; > > +}; > > + > > +template<class> > > +void f() { > > + C c = C{}; > > +}; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-22 16:31 ` Patrick Palka @ 2022-12-22 21:33 ` Jason Merrill 2022-12-22 21:41 ` Patrick Palka 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2022-12-22 21:33 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 12/22/22 11:31, Patrick Palka wrote: > On Wed, 21 Dec 2022, Jason Merrill wrote: > >> On 12/21/22 09:52, Patrick Palka wrote: >>> Here during ahead of time checking of C{}, we indirectly call get_nsdmi >>> for C::m from finish_compound_literal, which in turn calls >>> break_out_target_exprs for C::m's (non-templated) initializer, during >>> which we end up building a call to A::~A and checking expr_noexcept_p >>> for it (from build_vec_delete_1). But this is all done with >>> processing_template_decl set, so the built A::~A call is templated >>> (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which >>> expr_noexcept_p doesn't expect and we crash. >>> >>> In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a >>> expr_noexcept_p call with !processing_template_decl, which works here >>> too. But it seems to me since the initializer we obtain in get_nsdmi is >>> always non-templated, it should be calling break_out_target_exprs with >>> processing_template_decl cleared since otherwise the function might end >>> up mixing templated and non-templated trees. >>> >>> I'm not sure about this though, perhaps this is not the best fix here. >>> Alternatively, when processing_template_decl we could make get_nsdmi >>> avoid calling break_out_target_exprs at all or something. Additionally, >>> perhaps break_out_target_exprs should be a no-op more generally when >>> processing_template_decl since we shouldn't see any TARGET_EXPRs inside >>> a template? >> >> Hmm. >> >> Any time we would call break_out_target_exprs we're dealing with non-dependent >> expressions; if we're in a template, we're building up an initializer or a >> call that we'll soon throw away, just for the purpose of checking or type >> computation. >> >> Furthermore, as you say, the argument is always a non-template tree, whether >> in get_nsdmi or convert_default_arg. So having processing_template_decl >> cleared would be correct. >> >> I don't think we can get away with not calling break_out_target_exprs at all >> in a template; if nothing else, we would lose immediate invocation expansion. >> However, we could probably skip the bot_manip tree walk, which should avoid >> the problem. >> >> Either way we end up returning non-template trees, as we do now, and callers >> have to deal with transient CONSTRUCTORs containing such (as we do in >> massage_init_elt). > > Ah I see, makes sense. > >> >> Does convert_default_arg not run into the same problem, e.g. when calling >> >> void g(B = {0}); > > In practice it seems not, because we don't call convert_default_arg > when processing_template_decl is set (verified with an assert to > that effect). In build_over_call for example we exit early when > processing_template_decl is set, and return a templated CALL_EXPR > that doesn't include default arguments at all. A consequence of > this is that we don't reject ahead of time a call that would use > an ill-formed dependent default argument, e.g. > > template<class T> > void g(B = T{0}); > > template<class> > void f() { > g<void>(); > } > > since the default argument instantiation would be the responsibility > of convert_default_arg. > > Thinking hypothetically here, if we do in the future want to include default > arguments in the templated form of a CALL_EXPR, We definitely do not want to; the templated form should be as close as possible to the source. We might want to perform non-dependent conversions to get any errors (such as this one) before throwing away the result. Which would be parallel to what we currently do in calling get_nsdmi, and would want the same behavior. > [snip] > shall we go with the original approach to clear > processing_template_decl directly from get_nsdmi? OK, but then we should also checking_assert !processing_template_decl in b_o_t_e. Jason >> ? >> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu. >>> >>> PR c++/108116 >>> >>> gcc/cp/ChangeLog: >>> >>> * init.cc (get_nsdmi): Clear processing_template_decl before >>> processing the non-templated initializer. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp0x/nsdmi-template24.C: New test. >>> --- >>> gcc/cp/init.cc | 8 ++++++- >>> gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ >>> 2 files changed, 29 insertions(+), 1 deletion(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>> >>> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc >>> index 73e6547c076..c4345ebdaea 100644 >>> --- a/gcc/cp/init.cc >>> +++ b/gcc/cp/init.cc >>> @@ -561,7 +561,8 @@ perform_target_ctor (tree init) >>> return init; >>> } >>> -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ >>> +/* Return the non-static data initializer for FIELD_DECL MEMBER. >>> + The initializer returned is always non-templated. */ >>> static GTY((cache)) decl_tree_cache_map *nsdmi_inst; >>> @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t >>> complain) >>> current_class_ptr = build_address (current_class_ref); >>> } >>> + /* Since INIT is always non-templated clear processing_template_decl >>> + before processing it so that we don't interleave templated and >>> + non-templated trees. */ >>> + processing_template_decl_sentinel ptds; >>> + >>> /* Strip redundant TARGET_EXPR so we don't need to remap it, and >>> so the aggregate init code below will see a CONSTRUCTOR. */ >>> bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); >>> diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>> b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>> new file mode 100644 >>> index 00000000000..202c67d7321 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>> @@ -0,0 +1,22 @@ >>> +// PR c++/108116 >>> +// { dg-do compile { target c++11 } } >>> + >>> +#include <initializer_list> >>> + >>> +struct A { >>> + A(int); >>> + ~A(); >>> +}; >>> + >>> +struct B { >>> + B(std::initializer_list<A>); >>> +}; >>> + >>> +struct C { >>> + B m{0}; >>> +}; >>> + >>> +template<class> >>> +void f() { >>> + C c = C{}; >>> +}; >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-22 21:33 ` Jason Merrill @ 2022-12-22 21:41 ` Patrick Palka 2022-12-22 22:03 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Patrick Palka @ 2022-12-22 21:41 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Thu, 22 Dec 2022, Jason Merrill wrote: > On 12/22/22 11:31, Patrick Palka wrote: > > On Wed, 21 Dec 2022, Jason Merrill wrote: > > > > > On 12/21/22 09:52, Patrick Palka wrote: > > > > Here during ahead of time checking of C{}, we indirectly call get_nsdmi > > > > for C::m from finish_compound_literal, which in turn calls > > > > break_out_target_exprs for C::m's (non-templated) initializer, during > > > > which we end up building a call to A::~A and checking expr_noexcept_p > > > > for it (from build_vec_delete_1). But this is all done with > > > > processing_template_decl set, so the built A::~A call is templated > > > > (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which > > > > expr_noexcept_p doesn't expect and we crash. > > > > > > > > In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a > > > > expr_noexcept_p call with !processing_template_decl, which works here > > > > too. But it seems to me since the initializer we obtain in get_nsdmi is > > > > always non-templated, it should be calling break_out_target_exprs with > > > > processing_template_decl cleared since otherwise the function might end > > > > up mixing templated and non-templated trees. > > > > > > > > I'm not sure about this though, perhaps this is not the best fix here. > > > > Alternatively, when processing_template_decl we could make get_nsdmi > > > > avoid calling break_out_target_exprs at all or something. Additionally, > > > > perhaps break_out_target_exprs should be a no-op more generally when > > > > processing_template_decl since we shouldn't see any TARGET_EXPRs inside > > > > a template? > > > > > > Hmm. > > > > > > Any time we would call break_out_target_exprs we're dealing with > > > non-dependent > > > expressions; if we're in a template, we're building up an initializer or a > > > call that we'll soon throw away, just for the purpose of checking or type > > > computation. > > > > > > Furthermore, as you say, the argument is always a non-template tree, > > > whether > > > in get_nsdmi or convert_default_arg. So having processing_template_decl > > > cleared would be correct. > > > > > > I don't think we can get away with not calling break_out_target_exprs at > > > all > > > in a template; if nothing else, we would lose immediate invocation > > > expansion. > > > However, we could probably skip the bot_manip tree walk, which should > > > avoid > > > the problem. > > > > > > Either way we end up returning non-template trees, as we do now, and > > > callers > > > have to deal with transient CONSTRUCTORs containing such (as we do in > > > massage_init_elt). > > > > Ah I see, makes sense. > > > > > > > > Does convert_default_arg not run into the same problem, e.g. when calling > > > > > > void g(B = {0}); > > > > In practice it seems not, because we don't call convert_default_arg > > when processing_template_decl is set (verified with an assert to > > that effect). In build_over_call for example we exit early when > > processing_template_decl is set, and return a templated CALL_EXPR > > that doesn't include default arguments at all. A consequence of > > this is that we don't reject ahead of time a call that would use > > an ill-formed dependent default argument, e.g. > > > > template<class T> > > void g(B = T{0}); > > > > template<class> > > void f() { > > g<void>(); > > } > > > > since the default argument instantiation would be the responsibility > > of convert_default_arg. > > > > Thinking hypothetically here, if we do in the future want to include default > > arguments in the templated form of a CALL_EXPR, > > We definitely do not want to; the templated form should be as close as > possible to the source. Ah, sounds good. > > We might want to perform non-dependent conversions to get any errors (such as > this one) before throwing away the result. Which would be parallel to what we > currently do in calling get_nsdmi, and would want the same behavior. *nod* > > > [snip] > > > shall we go with the original approach to clear > > processing_template_decl directly from get_nsdmi? > > OK, but then we should also checking_assert !processing_template_decl in > b_o_t_e. Unfortunately we'd trigger that assert from maybe_constant_value, which potentially calls b_o_t_e with processing_template_decl set. > > Jason > > > > ? > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > > > PR c++/108116 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * init.cc (get_nsdmi): Clear processing_template_decl before > > > > processing the non-templated initializer. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/cpp0x/nsdmi-template24.C: New test. > > > > --- > > > > gcc/cp/init.cc | 8 ++++++- > > > > gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 > > > > +++++++++++++++++++ > > > > 2 files changed, 29 insertions(+), 1 deletion(-) > > > > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > > > > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > > > > index 73e6547c076..c4345ebdaea 100644 > > > > --- a/gcc/cp/init.cc > > > > +++ b/gcc/cp/init.cc > > > > @@ -561,7 +561,8 @@ perform_target_ctor (tree init) > > > > return init; > > > > } > > > > -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ > > > > +/* Return the non-static data initializer for FIELD_DECL MEMBER. > > > > + The initializer returned is always non-templated. */ > > > > static GTY((cache)) decl_tree_cache_map *nsdmi_inst; > > > > @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, > > > > tsubst_flags_t > > > > complain) > > > > current_class_ptr = build_address (current_class_ref); > > > > } > > > > + /* Since INIT is always non-templated clear > > > > processing_template_decl > > > > + before processing it so that we don't interleave templated and > > > > + non-templated trees. */ > > > > + processing_template_decl_sentinel ptds; > > > > + > > > > /* Strip redundant TARGET_EXPR so we don't need to remap it, and > > > > so the aggregate init code below will see a CONSTRUCTOR. */ > > > > bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); > > > > diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > new file mode 100644 > > > > index 00000000000..202c67d7321 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > @@ -0,0 +1,22 @@ > > > > +// PR c++/108116 > > > > +// { dg-do compile { target c++11 } } > > > > + > > > > +#include <initializer_list> > > > > + > > > > +struct A { > > > > + A(int); > > > > + ~A(); > > > > +}; > > > > + > > > > +struct B { > > > > + B(std::initializer_list<A>); > > > > +}; > > > > + > > > > +struct C { > > > > + B m{0}; > > > > +}; > > > > + > > > > +template<class> > > > > +void f() { > > > > + C c = C{}; > > > > +}; > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-22 21:41 ` Patrick Palka @ 2022-12-22 22:03 ` Jason Merrill 2022-12-22 22:41 ` Patrick Palka 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2022-12-22 22:03 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 12/22/22 16:41, Patrick Palka wrote: > On Thu, 22 Dec 2022, Jason Merrill wrote: > >> On 12/22/22 11:31, Patrick Palka wrote: >>> On Wed, 21 Dec 2022, Jason Merrill wrote: >>> >>>> On 12/21/22 09:52, Patrick Palka wrote: >>>>> Here during ahead of time checking of C{}, we indirectly call get_nsdmi >>>>> for C::m from finish_compound_literal, which in turn calls >>>>> break_out_target_exprs for C::m's (non-templated) initializer, during >>>>> which we end up building a call to A::~A and checking expr_noexcept_p >>>>> for it (from build_vec_delete_1). But this is all done with >>>>> processing_template_decl set, so the built A::~A call is templated >>>>> (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which >>>>> expr_noexcept_p doesn't expect and we crash. >>>>> >>>>> In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a >>>>> expr_noexcept_p call with !processing_template_decl, which works here >>>>> too. But it seems to me since the initializer we obtain in get_nsdmi is >>>>> always non-templated, it should be calling break_out_target_exprs with >>>>> processing_template_decl cleared since otherwise the function might end >>>>> up mixing templated and non-templated trees. >>>>> >>>>> I'm not sure about this though, perhaps this is not the best fix here. >>>>> Alternatively, when processing_template_decl we could make get_nsdmi >>>>> avoid calling break_out_target_exprs at all or something. Additionally, >>>>> perhaps break_out_target_exprs should be a no-op more generally when >>>>> processing_template_decl since we shouldn't see any TARGET_EXPRs inside >>>>> a template? >>>> >>>> Hmm. >>>> >>>> Any time we would call break_out_target_exprs we're dealing with >>>> non-dependent >>>> expressions; if we're in a template, we're building up an initializer or a >>>> call that we'll soon throw away, just for the purpose of checking or type >>>> computation. >>>> >>>> Furthermore, as you say, the argument is always a non-template tree, >>>> whether >>>> in get_nsdmi or convert_default_arg. So having processing_template_decl >>>> cleared would be correct. >>>> >>>> I don't think we can get away with not calling break_out_target_exprs at >>>> all >>>> in a template; if nothing else, we would lose immediate invocation >>>> expansion. >>>> However, we could probably skip the bot_manip tree walk, which should >>>> avoid >>>> the problem. >>>> >>>> Either way we end up returning non-template trees, as we do now, and >>>> callers >>>> have to deal with transient CONSTRUCTORs containing such (as we do in >>>> massage_init_elt). >>> >>> Ah I see, makes sense. >>> >>>> >>>> Does convert_default_arg not run into the same problem, e.g. when calling >>>> >>>> void g(B = {0}); >>> >>> In practice it seems not, because we don't call convert_default_arg >>> when processing_template_decl is set (verified with an assert to >>> that effect). In build_over_call for example we exit early when >>> processing_template_decl is set, and return a templated CALL_EXPR >>> that doesn't include default arguments at all. A consequence of >>> this is that we don't reject ahead of time a call that would use >>> an ill-formed dependent default argument, e.g. >>> >>> template<class T> >>> void g(B = T{0}); >>> >>> template<class> >>> void f() { >>> g<void>(); >>> } >>> >>> since the default argument instantiation would be the responsibility >>> of convert_default_arg. >>> >>> Thinking hypothetically here, if we do in the future want to include default >>> arguments in the templated form of a CALL_EXPR, >> >> We definitely do not want to; the templated form should be as close as >> possible to the source. > > Ah, sounds good. > >> >> We might want to perform non-dependent conversions to get any errors (such as >> this one) before throwing away the result. Which would be parallel to what we >> currently do in calling get_nsdmi, and would want the same behavior. > > *nod* > >> >>> [snip] >> >>> shall we go with the original approach to clear >>> processing_template_decl directly from get_nsdmi? >> >> OK, but then we should also checking_assert !processing_template_decl in >> b_o_t_e. > > Unfortunately we'd trigger that assert from maybe_constant_value, which > potentially calls b_o_t_e with processing_template_decl set. maybe_constant_value could also clear processing_template_decl; entries in cv_cache are non-templated. >>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu. >>>>> >>>>> PR c++/108116 >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * init.cc (get_nsdmi): Clear processing_template_decl before >>>>> processing the non-templated initializer. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * g++.dg/cpp0x/nsdmi-template24.C: New test. >>>>> --- >>>>> gcc/cp/init.cc | 8 ++++++- >>>>> gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 >>>>> +++++++++++++++++++ >>>>> 2 files changed, 29 insertions(+), 1 deletion(-) >>>>> create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>>>> >>>>> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc >>>>> index 73e6547c076..c4345ebdaea 100644 >>>>> --- a/gcc/cp/init.cc >>>>> +++ b/gcc/cp/init.cc >>>>> @@ -561,7 +561,8 @@ perform_target_ctor (tree init) >>>>> return init; >>>>> } >>>>> -/* Return the non-static data initializer for FIELD_DECL MEMBER. */ >>>>> +/* Return the non-static data initializer for FIELD_DECL MEMBER. >>>>> + The initializer returned is always non-templated. */ >>>>> static GTY((cache)) decl_tree_cache_map *nsdmi_inst; >>>>> @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, >>>>> tsubst_flags_t >>>>> complain) >>>>> current_class_ptr = build_address (current_class_ref); >>>>> } >>>>> + /* Since INIT is always non-templated clear >>>>> processing_template_decl >>>>> + before processing it so that we don't interleave templated and >>>>> + non-templated trees. */ >>>>> + processing_template_decl_sentinel ptds; >>>>> + >>>>> /* Strip redundant TARGET_EXPR so we don't need to remap it, and >>>>> so the aggregate init code below will see a CONSTRUCTOR. */ >>>>> bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); >>>>> diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>>>> b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>>>> new file mode 100644 >>>>> index 00000000000..202c67d7321 >>>>> --- /dev/null >>>>> +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C >>>>> @@ -0,0 +1,22 @@ >>>>> +// PR c++/108116 >>>>> +// { dg-do compile { target c++11 } } >>>>> + >>>>> +#include <initializer_list> >>>>> + >>>>> +struct A { >>>>> + A(int); >>>>> + ~A(); >>>>> +}; >>>>> + >>>>> +struct B { >>>>> + B(std::initializer_list<A>); >>>>> +}; >>>>> + >>>>> +struct C { >>>>> + B m{0}; >>>>> +}; >>>>> + >>>>> +template<class> >>>>> +void f() { >>>>> + C c = C{}; >>>>> +}; >>>> >>>> >>> >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-22 22:03 ` Jason Merrill @ 2022-12-22 22:41 ` Patrick Palka 2022-12-23 15:48 ` Patrick Palka 0 siblings, 1 reply; 10+ messages in thread From: Patrick Palka @ 2022-12-22 22:41 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Thu, 22 Dec 2022, Jason Merrill wrote: > On 12/22/22 16:41, Patrick Palka wrote: > > On Thu, 22 Dec 2022, Jason Merrill wrote: > > > > > On 12/22/22 11:31, Patrick Palka wrote: > > > > On Wed, 21 Dec 2022, Jason Merrill wrote: > > > > > > > > > On 12/21/22 09:52, Patrick Palka wrote: > > > > > > Here during ahead of time checking of C{}, we indirectly call > > > > > > get_nsdmi > > > > > > for C::m from finish_compound_literal, which in turn calls > > > > > > break_out_target_exprs for C::m's (non-templated) initializer, > > > > > > during > > > > > > which we end up building a call to A::~A and checking > > > > > > expr_noexcept_p > > > > > > for it (from build_vec_delete_1). But this is all done with > > > > > > processing_template_decl set, so the built A::~A call is templated > > > > > > (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which > > > > > > expr_noexcept_p doesn't expect and we crash. > > > > > > > > > > > > In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a > > > > > > expr_noexcept_p call with !processing_template_decl, which works > > > > > > here > > > > > > too. But it seems to me since the initializer we obtain in > > > > > > get_nsdmi is > > > > > > always non-templated, it should be calling break_out_target_exprs > > > > > > with > > > > > > processing_template_decl cleared since otherwise the function might > > > > > > end > > > > > > up mixing templated and non-templated trees. > > > > > > > > > > > > I'm not sure about this though, perhaps this is not the best fix > > > > > > here. > > > > > > Alternatively, when processing_template_decl we could make get_nsdmi > > > > > > avoid calling break_out_target_exprs at all or something. > > > > > > Additionally, > > > > > > perhaps break_out_target_exprs should be a no-op more generally when > > > > > > processing_template_decl since we shouldn't see any TARGET_EXPRs > > > > > > inside > > > > > > a template? > > > > > > > > > > Hmm. > > > > > > > > > > Any time we would call break_out_target_exprs we're dealing with > > > > > non-dependent > > > > > expressions; if we're in a template, we're building up an initializer > > > > > or a > > > > > call that we'll soon throw away, just for the purpose of checking or > > > > > type > > > > > computation. > > > > > > > > > > Furthermore, as you say, the argument is always a non-template tree, > > > > > whether > > > > > in get_nsdmi or convert_default_arg. So having > > > > > processing_template_decl > > > > > cleared would be correct. > > > > > > > > > > I don't think we can get away with not calling break_out_target_exprs > > > > > at > > > > > all > > > > > in a template; if nothing else, we would lose immediate invocation > > > > > expansion. > > > > > However, we could probably skip the bot_manip tree walk, which should > > > > > avoid > > > > > the problem. > > > > > > > > > > Either way we end up returning non-template trees, as we do now, and > > > > > callers > > > > > have to deal with transient CONSTRUCTORs containing such (as we do in > > > > > massage_init_elt). > > > > > > > > Ah I see, makes sense. > > > > > > > > > > > > > > Does convert_default_arg not run into the same problem, e.g. when > > > > > calling > > > > > > > > > > void g(B = {0}); > > > > > > > > In practice it seems not, because we don't call convert_default_arg > > > > when processing_template_decl is set (verified with an assert to > > > > that effect). In build_over_call for example we exit early when > > > > processing_template_decl is set, and return a templated CALL_EXPR > > > > that doesn't include default arguments at all. A consequence of > > > > this is that we don't reject ahead of time a call that would use > > > > an ill-formed dependent default argument, e.g. > > > > > > > > template<class T> > > > > void g(B = T{0}); > > > > > > > > template<class> > > > > void f() { > > > > g<void>(); > > > > } > > > > > > > > since the default argument instantiation would be the responsibility > > > > of convert_default_arg. > > > > > > > > Thinking hypothetically here, if we do in the future want to include > > > > default > > > > arguments in the templated form of a CALL_EXPR, > > > > > > We definitely do not want to; the templated form should be as close as > > > possible to the source. > > > > Ah, sounds good. > > > > > > > > We might want to perform non-dependent conversions to get any errors (such > > > as > > > this one) before throwing away the result. Which would be parallel to > > > what we > > > currently do in calling get_nsdmi, and would want the same behavior. > > > > *nod* > > > > > > > > > [snip] > > > > > > > shall we go with the original approach to clear > > > > processing_template_decl directly from get_nsdmi? > > > > > > OK, but then we should also checking_assert !processing_template_decl in > > > b_o_t_e. > > > > Unfortunately we'd trigger that assert from maybe_constant_value, which > > potentially calls b_o_t_e with processing_template_decl set. > > maybe_constant_value could also clear processing_template_decl; entries in > cv_cache are non-templated. Aha! I'll try that. > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > > > > > > > PR c++/108116 > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > * init.cc (get_nsdmi): Clear processing_template_decl before > > > > > > processing the non-templated initializer. > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > * g++.dg/cpp0x/nsdmi-template24.C: New test. > > > > > > --- > > > > > > gcc/cp/init.cc | 8 ++++++- > > > > > > gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 > > > > > > +++++++++++++++++++ > > > > > > 2 files changed, 29 insertions(+), 1 deletion(-) > > > > > > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > > > > > > > > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > > > > > > index 73e6547c076..c4345ebdaea 100644 > > > > > > --- a/gcc/cp/init.cc > > > > > > +++ b/gcc/cp/init.cc > > > > > > @@ -561,7 +561,8 @@ perform_target_ctor (tree init) > > > > > > return init; > > > > > > } > > > > > > -/* Return the non-static data initializer for FIELD_DECL > > > > > > MEMBER. */ > > > > > > +/* Return the non-static data initializer for FIELD_DECL MEMBER. > > > > > > + The initializer returned is always non-templated. */ > > > > > > static GTY((cache)) decl_tree_cache_map *nsdmi_inst; > > > > > > @@ -670,6 +671,11 @@ get_nsdmi (tree member, bool in_ctor, > > > > > > tsubst_flags_t > > > > > > complain) > > > > > > current_class_ptr = build_address (current_class_ref); > > > > > > } > > > > > > + /* Since INIT is always non-templated clear > > > > > > processing_template_decl > > > > > > + before processing it so that we don't interleave templated and > > > > > > + non-templated trees. */ > > > > > > + processing_template_decl_sentinel ptds; > > > > > > + > > > > > > /* Strip redundant TARGET_EXPR so we don't need to remap it, > > > > > > and > > > > > > so the aggregate init code below will see a CONSTRUCTOR. > > > > > > */ > > > > > > bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); > > > > > > diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > > > b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > > > new file mode 100644 > > > > > > index 00000000000..202c67d7321 > > > > > > --- /dev/null > > > > > > +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > > > > > @@ -0,0 +1,22 @@ > > > > > > +// PR c++/108116 > > > > > > +// { dg-do compile { target c++11 } } > > > > > > + > > > > > > +#include <initializer_list> > > > > > > + > > > > > > +struct A { > > > > > > + A(int); > > > > > > + ~A(); > > > > > > +}; > > > > > > + > > > > > > +struct B { > > > > > > + B(std::initializer_list<A>); > > > > > > +}; > > > > > > + > > > > > > +struct C { > > > > > > + B m{0}; > > > > > > +}; > > > > > > + > > > > > > +template<class> > > > > > > +void f() { > > > > > > + C c = C{}; > > > > > > +}; > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-22 22:41 ` Patrick Palka @ 2022-12-23 15:48 ` Patrick Palka 2022-12-23 16:04 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Patrick Palka @ 2022-12-23 15:48 UTC (permalink / raw) To: Patrick Palka; +Cc: Jason Merrill, gcc-patches On Thu, 22 Dec 2022, Patrick Palka wrote: > On Thu, 22 Dec 2022, Jason Merrill wrote: > > > On 12/22/22 16:41, Patrick Palka wrote: > > > On Thu, 22 Dec 2022, Jason Merrill wrote: > > > > > > > On 12/22/22 11:31, Patrick Palka wrote: > > > > > On Wed, 21 Dec 2022, Jason Merrill wrote: > > > > > > > > > > > On 12/21/22 09:52, Patrick Palka wrote: > > > > > > > Here during ahead of time checking of C{}, we indirectly call > > > > > > > get_nsdmi > > > > > > > for C::m from finish_compound_literal, which in turn calls > > > > > > > break_out_target_exprs for C::m's (non-templated) initializer, > > > > > > > during > > > > > > > which we end up building a call to A::~A and checking > > > > > > > expr_noexcept_p > > > > > > > for it (from build_vec_delete_1). But this is all done with > > > > > > > processing_template_decl set, so the built A::~A call is templated > > > > > > > (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which > > > > > > > expr_noexcept_p doesn't expect and we crash. > > > > > > > > > > > > > > In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a > > > > > > > expr_noexcept_p call with !processing_template_decl, which works > > > > > > > here > > > > > > > too. But it seems to me since the initializer we obtain in > > > > > > > get_nsdmi is > > > > > > > always non-templated, it should be calling break_out_target_exprs > > > > > > > with > > > > > > > processing_template_decl cleared since otherwise the function might > > > > > > > end > > > > > > > up mixing templated and non-templated trees. > > > > > > > > > > > > > > I'm not sure about this though, perhaps this is not the best fix > > > > > > > here. > > > > > > > Alternatively, when processing_template_decl we could make get_nsdmi > > > > > > > avoid calling break_out_target_exprs at all or something. > > > > > > > Additionally, > > > > > > > perhaps break_out_target_exprs should be a no-op more generally when > > > > > > > processing_template_decl since we shouldn't see any TARGET_EXPRs > > > > > > > inside > > > > > > > a template? > > > > > > > > > > > > Hmm. > > > > > > > > > > > > Any time we would call break_out_target_exprs we're dealing with > > > > > > non-dependent > > > > > > expressions; if we're in a template, we're building up an initializer > > > > > > or a > > > > > > call that we'll soon throw away, just for the purpose of checking or > > > > > > type > > > > > > computation. > > > > > > > > > > > > Furthermore, as you say, the argument is always a non-template tree, > > > > > > whether > > > > > > in get_nsdmi or convert_default_arg. So having > > > > > > processing_template_decl > > > > > > cleared would be correct. > > > > > > > > > > > > I don't think we can get away with not calling break_out_target_exprs > > > > > > at > > > > > > all > > > > > > in a template; if nothing else, we would lose immediate invocation > > > > > > expansion. > > > > > > However, we could probably skip the bot_manip tree walk, which should > > > > > > avoid > > > > > > the problem. > > > > > > > > > > > > Either way we end up returning non-template trees, as we do now, and > > > > > > callers > > > > > > have to deal with transient CONSTRUCTORs containing such (as we do in > > > > > > massage_init_elt). > > > > > > > > > > Ah I see, makes sense. > > > > > > > > > > > > > > > > > Does convert_default_arg not run into the same problem, e.g. when > > > > > > calling > > > > > > > > > > > > void g(B = {0}); > > > > > > > > > > In practice it seems not, because we don't call convert_default_arg > > > > > when processing_template_decl is set (verified with an assert to > > > > > that effect). In build_over_call for example we exit early when > > > > > processing_template_decl is set, and return a templated CALL_EXPR > > > > > that doesn't include default arguments at all. A consequence of > > > > > this is that we don't reject ahead of time a call that would use > > > > > an ill-formed dependent default argument, e.g. > > > > > > > > > > template<class T> > > > > > void g(B = T{0}); > > > > > > > > > > template<class> > > > > > void f() { > > > > > g<void>(); > > > > > } > > > > > > > > > > since the default argument instantiation would be the responsibility > > > > > of convert_default_arg. > > > > > > > > > > Thinking hypothetically here, if we do in the future want to include > > > > > default > > > > > arguments in the templated form of a CALL_EXPR, > > > > > > > > We definitely do not want to; the templated form should be as close as > > > > possible to the source. > > > > > > Ah, sounds good. > > > > > > > > > > > We might want to perform non-dependent conversions to get any errors (such > > > > as > > > > this one) before throwing away the result. Which would be parallel to > > > > what we > > > > currently do in calling get_nsdmi, and would want the same behavior. > > > > > > *nod* > > > > > > > > > > > > [snip] > > > > > > > > > shall we go with the original approach to clear > > > > > processing_template_decl directly from get_nsdmi? > > > > > > > > OK, but then we should also checking_assert !processing_template_decl in > > > > b_o_t_e. > > > > > > Unfortunately we'd trigger that assert from maybe_constant_value, which > > > potentially calls b_o_t_e with processing_template_decl set. > > > > maybe_constant_value could also clear processing_template_decl; entries in > > cv_cache are non-templated. > > Aha! I'll try that. How does this look? Bootstrapped and regtested on x86_64-pc-linux-gnu. -- >8 -- Subject: [PATCH] c++: get_nsdmi in template context [PR108116] Here during ahead of time checking of C{}, we indirectly call get_nsdmi for C::m from finish_compound_literal, which in turn calls break_out_target_exprs for C::m's (non-templated) initializer, during which we build a call to A::~A and check expr_noexcept_p for it (from build_vec_delete_1). But this is all done with processing_template_decl set, so the built A::~A call is templated (whose form was recently changed by r12-6897-gdec8d0e5fa00ceb2) which expr_noexcept_p doesn't expect, and we crash. This patch fixes this by clearing processing_template_decl before the call to break_out_target_exprs from get_nsdmi. And since it more generally seems we shouldn't be seeing (or producing) non-templated trees from break_out_target_exprs, this patch also adds an assert to that effect. PR c++/108116 gcc/cp/ChangeLog: * constexpr.cc (maybe_constant_value): Clear processing_template_decl before calling break_out_target_exprs. * init.cc (get_nsdmi): Likewise. * tree.cc (break_out_target_exprs): Assert processing_template_decl is cleared. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/nsdmi-template24.C: New test. --- gcc/cp/constexpr.cc | 4 ++++ gcc/cp/init.cc | 4 ++++ gcc/cp/tree.cc | 4 ++++ gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ 4 files changed, 34 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index d99c49bdbe2..414af7a6d4c 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -8507,6 +8507,10 @@ maybe_constant_value (tree t, tree decl /* = NULL_TREE */, r = *cached; if (r != t) { + /* Clear processing_template_decl for sake of break_out_target_exprs; + entries in the cv_cache are non-templated. */ + processing_template_decl_sentinel ptds; + r = break_out_target_exprs (r, /*clear_loc*/true); protected_set_expr_location (r, EXPR_LOCATION (t)); } diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc index 73e6547c076..b49a7ca9169 100644 --- a/gcc/cp/init.cc +++ b/gcc/cp/init.cc @@ -670,6 +670,10 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain) current_class_ptr = build_address (current_class_ref); } + /* Clear processing_template_decl for sake of break_out_target_exprs; + INIT is always non-templated. */ + processing_template_decl_sentinel ptds; + /* Strip redundant TARGET_EXPR so we don't need to remap it, and so the aggregate init code below will see a CONSTRUCTOR. */ bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc index 33bde16f128..faf01616f87 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -3342,6 +3342,10 @@ break_out_target_exprs (tree t, bool clear_location /* = false */) static int target_remap_count; static splay_tree target_remap; + /* We shouldn't be called on templated trees, nor do we want to + produce them. */ + gcc_checking_assert (!processing_template_decl); + if (!target_remap_count++) target_remap = splay_tree_new (splay_tree_compare_pointers, /*splay_tree_delete_key_fn=*/NULL, diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C new file mode 100644 index 00000000000..202c67d7321 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C @@ -0,0 +1,22 @@ +// PR c++/108116 +// { dg-do compile { target c++11 } } + +#include <initializer_list> + +struct A { + A(int); + ~A(); +}; + +struct B { + B(std::initializer_list<A>); +}; + +struct C { + B m{0}; +}; + +template<class> +void f() { + C c = C{}; +}; -- 2.39.0.95.g7c2ef319c5 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: get_nsdmi in template context [PR108116] 2022-12-23 15:48 ` Patrick Palka @ 2022-12-23 16:04 ` Jason Merrill 0 siblings, 0 replies; 10+ messages in thread From: Jason Merrill @ 2022-12-23 16:04 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 12/23/22 10:48, Patrick Palka wrote: > On Thu, 22 Dec 2022, Patrick Palka wrote: > >> On Thu, 22 Dec 2022, Jason Merrill wrote: >> >>> On 12/22/22 16:41, Patrick Palka wrote: >>>> On Thu, 22 Dec 2022, Jason Merrill wrote: >>>> >>>>> On 12/22/22 11:31, Patrick Palka wrote: >>>>>> On Wed, 21 Dec 2022, Jason Merrill wrote: >>>>>> >>>>>>> On 12/21/22 09:52, Patrick Palka wrote: >>>>>>>> Here during ahead of time checking of C{}, we indirectly call >>>>>>>> get_nsdmi >>>>>>>> for C::m from finish_compound_literal, which in turn calls >>>>>>>> break_out_target_exprs for C::m's (non-templated) initializer, >>>>>>>> during >>>>>>>> which we end up building a call to A::~A and checking >>>>>>>> expr_noexcept_p >>>>>>>> for it (from build_vec_delete_1). But this is all done with >>>>>>>> processing_template_decl set, so the built A::~A call is templated >>>>>>>> (whose form r12-6897-gdec8d0e5fa00ceb2 recently changed) which >>>>>>>> expr_noexcept_p doesn't expect and we crash. >>>>>>>> >>>>>>>> In r10-6183-g20afdcd3698275 we fixed a similar issue by guarding a >>>>>>>> expr_noexcept_p call with !processing_template_decl, which works >>>>>>>> here >>>>>>>> too. But it seems to me since the initializer we obtain in >>>>>>>> get_nsdmi is >>>>>>>> always non-templated, it should be calling break_out_target_exprs >>>>>>>> with >>>>>>>> processing_template_decl cleared since otherwise the function might >>>>>>>> end >>>>>>>> up mixing templated and non-templated trees. >>>>>>>> >>>>>>>> I'm not sure about this though, perhaps this is not the best fix >>>>>>>> here. >>>>>>>> Alternatively, when processing_template_decl we could make get_nsdmi >>>>>>>> avoid calling break_out_target_exprs at all or something. >>>>>>>> Additionally, >>>>>>>> perhaps break_out_target_exprs should be a no-op more generally when >>>>>>>> processing_template_decl since we shouldn't see any TARGET_EXPRs >>>>>>>> inside >>>>>>>> a template? >>>>>>> >>>>>>> Hmm. >>>>>>> >>>>>>> Any time we would call break_out_target_exprs we're dealing with >>>>>>> non-dependent >>>>>>> expressions; if we're in a template, we're building up an initializer >>>>>>> or a >>>>>>> call that we'll soon throw away, just for the purpose of checking or >>>>>>> type >>>>>>> computation. >>>>>>> >>>>>>> Furthermore, as you say, the argument is always a non-template tree, >>>>>>> whether >>>>>>> in get_nsdmi or convert_default_arg. So having >>>>>>> processing_template_decl >>>>>>> cleared would be correct. >>>>>>> >>>>>>> I don't think we can get away with not calling break_out_target_exprs >>>>>>> at >>>>>>> all >>>>>>> in a template; if nothing else, we would lose immediate invocation >>>>>>> expansion. >>>>>>> However, we could probably skip the bot_manip tree walk, which should >>>>>>> avoid >>>>>>> the problem. >>>>>>> >>>>>>> Either way we end up returning non-template trees, as we do now, and >>>>>>> callers >>>>>>> have to deal with transient CONSTRUCTORs containing such (as we do in >>>>>>> massage_init_elt). >>>>>> >>>>>> Ah I see, makes sense. >>>>>> >>>>>>> >>>>>>> Does convert_default_arg not run into the same problem, e.g. when >>>>>>> calling >>>>>>> >>>>>>> void g(B = {0}); >>>>>> >>>>>> In practice it seems not, because we don't call convert_default_arg >>>>>> when processing_template_decl is set (verified with an assert to >>>>>> that effect). In build_over_call for example we exit early when >>>>>> processing_template_decl is set, and return a templated CALL_EXPR >>>>>> that doesn't include default arguments at all. A consequence of >>>>>> this is that we don't reject ahead of time a call that would use >>>>>> an ill-formed dependent default argument, e.g. >>>>>> >>>>>> template<class T> >>>>>> void g(B = T{0}); >>>>>> >>>>>> template<class> >>>>>> void f() { >>>>>> g<void>(); >>>>>> } >>>>>> >>>>>> since the default argument instantiation would be the responsibility >>>>>> of convert_default_arg. >>>>>> >>>>>> Thinking hypothetically here, if we do in the future want to include >>>>>> default >>>>>> arguments in the templated form of a CALL_EXPR, >>>>> >>>>> We definitely do not want to; the templated form should be as close as >>>>> possible to the source. >>>> >>>> Ah, sounds good. >>>> >>>>> >>>>> We might want to perform non-dependent conversions to get any errors (such >>>>> as >>>>> this one) before throwing away the result. Which would be parallel to >>>>> what we >>>>> currently do in calling get_nsdmi, and would want the same behavior. >>>> >>>> *nod* >>>> >>>>> >>>>>> [snip] >>>>> >>>>>> shall we go with the original approach to clear >>>>>> processing_template_decl directly from get_nsdmi? >>>>> >>>>> OK, but then we should also checking_assert !processing_template_decl in >>>>> b_o_t_e. >>>> >>>> Unfortunately we'd trigger that assert from maybe_constant_value, which >>>> potentially calls b_o_t_e with processing_template_decl set. >>> >>> maybe_constant_value could also clear processing_template_decl; entries in >>> cv_cache are non-templated. >> >> Aha! I'll try that. > > How does this look? Bootstrapped and regtested on x86_64-pc-linux-gnu. OK. > -- >8 -- > > Subject: [PATCH] c++: get_nsdmi in template context [PR108116] > > Here during ahead of time checking of C{}, we indirectly call get_nsdmi > for C::m from finish_compound_literal, which in turn calls > break_out_target_exprs for C::m's (non-templated) initializer, during > which we build a call to A::~A and check expr_noexcept_p for it (from > build_vec_delete_1). But this is all done with processing_template_decl > set, so the built A::~A call is templated (whose form was recently > changed by r12-6897-gdec8d0e5fa00ceb2) which expr_noexcept_p doesn't > expect, and we crash. > > This patch fixes this by clearing processing_template_decl before > the call to break_out_target_exprs from get_nsdmi. And since it more > generally seems we shouldn't be seeing (or producing) non-templated > trees from break_out_target_exprs, this patch also adds an assert to > that effect. > > PR c++/108116 > > gcc/cp/ChangeLog: > > * constexpr.cc (maybe_constant_value): Clear > processing_template_decl before calling break_out_target_exprs. > * init.cc (get_nsdmi): Likewise. > * tree.cc (break_out_target_exprs): Assert processing_template_decl > is cleared. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/nsdmi-template24.C: New test. > --- > gcc/cp/constexpr.cc | 4 ++++ > gcc/cp/init.cc | 4 ++++ > gcc/cp/tree.cc | 4 ++++ > gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C | 22 +++++++++++++++++++ > 4 files changed, 34 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index d99c49bdbe2..414af7a6d4c 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -8507,6 +8507,10 @@ maybe_constant_value (tree t, tree decl /* = NULL_TREE */, > r = *cached; > if (r != t) > { > + /* Clear processing_template_decl for sake of break_out_target_exprs; > + entries in the cv_cache are non-templated. */ > + processing_template_decl_sentinel ptds; > + > r = break_out_target_exprs (r, /*clear_loc*/true); > protected_set_expr_location (r, EXPR_LOCATION (t)); > } > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > index 73e6547c076..b49a7ca9169 100644 > --- a/gcc/cp/init.cc > +++ b/gcc/cp/init.cc > @@ -670,6 +670,10 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain) > current_class_ptr = build_address (current_class_ref); > } > > + /* Clear processing_template_decl for sake of break_out_target_exprs; > + INIT is always non-templated. */ > + processing_template_decl_sentinel ptds; > + > /* Strip redundant TARGET_EXPR so we don't need to remap it, and > so the aggregate init code below will see a CONSTRUCTOR. */ > bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init)); > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc > index 33bde16f128..faf01616f87 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -3342,6 +3342,10 @@ break_out_target_exprs (tree t, bool clear_location /* = false */) > static int target_remap_count; > static splay_tree target_remap; > > + /* We shouldn't be called on templated trees, nor do we want to > + produce them. */ > + gcc_checking_assert (!processing_template_decl); > + > if (!target_remap_count++) > target_remap = splay_tree_new (splay_tree_compare_pointers, > /*splay_tree_delete_key_fn=*/NULL, > diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > new file mode 100644 > index 00000000000..202c67d7321 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-template24.C > @@ -0,0 +1,22 @@ > +// PR c++/108116 > +// { dg-do compile { target c++11 } } > + > +#include <initializer_list> > + > +struct A { > + A(int); > + ~A(); > +}; > + > +struct B { > + B(std::initializer_list<A>); > +}; > + > +struct C { > + B m{0}; > +}; > + > +template<class> > +void f() { > + C c = C{}; > +}; ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-12-23 16:04 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-21 14:52 [PATCH] c++: get_nsdmi in template context [PR108116] Patrick Palka 2022-12-21 14:56 ` Patrick Palka 2022-12-21 21:48 ` Jason Merrill 2022-12-22 16:31 ` Patrick Palka 2022-12-22 21:33 ` Jason Merrill 2022-12-22 21:41 ` Patrick Palka 2022-12-22 22:03 ` Jason Merrill 2022-12-22 22:41 ` Patrick Palka 2022-12-23 15:48 ` Patrick Palka 2022-12-23 16:04 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).