tolerate cdtors returning this in constexpr

tolerate cdtors returning this in constexpr

[Alexandre Oliva]

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))
 		{
 		  if (!ctx->quiet)
 		    error ("%<constexpr%> call flows off the end "

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: tolerate cdtors returning this in constexpr

[Marek Polacek]

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

Re: tolerate cdtors returning this in constexpr

[Alexandre Oliva]

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.

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: tolerate cdtors returning this in constexpr

[Jason Merrill]

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 "
> 

Re: tolerate cdtors returning this in constexpr

[Jason Merrill]

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

Re: [PATCH] c++: tolerate cdtors returning this in constexpr

[Alexandre Oliva]

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?

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] c++: tolerate cdtors returning this in constexpr

[Jason Merrill]

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

Re: [PATCH v1.1] c++: tolerate cdtors returning this in constexpr

[Alexandre Oliva]

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 "


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>