[v4,2/3] c++: Improve constexpr error for dangling local variables [PR110619]

  Currently, when typeck discovers that a return statement will refer to a
local variable it rewrites to return a null pointer. This causes the
error messages for using the return value in a constant expression to be
unhelpful, especially for reference return values.

This patch removes this "optimisation". Relying on this raises a warning
by default and causes UB anyway, so there should be no issue in doing
so. We also suppress additional warnings from later passes that detect
this as a dangling pointer, since we've already indicated this anyway.

	PR c++/110619

gcc/cp/ChangeLog:

	* semantics.cc (finish_return_stmt): Suppress dangling pointer
	reporting on return statement if already reported.
	* typeck.cc (check_return_expr): Don't set return expression to
	zero for dangling addresses.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1y/constexpr-lifetime5.C: Test reported message is
	correct.
	* g++.dg/cpp1y/constexpr-lifetime6.C: Likewise.
	* g++.dg/cpp1y/constexpr-110619.C: New test.
	* g++.dg/warn/Wreturn-local-addr-6.C: Remove check for return
	value optimisation.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/semantics.cc                              |  5 ++++-
 gcc/cp/typeck.cc                                 |  5 +++--
 gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C    | 10 ++++++++++
 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C |  4 ++--
 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C |  8 ++++----
 gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C |  3 ---
 6 files changed, 23 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
  

On 7/20/23 05:36, Nathaniel Shead wrote:
> Currently, when typeck discovers that a return statement will refer to a
> local variable it rewrites to return a null pointer. This causes the
> error messages for using the return value in a constant expression to be
> unhelpful, especially for reference return values.
> 
> This patch removes this "optimisation".

This isn't an optimization, it's for safety, removing a way for an 
attacker to get a handle on other data on the stack (CWE-562).

But I agree that we need to preserve some element of UB for constexpr 
evaluation to see.

Perhaps we want to move this transformation to 
cp_maybe_instrument_return, so it happens after maybe_save_constexpr_fundef?

> Relying on this raises a warning
> by default and causes UB anyway, so there should be no issue in doing
> so. We also suppress additional warnings from later passes that detect
> this as a dangling pointer, since we've already indicated this anyway.
> 
> 	PR c++/110619
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (finish_return_stmt): Suppress dangling pointer
> 	reporting on return statement if already reported.
> 	* typeck.cc (check_return_expr): Don't set return expression to
> 	zero for dangling addresses.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp1y/constexpr-lifetime5.C: Test reported message is
> 	correct.
> 	* g++.dg/cpp1y/constexpr-lifetime6.C: Likewise.
> 	* g++.dg/cpp1y/constexpr-110619.C: New test.
> 	* g++.dg/warn/Wreturn-local-addr-6.C: Remove check for return
> 	value optimisation.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/semantics.cc                              |  5 ++++-
>   gcc/cp/typeck.cc                                 |  5 +++--
>   gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C    | 10 ++++++++++
>   gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C |  4 ++--
>   gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C |  8 ++++----
>   gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C |  3 ---
>   6 files changed, 23 insertions(+), 12 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 8fb47fd179e..107407de513 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -1260,7 +1260,10 @@ finish_return_stmt (tree expr)
>   
>     r = build_stmt (input_location, RETURN_EXPR, expr);
>     if (no_warning)
> -    suppress_warning (r, OPT_Wreturn_type);
> +    {
> +      suppress_warning (r, OPT_Wreturn_type);
> +      suppress_warning (r, OPT_Wdangling_pointer_);
> +    }
>     r = maybe_cleanup_point_expr_void (r);
>     r = add_stmt (r);
>   
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 859b133a18d..47233b3b717 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -11273,8 +11273,9 @@ check_return_expr (tree retval, bool *no_warning)
>         else if (!processing_template_decl
>   	       && maybe_warn_about_returning_address_of_local (retval, loc)
>   	       && INDIRECT_TYPE_P (valtype))
> -	retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
> -			 build_zero_cst (TREE_TYPE (retval)));
> +	/* Suppress the Wdangling-pointer warning in the return statement
> +	   that would otherwise occur.  */
> +	*no_warning = true;
>       }
>   
>     /* A naive attempt to reduce the number of -Wdangling-reference false
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
> new file mode 100644
> index 00000000000..cca13302238
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
> @@ -0,0 +1,10 @@
> +// { dg-do compile { target c++14 } }
> +// { dg-options "-Wno-return-local-addr" }
> +// PR c++/110619
> +
> +constexpr auto f() {
> +    int i = 0;
> +    return &i;
> +};
> +
> +static_assert( f() != nullptr );
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C
> index a4bc71d890a..ad3ef579f63 100644
> --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C
> @@ -1,11 +1,11 @@
>   // { dg-do compile { target c++14 } }
>   // { dg-options "-Wno-return-local-addr" }
>   
> -constexpr const int& id(int x) { return x; }
> +constexpr const int& id(int x) { return x; }  // { dg-message "note: declared here" }
>   
>   constexpr bool test() {
>     const int& y = id(3);
>     return y == 3;
>   }
>   
> -constexpr bool x = test();  // { dg-error "" }
> +constexpr bool x = test();  // { dg-error "accessing object outside its lifetime" }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C
> index f358aff4490..b81e89af79c 100644
> --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C
> @@ -4,12 +4,12 @@
>   struct Empty {};
>   
>   constexpr const Empty& empty() {
> -  return Empty{};
> +  return Empty{};  // { dg-message "note: declared here" }
>   }
>   
> -constexpr const Empty& empty_parm(Empty e) {
> +constexpr const Empty& empty_parm(Empty e) {  // { dg-message "note: declared here" }
>     return e;
>   }
>   
> -constexpr Empty a = empty();  // { dg-error "" }
> -constexpr Empty b = empty_parm({});  // { dg-error "" }
> +constexpr Empty a = empty();  // { dg-error "accessing object outside its lifetime" }
> +constexpr Empty b = empty_parm({});  // { dg-error "accessing object outside its lifetime" }
> diff --git a/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C b/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C
> index fae8b7e766f..ec8e241d83e 100644
> --- a/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C
> +++ b/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C
> @@ -24,6 +24,3 @@ return_addr_local_as_intref (void)
>   
>     return (const intptr_t&)a;   // { dg-warning "\\\[-Wreturn-local-addr]" } */
>   }
> -
> -/* Verify that the return value has been replaced with zero:
> -  { dg-final { scan-tree-dump-times "return 0;" 2 "optimized" } } */
  

