Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Jan 4, 2019, Jason Merrill 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; -- Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo Be the change, be Free! FSF Latin America board member GNU Toolchain EngineerFree Software Evangelist Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On 12/28/18 9:33 PM, Alexandre Oliva wrote: On Dec 28, 2018, Alexandre Oliva 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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 28, 2018, Alexandre Oliva 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; -- Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo Be the change, be Free! FSF Latin America board member GNU Toolchain EngineerFree Software Evangelist Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 28, 2018, Alexandre Oliva 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 (, r,
false, _constant_p, _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 constexpr int f() { return T::value; } // { dg-error
"int" }
-template void g(decltype(B ? f() : 0));
-template void g(...);
template void h(decltype(int{B ? f() : 0}));
template void h(...);
void x() {
- g(0); // OK, B ? f() : 0 is not potentially constant evaluated
h(0); // error, instantiates f 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 ..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 constexpr int f() { return T::value; }
+template void g(decltype(B ? f() : 0));
+template void g(...);
+void x() {
+ g(0); // OK, B ? f() : 0 is not potentially constant evaluated
+}
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 20, 2018, Jason Merrill 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 (, r,
false, _constant_p, _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;
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Thu, 20 Dec 2018 at 01:04, Alexandre Oliva 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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
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;
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain EngineerFree Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Wed, Dec 19, 2018 at 04:47:51PM -0200, Alexandre Oliva wrote: > On Dec 19, 2018, Christophe Lyon 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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 19, 2018, Christophe Lyon 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. -- Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo Be the change, be Free! FSF Latin America board member GNU Toolchain EngineerFree Software Evangelist Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Sat, 15 Dec 2018 at 23:11, Jason Merrill wrote:
>
> On Fri, Dec 14, 2018 at 6:05 PM Alexandre Oliva wrote:
> >
> > On Dec 14, 2018, Jason Merrill 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 *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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Fri, Dec 14, 2018 at 6:05 PM Alexandre Oliva wrote:
>
> On Dec 14, 2018, Jason Merrill 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 *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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 14, 2018, Jason Merrill 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 *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 al
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On 12/14/18 5:33 PM, Alexandre Oliva wrote:
On Dec 14, 2018, Jason Merrill 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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 14, 2018, Jason Merrill 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 wh
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
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
Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)
On Dec 6, 2018, Alexandre Oliva 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. -- Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo Be the change, be Free! FSF Latin America board member GNU Toolchain EngineerFree Software Evangelist Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
[C++ Patch] [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 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 (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 ..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 {
+
