| Message ID | or35iqxaum.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | tolerate cdtors returning this in constexpr | expand |
On Wed, Apr 06, 2022 at 04:36:49PM -0300, Alexandre Oliva via Gcc-patches wrote: > > On targets that return this from cdtors, cxx_eval_call_expression may > flag flowing off the end of a dtor. That's preempted for ctors, and > avoided entirely when dtors return void, but when they return this, > the return value should be conceptually disregarded, without making > room for such internal ABI details to make a program ill-formed, as in > g++.dg/cpp2a/constexpr-dtor12.C on arm-eabi. > > Regstrapped on x86_64-linux-gnu, also verified the testcase fix on > arm-eabi. Ok to install? > > > for gcc/cp/ChangeLog > > * constexpr.cc (cxx_eval_call_expression): Disregard dtor > result. > --- > gcc/cp/constexpr.cc | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 9c40b0515747d..d8bc864ae6bcc 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -2889,7 +2889,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > else > { > result = *ctx->global->values.get (res); > - if (result == NULL_TREE && !*non_constant_p) > + if (result == NULL_TREE && !*non_constant_p > + && !DECL_DESTRUCTOR_P (fun)) Would it make sense to use && !(DECL_DESTRUCTOR_P (fun) && targetm.cxx.cdtor_returns_this ()) instead? > { > if (!ctx->quiet) > error ("%<constexpr%> call flows off the end " Marek
On Apr 6, 2022, Marek Polacek <polacek@redhat.com> wrote: > On Wed, Apr 06, 2022 at 04:36:49PM -0300, Alexandre Oliva via Gcc-patches wrote: >> else >> { >> result = *ctx->global->values.get (res); >> - if (result == NULL_TREE && !*non_constant_p) >> + if (result == NULL_TREE && !*non_constant_p >> + && !DECL_DESTRUCTOR_P (fun)) > Would it make sense to use > && !(DECL_DESTRUCTOR_P (fun) && targetm.cxx.cdtor_returns_this ()) > instead? Just before the 'else' above, there's a test for a void result type, which covers the '!targetm.cxx.cdtor_returns_this ()' case, so IMHO that would be excessive. And then, avoiding that error for dtors is not something that should be done only on some targets: no dtor should ever trigger that error.
On 4/6/22 15:36, Alexandre Oliva wrote: Please adjust your patch subject lines for the new guidelines adopted as part of the git transition: https://gcc.gnu.org/contribute.html#patches e.g. [PATCH] c++: tolerate cdtors returning this in constexpr [PRnnnnn] > On targets that return this from cdtors, cxx_eval_call_expression may > flag flowing off the end of a dtor. That's preempted for ctors, and > avoided entirely when dtors return void, but when they return this, > the return value should be conceptually disregarded, without making > room for such internal ABI details to make a program ill-formed, as in > g++.dg/cpp2a/constexpr-dtor12.C on arm-eabi. Is there a PR for this issue? The patch looks fine, but why doesn't the implicit return 'this' on arm-eabi already make the result non-null? Thanks, Jason > Regstrapped on x86_64-linux-gnu, also verified the testcase fix on > arm-eabi. Ok to install? > > > for gcc/cp/ChangeLog > > * constexpr.cc (cxx_eval_call_expression): Disregard dtor > result. > --- > gcc/cp/constexpr.cc | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 9c40b0515747d..d8bc864ae6bcc 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -2889,7 +2889,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > else > { > result = *ctx->global->values.get (res); > - if (result == NULL_TREE && !*non_constant_p) > + if (result == NULL_TREE && !*non_constant_p > + && !DECL_DESTRUCTOR_P (fun)) > { > if (!ctx->quiet) > error ("%<constexpr%> call flows off the end " >
On 4/6/22 15:45, Marek Polacek via Gcc-patches wrote: > On Wed, Apr 06, 2022 at 04:36:49PM -0300, Alexandre Oliva via Gcc-patches wrote: >> >> On targets that return this from cdtors, cxx_eval_call_expression may >> flag flowing off the end of a dtor. That's preempted for ctors, and >> avoided entirely when dtors return void, but when they return this, >> the return value should be conceptually disregarded, without making >> room for such internal ABI details to make a program ill-formed, as in >> g++.dg/cpp2a/constexpr-dtor12.C on arm-eabi. >> >> Regstrapped on x86_64-linux-gnu, also verified the testcase fix on >> arm-eabi. Ok to install? >> >> >> for gcc/cp/ChangeLog >> >> * constexpr.cc (cxx_eval_call_expression): Disregard dtor >> result. >> --- >> gcc/cp/constexpr.cc | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc >> index 9c40b0515747d..d8bc864ae6bcc 100644 >> --- a/gcc/cp/constexpr.cc >> +++ b/gcc/cp/constexpr.cc >> @@ -2889,7 +2889,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, >> else >> { >> result = *ctx->global->values.get (res); >> - if (result == NULL_TREE && !*non_constant_p) >> + if (result == NULL_TREE && !*non_constant_p >> + && !DECL_DESTRUCTOR_P (fun)) > > Would it make sense to use > > && !(DECL_DESTRUCTOR_P (fun) && targetm.cxx.cdtor_returns_this ()) > > instead? I don't think that's necessary; it's the destructorness that makes it OK. And if it doesn't return this we would have taken the VOID_TYPE_P branch before. Jason
Hello, Jason, On Apr 6, 2022, Jason Merrill <jason@redhat.com> wrote: > On 4/6/22 15:36, Alexandre Oliva wrote: > Please adjust your patch subject lines for the new guidelines adopted > as part of the git transition: > https://gcc.gnu.org/contribute.html#patches Oh, wow, I had missed those guidelines entirely! *blush* Thanks for the pointer. I'm taking note of it for future submissions. >> On targets that return this from cdtors, cxx_eval_call_expression may >> flag flowing off the end of a dtor. That's preempted for ctors, and >> avoided entirely when dtors return void, but when they return this, >> the return value should be conceptually disregarded, without making >> room for such internal ABI details to make a program ill-formed, as in >> g++.dg/cpp2a/constexpr-dtor12.C on arm-eabi. > Is there a PR for this issue? I'm afraid not, it's just one of many testsuite fails that I've been working on cleaning up. > The patch looks fine, but why doesn't the implicit return 'this' on > arm-eabi already make the result non-null? That's a good question that I didn't have an answer for. It's the explicit 'return' statement in T::~T(), turned into a goto, that interrupts the iteration over the stmt list containing the return stmt: <<< Unknown tree: must_not_throw_expr { <<< Unknown tree: cleanup_stmt <<< Unknown tree: cleanup_stmt <<cleanup_point <<< Unknown tree: expr_stmt (void) (((struct T *) this)->D.4458.s = (TARGET_EXPR <D.4487, operato\ r new (4)>;, TARGET_EXPR <D.4488, 1>;, *(int *) D.4487 = NON_LVALUE_EXPR <42>;,\ D.4488 = 0;;, (int *) D.4487;)) >>>>>; // predicted unlikely by goto predictor.; goto <D.4489>; (void) S::~S (&((struct T *) this)->D.4458) >>>; *(struct T *) this = {CLOBBER} >>>; } (*) <D.4489>:; return this; >>> (*) eval'ing this block sets jump_target to <D.4489>, which satisfies the returns(tree*) predicate, so cxx_eval_statement_list bails. Now, ISTM that the goto target selected for the return stmt bypasses the subobject dtor call and the full-object clobber. That sounds like another bug, no?
On 4/7/22 18:25, Alexandre Oliva wrote: > Hello, Jason, > > On Apr 6, 2022, Jason Merrill <jason@redhat.com> wrote: > >> On 4/6/22 15:36, Alexandre Oliva wrote: >> Please adjust your patch subject lines for the new guidelines adopted >> as part of the git transition: > >> https://gcc.gnu.org/contribute.html#patches > > Oh, wow, I had missed those guidelines entirely! *blush* > > Thanks for the pointer. I'm taking note of it for future submissions. > >>> On targets that return this from cdtors, cxx_eval_call_expression may >>> flag flowing off the end of a dtor. That's preempted for ctors, and >>> avoided entirely when dtors return void, but when they return this, >>> the return value should be conceptually disregarded, without making >>> room for such internal ABI details to make a program ill-formed, as in >>> g++.dg/cpp2a/constexpr-dtor12.C on arm-eabi. > >> Is there a PR for this issue? > > I'm afraid not, it's just one of many testsuite fails that I've been > working on cleaning up. > >> The patch looks fine, but why doesn't the implicit return 'this' on >> arm-eabi already make the result non-null? > > That's a good question that I didn't have an answer for. > > It's the explicit 'return' statement in T::~T(), turned into a goto, > that interrupts the iteration over the stmt list containing the return > stmt: > > <<< Unknown tree: must_not_throw_expr > { > <<< Unknown tree: cleanup_stmt > <<< Unknown tree: cleanup_stmt > <<cleanup_point <<< Unknown tree: expr_stmt > (void) (((struct T *) this)->D.4458.s = (TARGET_EXPR <D.4487, operato\ > r new (4)>;, TARGET_EXPR <D.4488, 1>;, *(int *) D.4487 = NON_LVALUE_EXPR <42>;,\ > D.4488 = 0;;, (int *) D.4487;)) >>>>>; > // predicted unlikely by goto predictor.; > goto <D.4489>; > (void) S::~S (&((struct T *) this)->D.4458) > >>>; > *(struct T *) this = {CLOBBER} > >>>; > } (*) > <D.4489>:; > return this; > >>> > > (*) eval'ing this block sets jump_target to <D.4489>, which satisfies > the returns(tree*) predicate, so cxx_eval_statement_list bails. > > Now, ISTM that the goto target selected for the return stmt bypasses the > subobject dtor call and the full-object clobber. That sounds like > another bug, no? The subobject cleanup and clobber should be evaluated along the way, the evaluation of CLEANUP_EXPR isn't affected by jump_target. I think the only thing we're wrongly skipping is the actual return, and your patch works around that. I think that makes sense for now; I have a patch for GCC 13 to remove cdtor_label entirely. Your patch is OK. Jason
On Apr 9, 2022, Jason Merrill <jason@redhat.com> wrote: >> goto <D.4489>; >> (void) S::~S (&((struct T *) this)->D.4458) >> Now, ISTM that the goto target selected for the return stmt bypasses the >> subobject dtor call and the full-object clobber. That sounds like >> another bug, no? > The subobject cleanup and clobber should be evaluated along the way, > the evaluation of CLEANUP_EXPR isn't affected by jump_target. *nod*, I was just confused by the dump, that made the CLEANUP_EXPR seem part of the STATEMENT_LIST. > I think the only thing we're wrongly skipping is the actual return, > and your patch works around that. I think that makes sense for now; I > have a patch for GCC 13 to remove cdtor_label entirely. Ah, great, thanks. > Your patch is OK. Here's the adjusted (subject, thanks) patch I'm about to install. c++: Tolerate cdtors returning this in constexpr On targets that return this from cdtors, cxx_eval_call_expression may flag flowing off the end of a dtor. That's preempted for ctors, and avoided entirely when dtors return void, but when they return this, the return value should be conceptually disregarded, without making room for such internal ABI details to make a program ill-formed, as in g++.dg/cpp2a/constexpr-dtor12.C on arm-eabi. for gcc/cp/ChangeLog * constexpr.cc (cxx_eval_call_expression): Disregard dtor result. --- gcc/cp/constexpr.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index 9c40b0515747d..d8bc864ae6bcc 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -2889,7 +2889,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, else { result = *ctx->global->values.get (res); - if (result == NULL_TREE && !*non_constant_p) + if (result == NULL_TREE && !*non_constant_p + && !DECL_DESTRUCTOR_P (fun)) { if (!ctx->quiet) error ("%<constexpr%> call flows off the end "
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index 9c40b0515747d..d8bc864ae6bcc 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -2889,7 +2889,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, else { result = *ctx->global->values.get (res); - if (result == NULL_TREE && !*non_constant_p) + if (result == NULL_TREE && !*non_constant_p + && !DECL_DESTRUCTOR_P (fun)) { if (!ctx->quiet) error ("%<constexpr%> call flows off the end "