On Thu, Jul 20, 2023 at 11:46:47AM -0400, Jason Merrill wrote:
> On 7/20/23 05:36, Nathaniel Shead wrote:
> > Currently, when typeck discovers that a return statement will refer to a
> > local variable it rewrites to return a null pointer. This causes the
> > error messages for using the return value in a constant expression to be
> > unhelpful, especially for reference return values.
> > 
> > This patch removes this "optimisation".
> 
> This isn't an optimization, it's for safety, removing a way for an attacker
> to get a handle on other data on the stack (CWE-562).
> 
> But I agree that we need to preserve some element of UB for constexpr
> evaluation to see.
> 
> Perhaps we want to move this transformation to cp_maybe_instrument_return,
> so it happens after maybe_save_constexpr_fundef?

Hm, OK. I can try giving this a go. I guess I should move the entire
maybe_warn_about_returning_address_of_local function to cp-gimplify.cc
to be able to detect this? Or is there a better way of marking that a
return expression will return a reference to a local for this
transformation? (I guess I can't use whether the warning has been
surpressed or not because the warning might not be enabled at all.)

It looks like this warning is raised also by diag_return_locals in
gimple-ssa-isolate-paths, should the transformation also be made here?

I note that the otherwise very similar -Wdangling-pointer warning
doesn't do this transformation either, should that also be something I
look into fixing here?

