diff mbox series

[v2] c++: Fix crash during NRV optimization with invalid input [PR117099, PR117129]

Message ID 010201929ae34be2-65ab82c7-d542-481d-959d-05fac1213d0c-000000@eu-west-1.amazonses.com
State New
Headers show
Series [v2] c++: Fix crash during NRV optimization with invalid input [PR117099, PR117129] | expand

Commit Message

Simon Martin Oct. 17, 2024, 2:30 p.m. UTC 
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.

Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

Thanks, Simon
From ff01d18d97893ef65259f9063794945d5062cf4e Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@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/crash78.C: New test.

---
 gcc/cp/typeck.cc                     | 11 ++++++++---
 gcc/testsuite/g++.dg/parse/crash77.C | 14 ++++++++++++++
 gcc/testsuite/g++.dg/parse/crash78.C | 15 +++++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/parse/crash77.C
 create mode 100644 gcc/testsuite/g++.dg/parse/crash78.C

Comments

Jason Merrill Oct. 22, 2024, 3:07 p.m. UTC | #1 
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.

Jason
Simon Martin Oct. 30, 2024, 4:34 p.m. UTC | #2 
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 <simon@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 }
+  }
+}
Jason Merrill Nov. 5, 2024, 2:30 a.m. UTC | #3 
On 10/30/24 12:34 PM, Simon Martin wrote:
> 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?

I think let's include it, but add a comment that the return is UB in 
case it breaks later.

> @@ -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.  */
...
> -      /* If the conversion failed, treat this just like `return;'.  */
> +      /* If the conversion failed, treat this just like
> +	 `return <ERROR>;'.  */

Let's leave these comments alone, but comment on the 
current_function_return_value assignments that we are suppressing NRV.

OK with those comment adjustments.

Jason
Simon Martin Nov. 5, 2024, 9:49 a.m. UTC | #4 
Hi Jason,

On 5 Nov 2024, at 3:30, Jason Merrill wrote:

> On 10/30/24 12:34 PM, Simon Martin wrote:
>> 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?
>
> I think let's include it, but add a comment that the return is UB in 
> case it breaks later.
>
>> @@ -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.  */
> ...
>> -      /* If the conversion failed, treat this just like `return;'.  
>> */
>> +      /* If the conversion failed, treat this just like
>> +	 `return <ERROR>;'.  */
>
> Let's leave these comments alone, but comment on the 
> current_function_return_value assignments that we are suppressing NRV.
>
> OK with those comment adjustments.
Thanks, amended accordingly and merged as r15-4959-gf31b72b75ef7cd. For 
the record I also renumbered the added tests since 
r15-4957-gb1d92aeb8583c8 added a crash77.C test already.

Simon
diff mbox series

Patch

diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 71d879abef1..485e8b347bb 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -11232,8 +11232,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;
+      retval = 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.  */
@@ -11442,9 +11443,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..8c3dddc13d2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/crash77.C
@@ -0,0 +1,14 @@ 
+// PR c++/117099
+// { dg-do "compile" }
+
+struct X {
+  ~X();
+};
+
+X test(bool b) {
+  {
+    X x;
+    return x;
+  } 
+  if (!(b)) return; // { dg-error "return-statement with no value" }
+}
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 }
+  }
+}