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 <nathanielosh...@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" } } */ >