Hi Jason,
On 22 Oct 2024, at 17:07, Jason Merrill wrote:
> On 10/17/24 10:30 AM, Simon Martin wrote:
>> Hi,
>>
>> The issue reported in PR117129 is pretty similar to the one in
>> PR117099,
>>
>> so here’s an updated version of the patch that fixes both issues,
>> by
>> ensuring that finish_return_expr sets current_function_return_value
>> to
>> error_mark_node in case of error with said return value.
>
>> permerror (input_location, "return-statement with no value, in "
>> "function returning %qT", valtype);
>> - /* Remember that this function did return. */
>> + /* Remember that this function returns an error. */
>> current_function_returns_value = 1;
>> + retval = error_mark_node;
>
> We shouldn't do this if -fpermissive made the permerror into a
> warning; let's just set current_function_return_value.
Right, thanks for calling this out. The updated patch addresses this,
and also adds an execution test case that was regressing with my initial
version. Note that it relies on undefined behaviour, so I’m curious
whether I should include it or not?
Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
Thanks, Simon
From 3de79b3cdb413c34ff520ae31114f84c2f81ab2b Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Wed, 16 Oct 2024 15:47:12 +0200
Subject: [PATCH] c++: Fix crash during NRV optimization with invalid input
[PR117099, PR117129]
PR117099 and PR117129 are ICEs upon invalid code that happen when NRVO
is activated, and both due to the fact that we don't consistently set
current_function_return_value to error_mark_node upon error in
finish_return_expr.
This patch fixes this inconsistency which fixes both cases since we skip
calling finalize_nrv when current_function_return_value is
error_mark_node.
Successfully tested on x86_64-pc-linux-gnu.
PR c++/117099
PR c++/117129
gcc/cp/ChangeLog:
* typeck.cc (check_return_expr): Upon error, set
current_function_return_value to error_mark_node.
gcc/testsuite/ChangeLog:
* g++.dg/parse/crash77.C: New test.
* g++.dg/parse/crash77a.C: New test.
* g++.dg/parse/crash78.C: New test.
---
gcc/cp/typeck.cc | 11 ++++++++---
gcc/testsuite/g++.dg/parse/crash77.C | 15 +++++++++++++++
gcc/testsuite/g++.dg/parse/crash77a.C | 22 ++++++++++++++++++++++
gcc/testsuite/g++.dg/parse/crash78.C | 15 +++++++++++++++
4 files changed, 60 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/parse/crash77.C
create mode 100644 gcc/testsuite/g++.dg/parse/crash77a.C
create mode 100644 gcc/testsuite/g++.dg/parse/crash78.C
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 6ce1bb61fe7..78a8f4809ed 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -11236,8 +11236,9 @@ check_return_expr (tree retval, bool *no_warning, bool
*dangling)
if (functype != error_mark_node)
permerror (input_location, "return-statement with no value, in "
"function returning %qT", valtype);
- /* Remember that this function did return. */
+ /* Remember that this function returns an error. */
current_function_returns_value = 1;
+ current_function_return_value = error_mark_node;
/* And signal caller that TREE_NO_WARNING should be set on the
RETURN_EXPR to avoid control reaches end of non-void function
warnings in tree-cfg.cc. */
@@ -11446,9 +11447,13 @@ check_return_expr (tree retval, bool *no_warning, bool
*dangling)
tf_warning_or_error);
retval = convert (valtype, retval);
- /* If the conversion failed, treat this just like `return;'. */
+ /* If the conversion failed, treat this just like
+ `return <ERROR>;'. */
if (retval == error_mark_node)
- return retval;
+ {
+ current_function_return_value = error_mark_node;
+ return retval;
+ }
/* We can't initialize a register from a AGGR_INIT_EXPR. */
else if (! cfun->returns_struct
&& TREE_CODE (retval) == TARGET_EXPR
diff --git a/gcc/testsuite/g++.dg/parse/crash77.C
b/gcc/testsuite/g++.dg/parse/crash77.C
new file mode 100644
index 00000000000..f30fe08df54
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/crash77.C
@@ -0,0 +1,15 @@
+// PR c++/117099
+// { dg-do "compile" }
+
+struct X {
+ ~X();
+};
+
+X test(bool b) {
+ {
+ X x;
+ return x;
+ }
+ return X();
+ if (!(b)) return; // { dg-error "return-statement with no value" }
+}
diff --git a/gcc/testsuite/g++.dg/parse/crash77a.C
b/gcc/testsuite/g++.dg/parse/crash77a.C
new file mode 100644
index 00000000000..b12dafe376f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/crash77a.C
@@ -0,0 +1,22 @@
+// PR c++/117099
+// With -fpermissive, make sure we don't do NRVO in this case, but keep
+// executing fine.
+// { dg-do "run" }
+// { dg-options "-fpermissive" }
+
+struct X {
+ ~X() {}
+};
+
+X test(bool b) {
+ X x;
+ if (b)
+ return x;
+ else
+ return; // { dg-warning "return-statement with no value" }
+}
+
+int main(int, char*[]) {
+ (void) test (false);
+ return 0;
+}
diff --git a/gcc/testsuite/g++.dg/parse/crash78.C
b/gcc/testsuite/g++.dg/parse/crash78.C
new file mode 100644
index 00000000000..08d62af39c6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/crash78.C
@@ -0,0 +1,15 @@
+// PR c++/117129
+// { dg-do "compile" { target c++11 } }
+
+struct Noncopyable {
+ Noncopyable();
+ Noncopyable(const Noncopyable &) = delete; // { dg-note "declared here" }
+ virtual ~Noncopyable();
+};
+Noncopyable nrvo() {
+ {
+ Noncopyable A;
+ return A; // { dg-error "use of deleted function" }
+ // { dg-note "display considered" "" { target *-*-* } .-1 }
+ }
+}
--
2.44.0