> > Relying on this raises a warning
> > by default and causes UB anyway, so there should be no issue in doing
> > so. We also suppress additional warnings from later passes that detect
> > this as a dangling pointer, since we've already indicated this anyway.
> > 
> > 	PR c++/110619
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* semantics.cc (finish_return_stmt): Suppress dangling pointer
> > 	reporting on return statement if already reported.
> > 	* typeck.cc (check_return_expr): Don't set return expression to
> > 	zero for dangling addresses.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp1y/constexpr-lifetime5.C: Test reported message is
> > 	correct.
> > 	* g++.dg/cpp1y/constexpr-lifetime6.C: Likewise.
> > 	* g++.dg/cpp1y/constexpr-110619.C: New test.
> > 	* g++.dg/warn/Wreturn-local-addr-6.C: Remove check for return
> > 	value optimisation.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >   gcc/cp/semantics.cc                              |  5 ++++-
> >   gcc/cp/typeck.cc                                 |  5 +++--
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C    | 10 ++++++++++
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C |  4 ++--
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C |  8 ++++----
> >   gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C |  3 ---
> >   6 files changed, 23 insertions(+), 12 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
> > 
> > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > index 8fb47fd179e..107407de513 100644
> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -1260,7 +1260,10 @@ finish_return_stmt (tree expr)
> >     r = build_stmt (input_location, RETURN_EXPR, expr);
> >     if (no_warning)
> > -    suppress_warning (r, OPT_Wreturn_type);
> > +    {
> > +      suppress_warning (r, OPT_Wreturn_type);
> > +      suppress_warning (r, OPT_Wdangling_pointer_);
> > +    }
> >     r = maybe_cleanup_point_expr_void (r);
> >     r = add_stmt (r);
> > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> > index 859b133a18d..47233b3b717 100644
> > --- a/gcc/cp/typeck.cc
> > +++ b/gcc/cp/typeck.cc
> > @@ -11273,8 +11273,9 @@ check_return_expr (tree retval, bool *no_warning)
> >         else if (!processing_template_decl
> >   	       && maybe_warn_about_returning_address_of_local (retval, loc)
> >   	       && INDIRECT_TYPE_P (valtype))
> > -	retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
> > -			 build_zero_cst (TREE_TYPE (retval)));
> > +	/* Suppress the Wdangling-pointer warning in the return statement
> > +	   that would otherwise occur.  */
> > +	*no_warning = true;
> >       }
> >     /* A naive attempt to reduce the number of -Wdangling-reference false
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
> > new file mode 100644
> > index 00000000000..cca13302238
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C
> > @@ -0,0 +1,10 @@
> > +// { dg-do compile { target c++14 } }
> > +// { dg-options "-Wno-return-local-addr" }
> > +// PR c++/110619
> > +
> > +constexpr auto f() {
> > +    int i = 0;
> > +    return &i;
> > +};
> > +
> > +static_assert( f() != nullptr );
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C
> > index a4bc71d890a..ad3ef579f63 100644
> > --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C
> > @@ -1,11 +1,11 @@
> >   // { dg-do compile { target c++14 } }
> >   // { dg-options "-Wno-return-local-addr" }
> > -constexpr const int& id(int x) { return x; }
> > +constexpr const int& id(int x) { return x; }  // { dg-message "note: declared here" }
> >   constexpr bool test() {
> >     const int& y = id(3);
> >     return y == 3;
> >   }
> > -constexpr bool x = test();  // { dg-error "" }
> > +constexpr bool x = test();  // { dg-error "accessing object outside its lifetime" }
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C
> > index f358aff4490..b81e89af79c 100644
> > --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C
> > @@ -4,12 +4,12 @@
> >   struct Empty {};
> >   constexpr const Empty& empty() {
> > -  return Empty{};
> > +  return Empty{};  // { dg-message "note: declared here" }
> >   }
> > -constexpr const Empty& empty_parm(Empty e) {
> > +constexpr const Empty& empty_parm(Empty e) {  // { dg-message "note: declared here" }
> >     return e;
> >   }
> > -constexpr Empty a = empty();  // { dg-error "" }
> > -constexpr Empty b = empty_parm({});  // { dg-error "" }
> > +constexpr Empty a = empty();  // { dg-error "accessing object outside its lifetime" }
> > +constexpr Empty b = empty_parm({});  // { dg-error "accessing object outside its lifetime" }
> > diff --git a/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C b/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C
> > index fae8b7e766f..ec8e241d83e 100644
> > --- a/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C
> > +++ b/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C
> > @@ -24,6 +24,3 @@ return_addr_local_as_intref (void)
> >     return (const intptr_t&)a;   // { dg-warning "\\\[-Wreturn-local-addr]" } */
> >   }
> > -
> > -/* Verify that the return value has been replaced with zero:
> > -  { dg-final { scan-tree-dump-times "return 0;" 2 "optimized" } } */
>
  

