| Message ID | or5zw6kww0.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [C++,PR,c++/88146] do not crash synthesizing inherited ctor(...) | expand |
On Dec 6, 2018, Alexandre Oliva <aoliva@redhat.com> wrote: > Regstrapped on x86_64- and i686-linux-gnu, mistakenly along with a patch > with a known regression, and got only that known regression. Retesting > without it. Ok to install? Ping? That round of retesting confirmed no regressions. https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00424.html > for gcc/cp/ChangeLog > PR c++/88146 > * method.c (do_build_copy_constructor): Do not crash with > ellipsis-only parm list. > (synthesize_method): Retain location of inherited ctor. > for gcc/testsuite/ChangeLog > PR c++/88146 > * g++.dg/cpp0x/inh-ctor32.C: New.
On 12/6/18 7:23 PM, Alexandre Oliva wrote: > This patch started out from the testcase in PR88146, that attempted to > synthesize an inherited ctor without any args before a varargs > ellipsis and crashed while at that, because of the unguarded > dereferencing of the parm type list, that usually contains a > terminator. The terminator is not there for varargs functions, > however, and without any other args, we ended up dereferencing a NULL > pointer. Oops. > > Guarding the accesses there was easy, but I missed the sorry message > we got in other testcases that passed arguments through the ellipsis > in inherited ctors. I put a check in, and noticed the inherited ctors > were synthesized with the location assigned to the class name, > although they were initially assigned the location of the using > declaration. I decided the latter was better, and arranged for the > better location to be retained. > > Further investigation revealed the lack of a sorry message had to do > with the call being in a non-evaluated context, in this case, a > noexcept expression. The sorry would be correctly reported in other > contexts, so I rolled back the check I'd added, but retained the > source location improvement. > > I was still concerned about issuing sorry messages while instantiating > template ctors even in non-evaluated contexts, e.g., if a template > ctor had a base initializer that used an inherited ctor with enough > arguments that they'd go through an ellipsis. I wanted to defer the > instantiation of such template ctors, but that would have been wrong > for constexpr template ctors, and already done for non-constexpr ones. > So, I just consolidated multiple test variants into a single testcase > that explores and explains various of the possibilities I thought of. > > Regstrapped on x86_64- and i686-linux-gnu, mistakenly along with a patch > with a known regression, and got only that known regression. Retesting > without it. Ok to install? > > > for gcc/cp/ChangeLog > > PR c++/88146 > * method.c (do_build_copy_constructor): Do not crash with > ellipsis-only parm list. > (synthesize_method): Retain location of inherited ctor. > > for gcc/testsuite/ChangeLog > > PR c++/88146 > * g++.dg/cpp0x/inh-ctor32.C: New. > --- > gcc/cp/method.c | 9 + > gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C | 229 +++++++++++++++++++++++++++++++ > 2 files changed, 234 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C > > diff --git a/gcc/cp/method.c b/gcc/cp/method.c > index fd023e200538..41d609fb1de6 100644 > --- a/gcc/cp/method.c > +++ b/gcc/cp/method.c > @@ -643,7 +643,7 @@ do_build_copy_constructor (tree fndecl) > bool trivial = trivial_fn_p (fndecl); > tree inh = DECL_INHERITED_CTOR (fndecl); > > - if (!inh) > + if (parm && !inh) > parm = convert_from_reference (parm); If inh is false, we're a copy constructor, which always has a parm, so this hunk seems unnecessary. > > if (trivial) > @@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl) > { > tree fields = TYPE_FIELDS (current_class_type); > tree member_init_list = NULL_TREE; > - int cvquals = cp_type_quals (TREE_TYPE (parm)); > + int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0; This could also check !inh. Jason
On 12/14/18 3:42 PM, Jason Merrill wrote: > On 12/6/18 7:23 PM, Alexandre Oliva wrote: >> This patch started out from the testcase in PR88146, that attempted to >> synthesize an inherited ctor without any args before a varargs >> ellipsis and crashed while at that, because of the unguarded >> dereferencing of the parm type list, that usually contains a >> terminator. The terminator is not there for varargs functions, >> however, and without any other args, we ended up dereferencing a NULL >> pointer. Oops. >> >> Guarding the accesses there was easy, but I missed the sorry message >> we got in other testcases that passed arguments through the ellipsis >> in inherited ctors. I put a check in, and noticed the inherited ctors >> were synthesized with the location assigned to the class name, >> although they were initially assigned the location of the using >> declaration. I decided the latter was better, and arranged for the >> better location to be retained. >> >> Further investigation revealed the lack of a sorry message had to do >> with the call being in a non-evaluated context, in this case, a >> noexcept expression. The sorry would be correctly reported in other >> contexts, so I rolled back the check I'd added, but retained the >> source location improvement. >> >> I was still concerned about issuing sorry messages while instantiating >> template ctors even in non-evaluated contexts, e.g., if a template >> ctor had a base initializer that used an inherited ctor with enough >> arguments that they'd go through an ellipsis. I wanted to defer the >> instantiation of such template ctors, but that would have been wrong >> for constexpr template ctors, and already done for non-constexpr ones. >> So, I just consolidated multiple test variants into a single testcase >> that explores and explains various of the possibilities I thought of. >> >> Regstrapped on x86_64- and i686-linux-gnu, mistakenly along with a patch >> with a known regression, and got only that known regression. Retesting >> without it. Ok to install? >> >> >> for gcc/cp/ChangeLog >> >> PR c++/88146 >> * method.c (do_build_copy_constructor): Do not crash with >> ellipsis-only parm list. >> (synthesize_method): Retain location of inherited ctor. >> >> for gcc/testsuite/ChangeLog >> >> PR c++/88146 >> * g++.dg/cpp0x/inh-ctor32.C: New. >> --- >> gcc/cp/method.c | 9 + >> gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C | 229 >> +++++++++++++++++++++++++++++++ >> 2 files changed, 234 insertions(+), 4 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C >> >> diff --git a/gcc/cp/method.c b/gcc/cp/method.c >> index fd023e200538..41d609fb1de6 100644 >> --- a/gcc/cp/method.c >> +++ b/gcc/cp/method.c >> @@ -643,7 +643,7 @@ do_build_copy_constructor (tree fndecl) >> bool trivial = trivial_fn_p (fndecl); >> tree inh = DECL_INHERITED_CTOR (fndecl); >> - if (!inh) >> + if (parm && !inh) >> parm = convert_from_reference (parm); > > If inh is false, we're a copy constructor, which always has a parm, so > this hunk seems unnecessary. > >> if (trivial) >> @@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl) >> { >> tree fields = TYPE_FIELDS (current_class_type); >> tree member_init_list = NULL_TREE; >> - int cvquals = cp_type_quals (TREE_TYPE (parm)); >> + int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0; > > This could also check !inh. And in the existing code, while I'm looking at it: > for (; fields; fields = DECL_CHAIN (fields)) > { > tree field = fields; > tree expr_type; > > if (TREE_CODE (field) != FIELD_DECL) > continue; > if (inh) > continue; The "if (inh) continue" is odd, there's no reason to iterate through the fields ignoring all of them when we could skip the loop entirely. Jason
On Dec 14, 2018, Jason Merrill <jason@redhat.com> wrote: >> If inh is false, we're a copy constructor, which always has a parm, >> so this hunk seems unnecessary. ack >>> - int cvquals = cp_type_quals (TREE_TYPE (parm)); >>> + int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0; >> >> This could also check !inh. *nod* > And in the existing code, while I'm looking at it: > The "if (inh) continue" is odd, there's no reason to iterate through > the fields ignoring all of them when we could skip the loop entirely. Heh, funny, an earlier version of the patch that added an if (inh) to print an error on zero-args had an 'else fields = NULL;'. That improvement went away along with my course change. But look!, it's back in the version below ;-) Testing... Ok to install if it passes? [PR c++/88146] do not crash synthesizing inherited ctor(...) This patch started out from the testcase in PR88146, that attempted to synthesize an inherited ctor without any args before a varargs ellipsis and crashed while at that, because of the unguarded dereferencing of the parm type list, that usually contains a terminator. The terminator is not there for varargs functions, however, and without any other args, we ended up dereferencing a NULL pointer. Oops. Guarding accesses to parm would be easy, but not necessary. In do_build_copy_constructor, non-inherited ctors are copy-ctors, that always have at least one parm, so parm needs not be guarded when we know the access will only take place when we're dealing with an inherited ctor. The only other problematic use was in the cvquals initializer, a variable only used in a loop over fields, that we skipped individually in inherited ctors. I've arranged to skip the entire loop over fields for inherited ctors, and to only initialize cvquals otherwise. Avoiding the crash from unguarded accesses was easy, but I thought we should still produce the sorry message we got in other testcases that passed arguments through the ellipsis in inherited ctors. I put a check in, and noticed the inherited ctors were synthesized with the location assigned to the class name, although they were initially assigned the location of the using declaration. I decided the latter was better, and arranged for the better location to be retained. Further investigation revealed the lack of a sorry message had to do with the call being in a non-evaluated context, in this case, a noexcept expression. The sorry would be correctly reported in other contexts, so I rolled back the check I'd added, but retained the source location improvement. I was still concerned about issuing sorry messages while instantiating template ctors even in non-evaluated contexts, e.g., if a template ctor had a base initializer that used an inherited ctor with enough arguments that they'd go through an ellipsis. I wanted to defer the instantiation of such template ctors, but that would have been wrong for constexpr template ctors, and already done for non-constexpr ones. So, I just consolidated multiple test variants into a single testcase that explores and explains various of the possibilities I thought of. for gcc/cp/ChangeLog PR c++/88146 * method.c (do_build_copy_constructor): Skip iteration over fields for inherited ctors, and initialize cvquals otherwise. (synthesize_method): Retain location of inherited ctor. for gcc/testsuite/ChangeLog PR c++/88146 * g++.dg/cpp0x/inh-ctor32.C: New. --- gcc/cp/method.c | 14 +- gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C | 229 +++++++++++++++++++++++++++++++ 2 files changed, 238 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C diff --git a/gcc/cp/method.c b/gcc/cp/method.c index fd023e200538..4cbdadbe3d26 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl) { tree fields = TYPE_FIELDS (current_class_type); tree member_init_list = NULL_TREE; - int cvquals = cp_type_quals (TREE_TYPE (parm)); + int cvquals; int i; tree binfo, base_binfo; tree init; @@ -704,6 +704,11 @@ do_build_copy_constructor (tree fndecl) inh, member_init_list); } + if (!inh) + cvquals = cp_type_quals (TREE_TYPE (parm)); + else + fields = NULL; + for (; fields; fields = DECL_CHAIN (fields)) { tree field = fields; @@ -711,8 +716,6 @@ do_build_copy_constructor (tree fndecl) if (TREE_CODE (field) != FIELD_DECL) continue; - if (inh) - continue; expr_type = TREE_TYPE (field); if (DECL_NAME (field)) @@ -891,8 +894,9 @@ synthesize_method (tree fndecl) /* Reset the source location, we might have been previously deferred, and thus have saved where we were first needed. */ - DECL_SOURCE_LOCATION (fndecl) - = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); + if (!DECL_INHERITED_CTOR (fndecl)) + DECL_SOURCE_LOCATION (fndecl) + = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); /* If we've been asked to synthesize a clone, just synthesize the cloned function instead. Doing so will automatically fill in the diff --git a/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C new file mode 100644 index 000000000000..c40412fc5346 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C @@ -0,0 +1,229 @@ +// { dg-do compile { target c++11 } } +// Minimized from the testcase for PR c++/88146, +// then turned into multiple variants. + +// We issue an error when calling an inherited ctor with at least one +// argument passed through a varargs ellipsis, if the call is in an +// evaluated context. Even in nonevaluated contexts, we will +// instantiate constexpr templates (unlike non-constexpr templates), +// which might then issue errors that in nonevlauated contexts +// wouldn't be issued. + +// In these variants, the inherited ctor is constexpr, but it's only +// called in unevaluated contexts, so no error is issued. The +// templateness of the ctor doesn't matter, because the only call that +// passes args through the ellipsis is in a noexcept expr, that is not +// evaluated. The ctors in derived classes are created and +// instantiated, discarding arguments passed through the ellipsis when +// calling base ctors, but that's not reported: we only report a +// problem when *calling* ctors that behave this way. +namespace unevaled_call { + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + + namespace with_template { + struct foo { + template <typename... T> + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + } + + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace with_template { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +} + +// In these variants, the inherited ctor is constexpr, and it's called +// in unevaluated contexts in ways that would otherwise trigger the +// sorry message. Here we check that the message is not issued at +// those calls, nor at subsequent calls that use the same ctor without +// passing arguments through its ellipsis. We check that it is issued +// later, when we pass the ctor arguments through the ellipsis. +namespace evaled_bad_call_in_u { + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0, 1})); + bar t(0); + bar u(0, 1); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + + namespace with_template { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + bar t(0); + bar u(0,1); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + } + + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + + namespace with_template { + struct foo { + template <typename... T> + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + } +} + +// Now, instead of instantiating a class that uses a derived ctor, we +// introduce another template ctor that will use the varargs ctor to +// initialize its base class. The idea is to verify that the error +// message is issued, even if the instantiation occurs in a +// nonevaluated context, e.g., for constexpr templates. In the +// inherited_derived_ctor, we check that even an inherited ctor of a +// constexpr ctor is instantiated and have an error message issued. +namespace derived_ctor { + namespace direct_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template <typename ...T> + constexpr bar(T ... args) : boo(args...) {} // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template <typename ...T> + /* constexpr */ bar(T ... args) : boo(args...) {} + }; + void f() noexcept(noexcept(bar{0,1})); + } + } + + namespace inherited_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template <typename ...T> + constexpr bor(T ... args) : boo(args...) {} // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); // { dg-message "'constexpr' expansion" } + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template <typename ...T> + /* constexpr */ bor(T ... args) : boo(args...) {} + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +}
On 12/14/18 5:33 PM, Alexandre Oliva wrote: > On Dec 14, 2018, Jason Merrill <jason@redhat.com> wrote: > >>> If inh is false, we're a copy constructor, which always has a parm, >>> so this hunk seems unnecessary. > > ack > >>>> - int cvquals = cp_type_quals (TREE_TYPE (parm)); >>>> + int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0; >>> >>> This could also check !inh. > > *nod* > >> And in the existing code, while I'm looking at it: > >> The "if (inh) continue" is odd, there's no reason to iterate through >> the fields ignoring all of them when we could skip the loop entirely. > > Heh, funny, an earlier version of the patch that added an if (inh) to > print an error on zero-args had an 'else fields = NULL;'. That > improvement went away along with my course change. But look!, it's back > in the version below ;-) > > Testing... Ok to install if it passes? > > > [PR c++/88146] do not crash synthesizing inherited ctor(...) > > This patch started out from the testcase in PR88146, that attempted to > synthesize an inherited ctor without any args before a varargs > ellipsis and crashed while at that, because of the unguarded > dereferencing of the parm type list, that usually contains a > terminator. The terminator is not there for varargs functions, > however, and without any other args, we ended up dereferencing a NULL > pointer. Oops. > > Guarding accesses to parm would be easy, but not necessary. In > do_build_copy_constructor, non-inherited ctors are copy-ctors, that > always have at least one parm, so parm needs not be guarded when we > know the access will only take place when we're dealing with an > inherited ctor. The only other problematic use was in the cvquals > initializer, a variable only used in a loop over fields, that we > skipped individually in inherited ctors. I've arranged to skip the > entire loop over fields for inherited ctors, and to only initialize > cvquals otherwise. > > Avoiding the crash from unguarded accesses was easy, but I thought we > should still produce the sorry message we got in other testcases that > passed arguments through the ellipsis in inherited ctors. I put a > check in, and noticed the inherited ctors were synthesized with the > location assigned to the class name, although they were initially > assigned the location of the using declaration. I decided the latter > was better, and arranged for the better location to be retained. > > Further investigation revealed the lack of a sorry message had to do > with the call being in a non-evaluated context, in this case, a > noexcept expression. The sorry would be correctly reported in other > contexts, so I rolled back the check I'd added, but retained the > source location improvement. > > I was still concerned about issuing sorry messages while instantiating > template ctors even in non-evaluated contexts, e.g., if a template > ctor had a base initializer that used an inherited ctor with enough > arguments that they'd go through an ellipsis. I wanted to defer the > instantiation of such template ctors, but that would have been wrong > for constexpr template ctors, and already done for non-constexpr ones. > So, I just consolidated multiple test variants into a single testcase > that explores and explains various of the possibilities I thought of. > > > for gcc/cp/ChangeLog > > PR c++/88146 > * method.c (do_build_copy_constructor): Skip iteration over > fields for inherited ctors, and initialize cvquals otherwise. > (synthesize_method): Retain location of inherited ctor. > > for gcc/testsuite/ChangeLog > > PR c++/88146 > * g++.dg/cpp0x/inh-ctor32.C: New. > --- > gcc/cp/method.c | 14 +- > gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C | 229 +++++++++++++++++++++++++++++++ > 2 files changed, 238 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C > > diff --git a/gcc/cp/method.c b/gcc/cp/method.c > index fd023e200538..4cbdadbe3d26 100644 > --- a/gcc/cp/method.c > +++ b/gcc/cp/method.c > @@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl) > { > tree fields = TYPE_FIELDS (current_class_type); > tree member_init_list = NULL_TREE; > - int cvquals = cp_type_quals (TREE_TYPE (parm)); > + int cvquals; > int i; > tree binfo, base_binfo; > tree init; > @@ -704,6 +704,11 @@ do_build_copy_constructor (tree fndecl) > inh, member_init_list); > } > > + if (!inh) > + cvquals = cp_type_quals (TREE_TYPE (parm)); > + else > + fields = NULL; Let's move the initialization of "fields" inside the 'then' block here with the initialization of "cvquals", rather than clear it in the 'else'. OK with that change. Jason
On Dec 14, 2018, Jason Merrill <jason@redhat.com> wrote: > Let's move the initialization of "fields" inside the 'then' block here > with the initialization of "cvquals", rather than clear it in the > 'else'. We'd still have to NULL-initialize it somewhere, so I'd rather just move the entire loop into the conditional, and narrow the scope of variables only used within the loop, like this. The full patch below is very hard to read because of the reindentation, so here's a diff -b. diff --git a/gcc/cp/method.c b/gcc/cp/method.c index fd023e200538..17404a65b0fd 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -675,12 +675,9 @@ do_build_copy_constructor (tree fndecl) } else { - tree fields = TYPE_FIELDS (current_class_type); tree member_init_list = NULL_TREE; - int cvquals = cp_type_quals (TREE_TYPE (parm)); int i; tree binfo, base_binfo; - tree init; vec<tree, va_gc> *vbases; /* Initialize all the base-classes with the parameter converted @@ -704,15 +701,18 @@ do_build_copy_constructor (tree fndecl) inh, member_init_list); } - for (; fields; fields = DECL_CHAIN (fields)) + if (!inh) + { + int cvquals = cp_type_quals (TREE_TYPE (parm)); + + for (tree fields = TYPE_FIELDS (current_class_type); + fields; fields = DECL_CHAIN (fields)) { tree field = fields; tree expr_type; if (TREE_CODE (field) != FIELD_DECL) continue; - if (inh) - continue; expr_type = TREE_TYPE (field); if (DECL_NAME (field)) @@ -742,7 +742,7 @@ do_build_copy_constructor (tree fndecl) expr_type = cp_build_qualified_type (expr_type, quals); } - init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); + tree init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); if (move_p && !TYPE_REF_P (expr_type) /* 'move' breaks bit-fields, and has no effect for scalars. */ && !scalarish_type_p (expr_type)) @@ -751,6 +751,8 @@ do_build_copy_constructor (tree fndecl) member_init_list = tree_cons (field, init, member_init_list); } + } + finish_mem_initializers (member_init_list); } } @@ -891,6 +893,7 @@ synthesize_method (tree fndecl) /* Reset the source location, we might have been previously deferred, and thus have saved where we were first needed. */ + if (!DECL_INHERITED_CTOR (fndecl)) DECL_SOURCE_LOCATION (fndecl) = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); Is this OK too? (pending regstrapping) [PR c++/88146] do not crash synthesizing inherited ctor(...) This patch started out from the testcase in PR88146, that attempted to synthesize an inherited ctor without any args before a varargs ellipsis and crashed while at that, because of the unguarded dereferencing of the parm type list, that usually contains a terminator. The terminator is not there for varargs functions, however, and without any other args, we ended up dereferencing a NULL pointer. Oops. Guarding accesses to parm would be easy, but not necessary. In do_build_copy_constructor, non-inherited ctors are copy-ctors, that always have at least one parm, so parm needs not be guarded when we know the access will only take place when we're dealing with an inherited ctor. The only other problematic use was in the cvquals initializer, a variable only used in a loop over fields, that we skipped individually in inherited ctors. I've guarded the cvquals initialization and the entire loop over fields so they only run for copy-ctors. Avoiding the crash from unguarded accesses was easy, but I thought we should still produce the sorry message we got in other testcases that passed arguments through the ellipsis in inherited ctors. I put a check in, and noticed the inherited ctors were synthesized with the location assigned to the class name, although they were initially assigned the location of the using declaration. I decided the latter was better, and arranged for the better location to be retained. Further investigation revealed the lack of a sorry message had to do with the call being in a non-evaluated context, in this case, a noexcept expression. The sorry would be correctly reported in other contexts, so I rolled back the check I'd added, but retained the source location improvement. I was still concerned about issuing sorry messages while instantiating template ctors even in non-evaluated contexts, e.g., if a template ctor had a base initializer that used an inherited ctor with enough arguments that they'd go through an ellipsis. I wanted to defer the instantiation of such template ctors, but that would have been wrong for constexpr template ctors, and already done for non-constexpr ones. So, I just consolidated multiple test variants into a single testcase that explores and explains various of the possibilities I thought of. for gcc/cp/ChangeLog PR c++/88146 * method.c (do_build_copy_constructor): Guard cvquals init and loop over fields to run for non-inherited ctors only. (synthesize_method): Retain location of inherited ctor. for gcc/testsuite/ChangeLog PR c++/88146 * g++.dg/cpp0x/inh-ctor32.C: New. --- gcc/cp/method.c | 89 ++++++------ gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C | 229 +++++++++++++++++++++++++++++++ 2 files changed, 275 insertions(+), 43 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C diff --git a/gcc/cp/method.c b/gcc/cp/method.c index fd023e200538..17404a65b0fd 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -675,12 +675,9 @@ do_build_copy_constructor (tree fndecl) } else { - tree fields = TYPE_FIELDS (current_class_type); tree member_init_list = NULL_TREE; - int cvquals = cp_type_quals (TREE_TYPE (parm)); int i; tree binfo, base_binfo; - tree init; vec<tree, va_gc> *vbases; /* Initialize all the base-classes with the parameter converted @@ -704,53 +701,58 @@ do_build_copy_constructor (tree fndecl) inh, member_init_list); } - for (; fields; fields = DECL_CHAIN (fields)) + if (!inh) { - tree field = fields; - tree expr_type; - - if (TREE_CODE (field) != FIELD_DECL) - continue; - if (inh) - continue; + int cvquals = cp_type_quals (TREE_TYPE (parm)); - expr_type = TREE_TYPE (field); - if (DECL_NAME (field)) + for (tree fields = TYPE_FIELDS (current_class_type); + fields; fields = DECL_CHAIN (fields)) { - if (VFIELD_NAME_P (DECL_NAME (field))) + tree field = fields; + tree expr_type; + + if (TREE_CODE (field) != FIELD_DECL) continue; - } - else if (ANON_AGGR_TYPE_P (expr_type) && TYPE_FIELDS (expr_type)) - /* Just use the field; anonymous types can't have - nontrivial copy ctors or assignment ops or this - function would be deleted. */; - else - continue; - /* Compute the type of "init->field". If the copy-constructor - parameter is, for example, "const S&", and the type of - the field is "T", then the type will usually be "const - T". (There are no cv-qualified variants of reference - types.) */ - if (!TYPE_REF_P (expr_type)) - { - int quals = cvquals; + expr_type = TREE_TYPE (field); + if (DECL_NAME (field)) + { + if (VFIELD_NAME_P (DECL_NAME (field))) + continue; + } + else if (ANON_AGGR_TYPE_P (expr_type) && TYPE_FIELDS (expr_type)) + /* Just use the field; anonymous types can't have + nontrivial copy ctors or assignment ops or this + function would be deleted. */; + else + continue; - if (DECL_MUTABLE_P (field)) - quals &= ~TYPE_QUAL_CONST; - quals |= cp_type_quals (expr_type); - expr_type = cp_build_qualified_type (expr_type, quals); - } + /* Compute the type of "init->field". If the copy-constructor + parameter is, for example, "const S&", and the type of + the field is "T", then the type will usually be "const + T". (There are no cv-qualified variants of reference + types.) */ + if (!TYPE_REF_P (expr_type)) + { + int quals = cvquals; - init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); - if (move_p && !TYPE_REF_P (expr_type) - /* 'move' breaks bit-fields, and has no effect for scalars. */ - && !scalarish_type_p (expr_type)) - init = move (init); - init = build_tree_list (NULL_TREE, init); + if (DECL_MUTABLE_P (field)) + quals &= ~TYPE_QUAL_CONST; + quals |= cp_type_quals (expr_type); + expr_type = cp_build_qualified_type (expr_type, quals); + } + + tree init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); + if (move_p && !TYPE_REF_P (expr_type) + /* 'move' breaks bit-fields, and has no effect for scalars. */ + && !scalarish_type_p (expr_type)) + init = move (init); + init = build_tree_list (NULL_TREE, init); - member_init_list = tree_cons (field, init, member_init_list); + member_init_list = tree_cons (field, init, member_init_list); + } } + finish_mem_initializers (member_init_list); } } @@ -891,8 +893,9 @@ synthesize_method (tree fndecl) /* Reset the source location, we might have been previously deferred, and thus have saved where we were first needed. */ - DECL_SOURCE_LOCATION (fndecl) - = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); + if (!DECL_INHERITED_CTOR (fndecl)) + DECL_SOURCE_LOCATION (fndecl) + = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); /* If we've been asked to synthesize a clone, just synthesize the cloned function instead. Doing so will automatically fill in the diff --git a/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C new file mode 100644 index 000000000000..c40412fc5346 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C @@ -0,0 +1,229 @@ +// { dg-do compile { target c++11 } } +// Minimized from the testcase for PR c++/88146, +// then turned into multiple variants. + +// We issue an error when calling an inherited ctor with at least one +// argument passed through a varargs ellipsis, if the call is in an +// evaluated context. Even in nonevaluated contexts, we will +// instantiate constexpr templates (unlike non-constexpr templates), +// which might then issue errors that in nonevlauated contexts +// wouldn't be issued. + +// In these variants, the inherited ctor is constexpr, but it's only +// called in unevaluated contexts, so no error is issued. The +// templateness of the ctor doesn't matter, because the only call that +// passes args through the ellipsis is in a noexcept expr, that is not +// evaluated. The ctors in derived classes are created and +// instantiated, discarding arguments passed through the ellipsis when +// calling base ctors, but that's not reported: we only report a +// problem when *calling* ctors that behave this way. +namespace unevaled_call { + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + + namespace with_template { + struct foo { + template <typename... T> + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + } + + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace with_template { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +} + +// In these variants, the inherited ctor is constexpr, and it's called +// in unevaluated contexts in ways that would otherwise trigger the +// sorry message. Here we check that the message is not issued at +// those calls, nor at subsequent calls that use the same ctor without +// passing arguments through its ellipsis. We check that it is issued +// later, when we pass the ctor arguments through the ellipsis. +namespace evaled_bad_call_in_u { + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0, 1})); + bar t(0); + bar u(0, 1); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + + namespace with_template { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + bar t(0); + bar u(0,1); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + } + + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + + namespace with_template { + struct foo { + template <typename... T> + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + } +} + +// Now, instead of instantiating a class that uses a derived ctor, we +// introduce another template ctor that will use the varargs ctor to +// initialize its base class. The idea is to verify that the error +// message is issued, even if the instantiation occurs in a +// nonevaluated context, e.g., for constexpr templates. In the +// inherited_derived_ctor, we check that even an inherited ctor of a +// constexpr ctor is instantiated and have an error message issued. +namespace derived_ctor { + namespace direct_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template <typename ...T> + constexpr bar(T ... args) : boo(args...) {} // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template <typename ...T> + /* constexpr */ bar(T ... args) : boo(args...) {} + }; + void f() noexcept(noexcept(bar{0,1})); + } + } + + namespace inherited_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template <typename ...T> + constexpr bor(T ... args) : boo(args...) {} // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); // { dg-message "'constexpr' expansion" } + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template <typename ...T> + /* constexpr */ bor(T ... args) : boo(args...) {} + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +}
On Fri, Dec 14, 2018 at 6:05 PM Alexandre Oliva <aoliva@redhat.com> wrote: > > On Dec 14, 2018, Jason Merrill <jason@redhat.com> wrote: > > > Let's move the initialization of "fields" inside the 'then' block here > > with the initialization of "cvquals", rather than clear it in the > > 'else'. > > We'd still have to NULL-initialize it somewhere, so I'd rather just move > the entire loop into the conditional, and narrow the scope of variables > only used within the loop, like this. The full patch below is very hard > to read because of the reindentation, so here's a diff -b. > > diff --git a/gcc/cp/method.c b/gcc/cp/method.c > index fd023e200538..17404a65b0fd 100644 > --- a/gcc/cp/method.c > +++ b/gcc/cp/method.c > @@ -675,12 +675,9 @@ do_build_copy_constructor (tree fndecl) > } > else > { > - tree fields = TYPE_FIELDS (current_class_type); > tree member_init_list = NULL_TREE; > - int cvquals = cp_type_quals (TREE_TYPE (parm)); > int i; > tree binfo, base_binfo; > - tree init; > vec<tree, va_gc> *vbases; > > /* Initialize all the base-classes with the parameter converted > @@ -704,15 +701,18 @@ do_build_copy_constructor (tree fndecl) > inh, member_init_list); > } > > - for (; fields; fields = DECL_CHAIN (fields)) > + if (!inh) > + { > + int cvquals = cp_type_quals (TREE_TYPE (parm)); > + > + for (tree fields = TYPE_FIELDS (current_class_type); > + fields; fields = DECL_CHAIN (fields)) > { > tree field = fields; > tree expr_type; > > if (TREE_CODE (field) != FIELD_DECL) > continue; > - if (inh) > - continue; > > expr_type = TREE_TYPE (field); > if (DECL_NAME (field)) > @@ -742,7 +742,7 @@ do_build_copy_constructor (tree fndecl) > expr_type = cp_build_qualified_type (expr_type, quals); > } > > - init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); > + tree init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); > if (move_p && !TYPE_REF_P (expr_type) > /* 'move' breaks bit-fields, and has no effect for scalars. */ > && !scalarish_type_p (expr_type)) > @@ -751,6 +751,8 @@ do_build_copy_constructor (tree fndecl) > > member_init_list = tree_cons (field, init, member_init_list); > } > + } > + > finish_mem_initializers (member_init_list); > } > } > @@ -891,6 +893,7 @@ synthesize_method (tree fndecl) > > /* Reset the source location, we might have been previously > deferred, and thus have saved where we were first needed. */ > + if (!DECL_INHERITED_CTOR (fndecl)) > DECL_SOURCE_LOCATION (fndecl) > = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); > > > Is this OK too? (pending regstrapping) Yes, thanks. Jason
On Sat, 15 Dec 2018 at 23:11, Jason Merrill <jason@redhat.com> wrote: > > On Fri, Dec 14, 2018 at 6:05 PM Alexandre Oliva <aoliva@redhat.com> wrote: > > > > On Dec 14, 2018, Jason Merrill <jason@redhat.com> wrote: > > > > > Let's move the initialization of "fields" inside the 'then' block here > > > with the initialization of "cvquals", rather than clear it in the > > > 'else'. > > > > We'd still have to NULL-initialize it somewhere, so I'd rather just move > > the entire loop into the conditional, and narrow the scope of variables > > only used within the loop, like this. The full patch below is very hard > > to read because of the reindentation, so here's a diff -b. > > > > diff --git a/gcc/cp/method.c b/gcc/cp/method.c > > index fd023e200538..17404a65b0fd 100644 > > --- a/gcc/cp/method.c > > +++ b/gcc/cp/method.c > > @@ -675,12 +675,9 @@ do_build_copy_constructor (tree fndecl) > > } > > else > > { > > - tree fields = TYPE_FIELDS (current_class_type); > > tree member_init_list = NULL_TREE; > > - int cvquals = cp_type_quals (TREE_TYPE (parm)); > > int i; > > tree binfo, base_binfo; > > - tree init; > > vec<tree, va_gc> *vbases; > > > > /* Initialize all the base-classes with the parameter converted > > @@ -704,15 +701,18 @@ do_build_copy_constructor (tree fndecl) > > inh, member_init_list); > > } > > > > - for (; fields; fields = DECL_CHAIN (fields)) > > + if (!inh) > > + { > > + int cvquals = cp_type_quals (TREE_TYPE (parm)); > > + > > + for (tree fields = TYPE_FIELDS (current_class_type); > > + fields; fields = DECL_CHAIN (fields)) > > { > > tree field = fields; > > tree expr_type; > > > > if (TREE_CODE (field) != FIELD_DECL) > > continue; > > - if (inh) > > - continue; > > > > expr_type = TREE_TYPE (field); > > if (DECL_NAME (field)) > > @@ -742,7 +742,7 @@ do_build_copy_constructor (tree fndecl) > > expr_type = cp_build_qualified_type (expr_type, quals); > > } > > > > - init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); > > + tree init = build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); > > if (move_p && !TYPE_REF_P (expr_type) > > /* 'move' breaks bit-fields, and has no effect for scalars. */ > > && !scalarish_type_p (expr_type)) > > @@ -751,6 +751,8 @@ do_build_copy_constructor (tree fndecl) > > > > member_init_list = tree_cons (field, init, member_init_list); > > } > > + } > > + > > finish_mem_initializers (member_init_list); > > } > > } > > @@ -891,6 +893,7 @@ synthesize_method (tree fndecl) > > > > /* Reset the source location, we might have been previously > > deferred, and thus have saved where we were first needed. */ > > + if (!DECL_INHERITED_CTOR (fndecl)) > > DECL_SOURCE_LOCATION (fndecl) > > = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); > > > > > > Is this OK too? (pending regstrapping) > > Yes, thanks. > Hi, The new test inh-ctor32.C fails on arm: FAIL: g++.dg/cpp0x/inh-ctor32.C -std=c++14 (test for warnings, line 208) FAIL: g++.dg/cpp0x/inh-ctor32.C -std=c++17 (test for warnings, line 208) The log has: /gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C: In instantiation of 'constexpr derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor::bor::bor(T ...) [with T = {int, int}]': /gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C:206:13: required from 'constexpr derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor::bar::bar(T ...) [with T = {int, int}][inherited from derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor::bor]' /gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C:208:42: required from here Christophe > Jason
On Dec 19, 2018, Christophe Lyon <christophe.lyon@linaro.org> wrote: > The new test inh-ctor32.C fails on arm: > FAIL: g++.dg/cpp0x/inh-ctor32.C -std=c++14 (test for warnings, line 208) > FAIL: g++.dg/cpp0x/inh-ctor32.C -std=c++17 (test for warnings, line 208) Thanks, sorry about the breakage, I'm looking into it. I'm very surprised and puzzled that the messages actually differ across targets, but I managed to get the same messages you got, with a cross compiler targeting arm-unknown-linux-gnueabi, that are slightly different from those I get with a native x86_64-linux-gnu compiler built out of the same sources.
On Wed, Dec 19, 2018 at 04:47:51PM -0200, Alexandre Oliva wrote: > On Dec 19, 2018, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > The new test inh-ctor32.C fails on arm: > > FAIL: g++.dg/cpp0x/inh-ctor32.C -std=c++14 (test for warnings, line 208) > > FAIL: g++.dg/cpp0x/inh-ctor32.C -std=c++17 (test for warnings, line 208) > > Thanks, sorry about the breakage, I'm looking into it. > > I'm very surprised and puzzled that the messages actually differ across > targets, but I managed to get the same messages you got, with a cross > compiler targeting arm-unknown-linux-gnueabi, that are slightly > different from those I get with a native x86_64-linux-gnu compiler built > out of the same sources. ARM returns this from ctors, compared to most other targets that return void. Maybe something related to that? Jakub
Christophe,
Thanks again for the report. This was quite an adventure to figure
out ;-) See below.
[PR88146] avoid diagnostics diffs if cdtor_returns_this
Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across
platforms. Specifically, on ARM, the diagnostics within the subtest
derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did
not match those displayed on other platforms, and the test failed.
The difference seemed to have to do with locations assigned to ctors,
but it was more subtle: on ARM, the instantiation of bor's template
ctor was nested within the instantiation of bar's template ctor
inherited from bor. The reason turned out to be related with the
internal return type of ctors: arm_cxx_cdtor_returns_this is enabled
for because of AAPCS, while cxx.cdtor_returns_this is disabled on most
other platforms. While convert_to_void returns early with a VOID
expr, the non-VOID return type of the base ctor CALL_EXPR causes
convert_to_void to inspect the called decl for nodiscard attributes:
maybe_warn_nodiscard -> cp_get_fndecl_from_callee ->
maybe_constant_init -> cxx_eval_outermost_constant_expr ->
instantiate_constexpr_fns -> nested instantiation.
The internal return type assigned to a cdtor should not affect
instantiation (constexpr or template) decisions, IMHO. We know it
affects diagnostics, but I have a hunch this might bring deeper issues
with it, so I've arranged for the CALL_EXPR handler in convert_to_void
to disregard cdtors, regardless of the ABI.
The patch is awkward on purpose: it's meant to illustrate both
portions of the affected code, to draw attention to a potential
problem, and to get bootstrap-testing coverage for the path that will
be taken on ARM. I envision removing the first hunk, and the else
from the second hunk, once testing is done.
The first hunk is there to highlight where convert_to_void returns
early on x86, instead of handling the CALL_EXPR.
BTW (here's the potential problem), shouldn't we go into the CALL_EXPR
case for the volatile void mentioned in comments next to the case, or
won't that match VOID_TYPE_P?
Finally, I shall mention the possibility of taking the opposite
direction, and actually looking for nodiscard in cdtor calls so as to
trigger the constexpr side effects that we've inadvertently triggered
and observed with the inh-ctor32.C testcase. It doesn't feel right to
me, but I've been wrong many times before ;-)
Would a rearranged version of the patch, dropping the redundant tests
and retaining only the addition of the test for cdtor identifiers, be
ok to install, provided that it passes regression testing?
Note this patch does NOT carry a ChangeLog entry. That's also on
purpose, to indicate it's not meant to be included as is.
---
gcc/cp/cvt.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index eb1687377c3e..1a15af8a6e99 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1112,7 +1112,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
error_at (loc, "pseudo-destructor is not called");
return error_mark_node;
}
- if (VOID_TYPE_P (TREE_TYPE (expr)))
+ if (VOID_TYPE_P (TREE_TYPE (expr))
+ && TREE_CODE (expr) != CALL_EXPR)
return expr;
switch (TREE_CODE (expr))
{
@@ -1169,6 +1170,24 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
break;
case CALL_EXPR: /* We have a special meaning for volatile void fn(). */
+ /* cdtors may return this or void, depending on
+ targetm.cxx.cdtor_returns_this, but this shouldn't affect our
+ decisions here: nodiscard cdtors are nonsensical, and we
+ don't want to call maybe_warn_nodiscard because it may
+ trigger constexpr or template instantiation in a way that
+ changes their instantiaton nesting. This changes the way
+ contexts are printed in diagnostics, with bad consequences
+ for the testsuite, but there may be other undesirable
+ consequences of visiting referenced ctors too soon. */
+ if (DECL_P (TREE_OPERAND (expr, 0))
+ && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (expr, 0))))
+ return expr;
+ /* FIXME: Move this test before the one above, after a round of
+ testing as it is, to get coverage of the behavior we'd get on
+ ARM. */
+ else if (VOID_TYPE_P (TREE_TYPE (expr)))
+ return expr;
+
maybe_warn_nodiscard (expr, implicit);
break;
On Thu, 20 Dec 2018 at 01:04, Alexandre Oliva <aoliva@redhat.com> wrote: > > Christophe, > > Thanks again for the report. This was quite an adventure to figure > out ;-) See below. > Glad I've helped. I wouldn't have been able to do the analysis :) > > [PR88146] avoid diagnostics diffs if cdtor_returns_this > > Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across > platforms. Specifically, on ARM, the diagnostics within the subtest > derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did > not match those displayed on other platforms, and the test failed. > > The difference seemed to have to do with locations assigned to ctors, > but it was more subtle: on ARM, the instantiation of bor's template > ctor was nested within the instantiation of bar's template ctor > inherited from bor. The reason turned out to be related with the > internal return type of ctors: arm_cxx_cdtor_returns_this is enabled > for because of AAPCS, while cxx.cdtor_returns_this is disabled on most > other platforms. While convert_to_void returns early with a VOID > expr, the non-VOID return type of the base ctor CALL_EXPR causes > convert_to_void to inspect the called decl for nodiscard attributes: > maybe_warn_nodiscard -> cp_get_fndecl_from_callee -> > maybe_constant_init -> cxx_eval_outermost_constant_expr -> > instantiate_constexpr_fns -> nested instantiation. > > The internal return type assigned to a cdtor should not affect > instantiation (constexpr or template) decisions, IMHO. We know it > affects diagnostics, but I have a hunch this might bring deeper issues > with it, so I've arranged for the CALL_EXPR handler in convert_to_void > to disregard cdtors, regardless of the ABI. > > > The patch is awkward on purpose: it's meant to illustrate both > portions of the affected code, to draw attention to a potential > problem, and to get bootstrap-testing coverage for the path that will > be taken on ARM. I envision removing the first hunk, and the else > from the second hunk, once testing is done. > > The first hunk is there to highlight where convert_to_void returns > early on x86, instead of handling the CALL_EXPR. > > BTW (here's the potential problem), shouldn't we go into the CALL_EXPR > case for the volatile void mentioned in comments next to the case, or > won't that match VOID_TYPE_P? > > Finally, I shall mention the possibility of taking the opposite > direction, and actually looking for nodiscard in cdtor calls so as to > trigger the constexpr side effects that we've inadvertently triggered > and observed with the inh-ctor32.C testcase. It doesn't feel right to > me, but I've been wrong many times before ;-) > > Would a rearranged version of the patch, dropping the redundant tests > and retaining only the addition of the test for cdtor identifiers, be > ok to install, provided that it passes regression testing? > > > Note this patch does NOT carry a ChangeLog entry. That's also on > purpose, to indicate it's not meant to be included as is. > --- > gcc/cp/cvt.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c > index eb1687377c3e..1a15af8a6e99 100644 > --- a/gcc/cp/cvt.c > +++ b/gcc/cp/cvt.c > @@ -1112,7 +1112,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) > error_at (loc, "pseudo-destructor is not called"); > return error_mark_node; > } > - if (VOID_TYPE_P (TREE_TYPE (expr))) > + if (VOID_TYPE_P (TREE_TYPE (expr)) > + && TREE_CODE (expr) != CALL_EXPR) > return expr; > switch (TREE_CODE (expr)) > { > @@ -1169,6 +1170,24 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) > break; > > case CALL_EXPR: /* We have a special meaning for volatile void fn(). */ > + /* cdtors may return this or void, depending on > + targetm.cxx.cdtor_returns_this, but this shouldn't affect our > + decisions here: nodiscard cdtors are nonsensical, and we > + don't want to call maybe_warn_nodiscard because it may > + trigger constexpr or template instantiation in a way that > + changes their instantiaton nesting. This changes the way > + contexts are printed in diagnostics, with bad consequences > + for the testsuite, but there may be other undesirable > + consequences of visiting referenced ctors too soon. */ > + if (DECL_P (TREE_OPERAND (expr, 0)) > + && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (expr, 0)))) > + return expr; > + /* FIXME: Move this test before the one above, after a round of > + testing as it is, to get coverage of the behavior we'd get on > + ARM. */ > + else if (VOID_TYPE_P (TREE_TYPE (expr))) > + return expr; > + > maybe_warn_nodiscard (expr, implicit); > break; > > > > -- > Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo > Be the change, be Free! FSF Latin America board member > GNU Toolchain Engineer Free Software Evangelist > Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
On 12/19/18 7:04 PM, Alexandre Oliva wrote: > Christophe, > > Thanks again for the report. This was quite an adventure to figure > out ;-) See below. > > > [PR88146] avoid diagnostics diffs if cdtor_returns_this > > Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across > platforms. Specifically, on ARM, the diagnostics within the subtest > derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did > not match those displayed on other platforms, and the test failed. > > The difference seemed to have to do with locations assigned to ctors, > but it was more subtle: on ARM, the instantiation of bor's template > ctor was nested within the instantiation of bar's template ctor > inherited from bor. The reason turned out to be related with the > internal return type of ctors: arm_cxx_cdtor_returns_this is enabled > for because of AAPCS, while cxx.cdtor_returns_this is disabled on most > other platforms. While convert_to_void returns early with a VOID > expr, the non-VOID return type of the base ctor CALL_EXPR causes > convert_to_void to inspect the called decl for nodiscard attributes: > maybe_warn_nodiscard -> cp_get_fndecl_from_callee -> > maybe_constant_init -> cxx_eval_outermost_constant_expr -> > instantiate_constexpr_fns -> nested instantiation. I think the bug is in calling instantiate_constexpr_fns in this case. I think that should only happen when ctx->manifestly_const_eval. Jason
On Dec 20, 2018, Jason Merrill <jason@redhat.com> wrote: > I think the bug is in calling instantiate_constexpr_fns in this case. > I think that should only happen when ctx->manifestly_const_eval. FTR, changing that breaks cpp2a/constexpr-init1.C (P0859), unfortunately. I don't see why the operand of decltype or of int{} would be manifestly const eval. Plus, I don't quite follow why the two cases covered in the constexpr-init1.C testcase have different consequences, constexpr-wise, even after rereading the verbiage about it in the standard development tree a few times; I guess I still need to fill in other gaps to in my knowledge to try and make sense of it. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index cea414d33def..88bee7aa1fed 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -5076,7 +5076,8 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, r = TARGET_EXPR_INITIAL (r); } - instantiate_constexpr_fns (r); + if (ctx.manifestly_const_eval) + instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); But, really, should varying but user-invisible cdtor return types really ever play a role in determining whether a program is well-formed, like they do ATM? I mean, is my suggested change wrong or undesirable for some reason I can't grasp, aside from its incorrect implementation, using operand0 of CALL_EXPR instead of operand0 of the ADDR_EXPR that will be operand1 of CALL_EXPR if it's not NULL? diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c index f758f2d9bc8f..9b3b4d039a16 100644 --- a/gcc/cp/cvt.c +++ b/gcc/cp/cvt.c @@ -1118,7 +1118,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) error_at (loc, "pseudo-destructor is not called"); return error_mark_node; } - if (VOID_TYPE_P (TREE_TYPE (expr))) + if (VOID_TYPE_P (TREE_TYPE (expr)) + && TREE_CODE (expr) != CALL_EXPR) return expr; switch (TREE_CODE (expr)) { @@ -1175,6 +1176,26 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) break; case CALL_EXPR: /* We have a special meaning for volatile void fn(). */ + /* cdtors may return this or void, depending on + targetm.cxx.cdtor_returns_this, but this shouldn't affect our + decisions here: nodiscard cdtors are nonsensical, and we + don't want to call maybe_warn_nodiscard because it may + trigger constexpr or template instantiation in a way that + changes their instantiaton nesting. This changes the way + contexts are printed in diagnostics, with bad consequences + for the testsuite, but there may be other undesirable + consequences of visiting referenced ctors too soon. */ + if (TREE_OPERAND (expr, 1) + && TREE_CODE (TREE_OPERAND (expr, 1)) == ADDR_EXPR + && DECL_P (TREE_OPERAND (TREE_OPERAND (expr, 1), 0)) + && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (TREE_OPERAND (expr, 1), 0)))) + return expr; + /* FIXME: Move this test before the one above, after a round of + testing as it is, to get coverage of the behavior we'd get on + ARM. */ + else if (VOID_TYPE_P (TREE_TYPE (expr))) + return expr; + maybe_warn_nodiscard (expr, implicit); break;
On Dec 28, 2018, Alexandre Oliva <aoliva@redhat.com> wrote: > I guess I still need to > fill in other gaps to in my knowledge to try and make sense of it. Done. > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c Here's a patch based on your suggestion. [PR88146] do not instantiate constexpr if not manifestly const This is another approach at fixing the g++.dg/cpp0x/inh-ctor32.C test failures on targets that have non-void return values for cdtors: only instantiate constexpr functions for expressions that are potentially constant evaluated. Alas, this exposed a latent problem elsewhere: braced initializer lists underwent check_narrowing when simplified to non-lists, but that did not signal maybe_constant_value the potentially constant evaluated constant that all direct members of braced initializer lists have, so after the change above we'd no longer initialize constexpr functions that per P0859 we ought to. cpp2a/constexpr-init1.C, taken directly from P0859, flagged the regression. While looking into it, I realized it was fragile: it could have passed even if we flagged the error we should flag for the construct that we are supposed to accept. So, I split it out, to catch too-aggressive constexpr instantiation as much as the too-lax instantiation the original testcase flagged after the first part of the change. Regstrapped on x86_64- and i686-linux-gnu. Ok to install? for gcc/cp/ChangeLog PR c++/88146 * constexpr.c (cxx_eval_outermost_constant_expr): Only instantiate constexpr fns for manifestly const eval. * typeck2.c (check_narrowing): Test for maybe constant value with manifestly const eval. for gcc/testsuite/ChangeLog PR c++/88146 * g++.dg/cpp2a/constexpr-inst1a.C: Split out of... * g++.dg/cpp2a/constexpr-inst1.C: ... this. --- gcc/cp/constexpr.c | 3 ++- gcc/cp/typeck2.c | 6 +++++- gcc/testsuite/g++.dg/cpp2a/constexpr-inst1.C | 3 --- gcc/testsuite/g++.dg/cpp2a/constexpr-inst1a.C | 9 +++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-inst1a.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index cea414d33def..88bee7aa1fed 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -5076,7 +5076,8 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, r = TARGET_EXPR_INITIAL (r); } - instantiate_constexpr_fns (r); + if (ctx.manifestly_const_eval) + instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c index cc9bf02439b6..ae194d519395 100644 --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -918,7 +918,11 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain, bool const_only) return ok; } - init = maybe_constant_value (init); + /* Immediate subexpressions in BRACED_ENCLOSED_INITIALIZERs are + potentially constant evaluated. Without manifestly_const_eval, + we won't instantiate constexpr functions that we must + instantiate. */ + init = maybe_constant_value (init, NULL_TREE, /*manifestly_const_eval=*/true); /* If we were asked to only check constants, return early. */ if (const_only && !TREE_CONSTANT (init)) diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1.C index 1016bec9d3e1..34863de3cf84 100644 --- a/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1.C +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1.C @@ -2,12 +2,9 @@ // { dg-do compile { target c++14 } } template<typename T> constexpr int f() { return T::value; } // { dg-error "int" } -template<bool B, typename T> void g(decltype(B ? f<T>() : 0)); -template<bool B, typename T> void g(...); template<bool B, typename T> void h(decltype(int{B ? f<T>() : 0})); template<bool B, typename T> void h(...); void x() { - g<false, int>(0); // OK, B ? f<T>() : 0 is not potentially constant evaluated h<false, int>(0); // error, instantiates f<int> even though B evaluates to false and // list-initialization of int from int cannot be narrowing } diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1a.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1a.C new file mode 100644 index 000000000000..1c068595e374 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-inst1a.C @@ -0,0 +1,9 @@ +// Testcase from P0859 +// { dg-do compile { target c++14 } } + +template<typename T> constexpr int f() { return T::value; } +template<bool B, typename T> void g(decltype(B ? f<T>() : 0)); +template<bool B, typename T> void g(...); +void x() { + g<false, int>(0); // OK, B ? f<T>() : 0 is not potentially constant evaluated +}
On Dec 28, 2018, Alexandre Oliva <aoliva@redhat.com> wrote:
> diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
And here's a cleaned-up patch based on my initial approach.
[PR88146] avoid diagnostics diffs if cdtor_returns_this
Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across
platforms. Specifically, on ARM, the diagnostics within the subtest
derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did
not match those displayed on other platforms, and the test failed.
The difference seemed to have to do with locations assigned to ctors,
but it was more subtle: on ARM, the instantiation of bor's template
ctor was nested within the instantiation of bar's template ctor
inherited from bor. The reason turned out to be related with the
internal return type of ctors: arm_cxx_cdtor_returns_this is enabled
for because of AAPCS, while cxx.cdtor_returns_this is disabled on most
other platforms. While convert_to_void returns early with a VOID
expr, the non-VOID return type of the base ctor CALL_EXPR causes
convert_to_void to inspect the called decl for nodiscard attributes:
maybe_warn_nodiscard -> cp_get_fndecl_from_callee ->
maybe_constant_init -> cxx_eval_outermost_constant_expr ->
instantiate_constexpr_fns -> nested instantiation.
The internal return type assigned to a cdtor should not affect
instantiation (constexpr or template) decisions, IMHO. We know it
affects diagnostics, but I have a hunch this might bring deeper issues
with it, so I've arranged for the CALL_EXPR handler in convert_to_void
to disregard cdtors, regardless of the ABI.
Regstrapped on x86_64- and i686-linux-gnu. Ok to install?
for gcc/cp/ChangeLog
PR c++/88146
* cvt.c (convert_to_void): Handle all cdtor calls as if
returning void.
---
gcc/cp/cvt.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index f758f2d9bc8f..092c81434e33 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1175,6 +1175,18 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
break;
case CALL_EXPR: /* We have a special meaning for volatile void fn(). */
+ /* cdtors may return this or void, depending on
+ targetm.cxx.cdtor_returns_this, but this shouldn't affect our
+ decisions here: neither nodiscard warnings (nodiscard cdtors
+ are nonsensical), nor should any constexpr or template
+ instantiations be affected by an ABI property that is, or at
+ least ought to be transparent to the language. */
+ if (TREE_OPERAND (expr, 1)
+ && TREE_CODE (TREE_OPERAND (expr, 1)) == ADDR_EXPR
+ && DECL_P (TREE_OPERAND (TREE_OPERAND (expr, 1), 0))
+ && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (TREE_OPERAND (expr, 1), 0))))
+ return expr;
+
maybe_warn_nodiscard (expr, implicit);
break;
On Sat, Dec 29, 2018 at 12:33:18AM -0200, Alexandre Oliva wrote: > --- a/gcc/cp/typeck2.c > +++ b/gcc/cp/typeck2.c > @@ -918,7 +918,11 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain, bool const_only) > return ok; > } > > - init = maybe_constant_value (init); > + /* Immediate subexpressions in BRACED_ENCLOSED_INITIALIZERs are > + potentially constant evaluated. Without manifestly_const_eval, The term in latest draft is manifestly constant-evaluated. > + we won't instantiate constexpr functions that we must > + instantiate. */ > + init = maybe_constant_value (init, NULL_TREE, /*manifestly_const_eval=*/true); If this is a right change, then it should be covered also by a testcase that makes sure __builtin_is_constant_evaluated () in those initializers evaluates to true in a new g++.dg/cpp2a/is-constant-evaluated-*.C testcase. Jakub
On 12/28/18 9:33 PM, Alexandre Oliva wrote: > On Dec 28, 2018, Alexandre Oliva <aoliva@redhat.com> wrote: > >> I guess I still need to >> fill in other gaps to in my knowledge to try and make sense of it. > > Done. > >> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > Here's a patch based on your suggestion. > > [PR88146] do not instantiate constexpr if not manifestly const > > This is another approach at fixing the g++.dg/cpp0x/inh-ctor32.C test > failures on targets that have non-void return values for cdtors: only > instantiate constexpr functions for expressions that are potentially > constant evaluated. > > Alas, this exposed a latent problem elsewhere: braced initializer > lists underwent check_narrowing when simplified to non-lists, but that > did not signal maybe_constant_value the potentially constant evaluated > constant that all direct members of braced initializer lists have, so > after the change above we'd no longer initialize constexpr functions > that per P0859 we ought to. Ah, yeah, that's a troublesome complication. This area of the standard is very new, so I think let's defer trying to deal with this issue and pursue your other patch for now. > + if (TREE_OPERAND (expr, 1) > + && TREE_CODE (TREE_OPERAND (expr, 1)) == ADDR_EXPR > + && DECL_P (TREE_OPERAND (TREE_OPERAND (expr, 1), 0)) > + && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (TREE_OPERAND (expr, 1), 0)))) Maybe if (tree fn = cp_get_callee_fndecl_nofold (expr)) if (DECL_CONSTRUCTOR_P (fn) || DECL_DESTRUCTOR_P (fn)) ? The other patch is OK with that change. Jason
On Jan 4, 2019, Jason Merrill <jason@redhat.com> wrote: > if (tree fn = cp_get_callee_fndecl_nofold (expr)) > if (DECL_CONSTRUCTOR_P (fn) || DECL_DESTRUCTOR_P (fn)) > ? The other patch is OK with that change. Thanks, I've regstrapped this on x86_64- and i686-linux-gnu, and I'm checking it in momentarily. [PR88146] avoid diagnostics diffs if cdtor_returns_this Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across platforms. Specifically, on ARM, the diagnostics within the subtest derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did not match those displayed on other platforms, and the test failed. The difference seemed to have to do with locations assigned to ctors, but it was more subtle: on ARM, the instantiation of bor's template ctor was nested within the instantiation of bar's template ctor inherited from bor. The reason turned out to be related with the internal return type of ctors: arm_cxx_cdtor_returns_this is enabled for because of AAPCS, while cxx.cdtor_returns_this is disabled on most other platforms. While convert_to_void returns early with a VOID expr, the non-VOID return type of the base ctor CALL_EXPR causes convert_to_void to inspect the called decl for nodiscard attributes: maybe_warn_nodiscard -> cp_get_fndecl_from_callee -> maybe_constant_init -> cxx_eval_outermost_constant_expr -> instantiate_constexpr_fns -> nested instantiation. The internal return type assigned to a cdtor should not affect instantiation (constexpr or template) decisions, IMHO. We know it affects diagnostics, but I have a hunch this might bring deeper issues with it, so I've arranged for the CALL_EXPR handler in convert_to_void to disregard cdtors, regardless of the ABI. for gcc/cp/ChangeLog PR c++/88146 * cvt.c (convert_to_void): Handle all cdtor calls as if returning void. --- gcc/cp/cvt.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c index 914281193980..82a44f353c76 100644 --- a/gcc/cp/cvt.c +++ b/gcc/cp/cvt.c @@ -1175,6 +1175,16 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) break; case CALL_EXPR: /* We have a special meaning for volatile void fn(). */ + /* cdtors may return this or void, depending on + targetm.cxx.cdtor_returns_this, but this shouldn't affect our + decisions here: neither nodiscard warnings (nodiscard cdtors + are nonsensical), nor should any constexpr or template + instantiations be affected by an ABI property that is, or at + least ought to be transparent to the language. */ + if (tree fn = cp_get_callee_fndecl_nofold (expr)) + if (DECL_CONSTRUCTOR_P (fn) || DECL_DESTRUCTOR_P (fn)) + return expr; + maybe_warn_nodiscard (expr, implicit); break;
diff --git a/gcc/cp/method.c b/gcc/cp/method.c index fd023e200538..41d609fb1de6 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -643,7 +643,7 @@ do_build_copy_constructor (tree fndecl) bool trivial = trivial_fn_p (fndecl); tree inh = DECL_INHERITED_CTOR (fndecl); - if (!inh) + if (parm && !inh) parm = convert_from_reference (parm); if (trivial) @@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl) { tree fields = TYPE_FIELDS (current_class_type); tree member_init_list = NULL_TREE; - int cvquals = cp_type_quals (TREE_TYPE (parm)); + int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0; int i; tree binfo, base_binfo; tree init; @@ -891,8 +891,9 @@ synthesize_method (tree fndecl) /* Reset the source location, we might have been previously deferred, and thus have saved where we were first needed. */ - DECL_SOURCE_LOCATION (fndecl) - = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); + if (!DECL_INHERITED_CTOR (fndecl)) + DECL_SOURCE_LOCATION (fndecl) + = DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); /* If we've been asked to synthesize a clone, just synthesize the cloned function instead. Doing so will automatically fill in the diff --git a/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C new file mode 100644 index 000000000000..c40412fc5346 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C @@ -0,0 +1,229 @@ +// { dg-do compile { target c++11 } } +// Minimized from the testcase for PR c++/88146, +// then turned into multiple variants. + +// We issue an error when calling an inherited ctor with at least one +// argument passed through a varargs ellipsis, if the call is in an +// evaluated context. Even in nonevaluated contexts, we will +// instantiate constexpr templates (unlike non-constexpr templates), +// which might then issue errors that in nonevlauated contexts +// wouldn't be issued. + +// In these variants, the inherited ctor is constexpr, but it's only +// called in unevaluated contexts, so no error is issued. The +// templateness of the ctor doesn't matter, because the only call that +// passes args through the ellipsis is in a noexcept expr, that is not +// evaluated. The ctors in derived classes are created and +// instantiated, discarding arguments passed through the ellipsis when +// calling base ctors, but that's not reported: we only report a +// problem when *calling* ctors that behave this way. +namespace unevaled_call { + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + + namespace with_template { + struct foo { + template <typename... T> + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + } + + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace with_template { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +} + +// In these variants, the inherited ctor is constexpr, and it's called +// in unevaluated contexts in ways that would otherwise trigger the +// sorry message. Here we check that the message is not issued at +// those calls, nor at subsequent calls that use the same ctor without +// passing arguments through its ellipsis. We check that it is issued +// later, when we pass the ctor arguments through the ellipsis. +namespace evaled_bad_call_in_u { + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0, 1})); + bar t(0); + bar u(0, 1); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + + namespace with_template { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + bar t(0); + bar u(0,1); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + } + + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + + namespace with_template { + struct foo { + template <typename... T> + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + } + } +} + +// Now, instead of instantiating a class that uses a derived ctor, we +// introduce another template ctor that will use the varargs ctor to +// initialize its base class. The idea is to verify that the error +// message is issued, even if the instantiation occurs in a +// nonevaluated context, e.g., for constexpr templates. In the +// inherited_derived_ctor, we check that even an inherited ctor of a +// constexpr ctor is instantiated and have an error message issued. +namespace derived_ctor { + namespace direct_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template <typename ...T> + constexpr bar(T ... args) : boo(args...) {} // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template <typename ...T> + /* constexpr */ bar(T ... args) : boo(args...) {} + }; + void f() noexcept(noexcept(bar{0,1})); + } + } + + namespace inherited_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template <typename ...T> + constexpr bor(T ... args) : boo(args...) {} // { dg-message "sorry, unimplemented: passing arguments to ellipsis" } + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); // { dg-message "'constexpr' expansion" } + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template <typename T> + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template <typename ...T> + /* constexpr */ bor(T ... args) : boo(args...) {} + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +}