Re: [PATCH v2 2/3] c++: Improve constexpr error for dangling local variables

2023-06-23 Thread Patrick Palka via Gcc-patches
On Wed, 29 Mar 2023, Nathaniel Shead via Gcc-patches 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". 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.

LGTM.  It seems the original motivation for returning a null pointer
here was to avoid issuing duplicate warnings
(https://gcc.gnu.org/legacy-ml/gcc-patches/2014-04/msg00269.html)
which your patch addresses.

> 
> 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/warn/Wreturn-local-addr-6.C: Remove check for return
> value optimisation.
> 
> Signed-off-by: Nathaniel Shead 
> ---
>  gcc/cp/semantics.cc  | 5 -
>  gcc/cp/typeck.cc | 5 +++--
>  gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C | 4 ++--
>  gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C | 3 ---
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 87c2e8a7111..14b4b7f4ce1 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -1246,7 +1246,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 afb956087ce..a7d642e2029 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -11235,8 +11235,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;
>  }
>  
>if (processing_template_decl)
> 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/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" } } */
> -- 
> 2.34.1
> 
> 



[PATCH v2 2/3] c++: Improve constexpr error for dangling local variables

2023-03-28 Thread Nathaniel Shead via Gcc-patches
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.

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/warn/Wreturn-local-addr-6.C: Remove check for return
value optimisation.

Signed-off-by: Nathaniel Shead 
---
 gcc/cp/semantics.cc  | 5 -
 gcc/cp/typeck.cc | 5 +++--
 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C | 4 ++--
 gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C | 3 ---
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 87c2e8a7111..14b4b7f4ce1 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -1246,7 +1246,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 afb956087ce..a7d642e2029 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -11235,8 +11235,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;
 }
 
   if (processing_template_decl)
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/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" } } */
-- 
2.34.1