On 7/21/23 01:39, Nathaniel Shead wrote:
> On Thu, Jul 20, 2023 at 11:46:47AM -0400, Jason Merrill wrote:
>> On 7/20/23 05:36, Nathaniel Shead wrote:
>>> Currently, when typeck discovers that a return statement will refer to a
>>> local variable it rewrites to return a null pointer. This causes the
>>> error messages for using the return value in a constant expression to be
>>> unhelpful, especially for reference return values.
>>>
>>> This patch removes this "optimisation".
>>
>> This isn't an optimization, it's for safety, removing a way for an attacker
>> to get a handle on other data on the stack (CWE-562).
>>
>> But I agree that we need to preserve some element of UB for constexpr
>> evaluation to see.
>>
>> Perhaps we want to move this transformation to cp_maybe_instrument_return,
>> so it happens after maybe_save_constexpr_fundef?
> 
> Hm, OK. I can try giving this a go. I guess I should move the entire
> maybe_warn_about_returning_address_of_local function to cp-gimplify.cc
> to be able to detect this? Or is there a better way of marking that a
> return expression will return a reference to a local for this
> transformation? (I guess I can't use whether the warning has been
> surpressed or not because the warning might not be enabled at all.)

You could use a TREE_LANG_FLAG, looks like none of them are used on 
RETURN_EXPR.

> It looks like this warning is raised also by diag_return_locals in
> gimple-ssa-isolate-paths, should the transformation also be made here?

Looks like it already is, in warn_return_addr_local:

>       tree zero = build_zero_cst (TREE_TYPE (val));
>       gimple_return_set_retval (return_stmt, zero);
>       update_stmt (return_stmt);

...but, weirdly, only with -fisolate-erroneous-paths-*, even though it 
isn't isolating anything.  Perhaps there should be another flag for this.

> I note that the otherwise very similar -Wdangling-pointer warning
> doesn't do this transformation either, should that also be something I
> look into fixing here?

With that same flag, perhaps.  I wonder if it would make sense to remove 
the isolate-paths handling of locals in favor of the dangling-pointer 
handling?  I don't know either file much at all.

Jason
  

On Fri, Jul 21, 2023 at 05:44:51PM -0400, Jason Merrill wrote:
> On 7/21/23 01:39, Nathaniel Shead wrote:
> > On Thu, Jul 20, 2023 at 11:46:47AM -0400, Jason Merrill wrote:
> > > On 7/20/23 05:36, Nathaniel Shead wrote:
> > > > Currently, when typeck discovers that a return statement will refer to a
> > > > local variable it rewrites to return a null pointer. This causes the
> > > > error messages for using the return value in a constant expression to be
> > > > unhelpful, especially for reference return values.
> > > > 
> > > > This patch removes this "optimisation".
> > > 
> > > This isn't an optimization, it's for safety, removing a way for an attacker
> > > to get a handle on other data on the stack (CWE-562).
> > > 
> > > But I agree that we need to preserve some element of UB for constexpr
> > > evaluation to see.
> > > 
> > > Perhaps we want to move this transformation to cp_maybe_instrument_return,
> > > so it happens after maybe_save_constexpr_fundef?
> > 
> > Hm, OK. I can try giving this a go. I guess I should move the entire
> > maybe_warn_about_returning_address_of_local function to cp-gimplify.cc
> > to be able to detect this? Or is there a better way of marking that a
> > return expression will return a reference to a local for this
> > transformation? (I guess I can't use whether the warning has been
> > surpressed or not because the warning might not be enabled at all.)
> 
> You could use a TREE_LANG_FLAG, looks like none of them are used on
> RETURN_EXPR.
> 
> > It looks like this warning is raised also by diag_return_locals in
> > gimple-ssa-isolate-paths, should the transformation also be made here?
> 
> Looks like it already is, in warn_return_addr_local:
> 
> >       tree zero = build_zero_cst (TREE_TYPE (val));
> >       gimple_return_set_retval (return_stmt, zero);
> >       update_stmt (return_stmt);
> 
> ...but, weirdly, only with -fisolate-erroneous-paths-*, even though it isn't
> isolating anything.  Perhaps there should be another flag for this.
> 

I see, thanks. From this I've found that my above patch isn't sufficient
anyway, as compiling with -O2 causes the warning to appear twice as the
suppression I did wasn't sufficient. As such I'll exclude this patch
from the next revision since it's not actually necessary for the problem
I was trying to solve, and I'll work on trying to solve this properly
a bit later.

> > I note that the otherwise very similar -Wdangling-pointer warning
> > doesn't do this transformation either, should that also be something I
> > look into fixing here?
> 
> With that same flag, perhaps.  I wonder if it would make sense to remove the
> isolate-paths handling of locals in favor of the dangling-pointer handling?
> I don't know either file much at all.
> 
> Jason
>