diff mbox series

c++: Relax too strict assert in stabilize_expr [PR111160]

Message ID 0102019055ec2368-f8b335ab-baf4-421a-8748-f131254376c3-000000@eu-west-1.amazonses.com
State New
Headers show
Series c++: Relax too strict assert in stabilize_expr [PR111160] | expand

Commit Message

Simon Martin June 26, 2024, 7 p.m. UTC 
The case in the ticket is an ICE on invalid due to an assert in stabilize_expr,
but the underlying issue can actually trigger on this *valid* code:

=== cut here ===
struct TheClass {
  TheClass() {}
  TheClass(volatile TheClass& t) {}
  TheClass operator=(volatile TheClass& t) volatile { return t; }
};
void the_func() {
  volatile TheClass x, y, z;
  (false ? x : y) = z;
}
=== cut here ===

The problem is that stabilize_expr asserts that it returns an expression
without TREE_SIDE_EFFECTS, which can't be if the involved type is volatile.

This patch relaxes the assert to accept having TREE_THIS_VOLATILE on the
returned expression.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/111160

gcc/cp/ChangeLog:

	* tree.cc (stabilize_expr): Stabilized expressions can have
	TREE_SIDE_EFFECTS if they're volatile.

gcc/testsuite/ChangeLog:

	* g++.dg/overload/error8.C: New test.
	* g++.dg/overload/volatile2.C: New test.

---
 gcc/cp/tree.cc                            |  2 +-
 gcc/testsuite/g++.dg/overload/error8.C    |  9 +++++++++
 gcc/testsuite/g++.dg/overload/volatile2.C | 12 ++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/overload/error8.C
 create mode 100644 gcc/testsuite/g++.dg/overload/volatile2.C

Comments

Patrick Palka June 28, 2024, 10:07 p.m. UTC | #1 
On Wed, 26 Jun 2024, Simon Martin wrote:

> The case in the ticket is an ICE on invalid due to an assert in stabilize_expr,
> but the underlying issue can actually trigger on this *valid* code:
> 
> === cut here ===
> struct TheClass {
>   TheClass() {}
>   TheClass(volatile TheClass& t) {}
>   TheClass operator=(volatile TheClass& t) volatile { return t; }
> };
> void the_func() {
>   volatile TheClass x, y, z;
>   (false ? x : y) = z;
> }
> === cut here ===
> 
> The problem is that stabilize_expr asserts that it returns an expression
> without TREE_SIDE_EFFECTS, which can't be if the involved type is volatile.
> 
> This patch relaxes the assert to accept having TREE_THIS_VOLATILE on the
> returned expression.
> 
> Successfully tested on x86_64-pc-linux-gnu.
> 
> 	PR c++/111160
> 
> gcc/cp/ChangeLog:
> 
> 	* tree.cc (stabilize_expr): Stabilized expressions can have
> 	TREE_SIDE_EFFECTS if they're volatile.

LGTM (although I can't formally approve the patch)

> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/overload/error8.C: New test.
> 	* g++.dg/overload/volatile2.C: New test.
> 
> ---
>  gcc/cp/tree.cc                            |  2 +-
>  gcc/testsuite/g++.dg/overload/error8.C    |  9 +++++++++
>  gcc/testsuite/g++.dg/overload/volatile2.C | 12 ++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/overload/error8.C
>  create mode 100644 gcc/testsuite/g++.dg/overload/volatile2.C
> 
> diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> index 28648c14c6d..dfd4a3a948b 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -5969,7 +5969,7 @@ stabilize_expr (tree exp, tree* initp)
>      }
>    *initp = init_expr;
>  
> -  gcc_assert (!TREE_SIDE_EFFECTS (exp));
> +  gcc_assert (!TREE_SIDE_EFFECTS (exp) || TREE_THIS_VOLATILE (exp));
>    return exp;
>  }
>  
> diff --git a/gcc/testsuite/g++.dg/overload/error8.C b/gcc/testsuite/g++.dg/overload/error8.C
> new file mode 100644
> index 00000000000..a7e745860e0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/overload/error8.C
> @@ -0,0 +1,9 @@
> +// PR c++/111160
> +// { dg-do compile { target c++11 } }
> +
> +class TheClass {}; // { dg-error "discards|bind|discards|bind" }
> +void the_func() {
> +  TheClass x;
> +  volatile TheClass y;
> +  (false ? x : x) = y; // { dg-error "ambiguous|ambiguous" }
> +}
> diff --git a/gcc/testsuite/g++.dg/overload/volatile2.C b/gcc/testsuite/g++.dg/overload/volatile2.C
> new file mode 100644
> index 00000000000..9f27357aed6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/overload/volatile2.C
> @@ -0,0 +1,12 @@
> +// PR c++/111160
> +// { dg-do compile { target c++11 } }
> +
> +struct TheClass {
> +  TheClass() {}
> +  TheClass(volatile TheClass& t) {}
> +  TheClass operator=(volatile TheClass& t) volatile { return t; }
> +};
> +void the_func() {
> +  volatile TheClass x, y, z;
> +  (false ? x : y) = z;
> +}
> -- 
> 2.44.0
> 
> 
> 
>
Jason Merrill July 1, 2024, 6:27 p.m. UTC | #2 
On 6/26/24 3:00 PM, Simon Martin wrote:
> The case in the ticket is an ICE on invalid due to an assert in stabilize_expr,
> but the underlying issue can actually trigger on this *valid* code:
> 
> === cut here ===
> struct TheClass {
>    TheClass() {}
>    TheClass(volatile TheClass& t) {}
>    TheClass operator=(volatile TheClass& t) volatile { return t; }
> };
> void the_func() {
>    volatile TheClass x, y, z;
>    (false ? x : y) = z;
> }
> === cut here ===
> 
> The problem is that stabilize_expr asserts that it returns an expression
> without TREE_SIDE_EFFECTS, which can't be if the involved type is volatile.
> 
> This patch relaxes the assert to accept having TREE_THIS_VOLATILE on the
> returned expression.
> 
> Successfully tested on x86_64-pc-linux-gnu.

OK.

> 	PR c++/111160
> 
> gcc/cp/ChangeLog:
> 
> 	* tree.cc (stabilize_expr): Stabilized expressions can have
> 	TREE_SIDE_EFFECTS if they're volatile.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/overload/error8.C: New test.
> 	* g++.dg/overload/volatile2.C: New test.
> 
> ---
>   gcc/cp/tree.cc                            |  2 +-
>   gcc/testsuite/g++.dg/overload/error8.C    |  9 +++++++++
>   gcc/testsuite/g++.dg/overload/volatile2.C | 12 ++++++++++++
>   3 files changed, 22 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/overload/error8.C
>   create mode 100644 gcc/testsuite/g++.dg/overload/volatile2.C
> 
> diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> index 28648c14c6d..dfd4a3a948b 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -5969,7 +5969,7 @@ stabilize_expr (tree exp, tree* initp)
>       }
>     *initp = init_expr;
>   
> -  gcc_assert (!TREE_SIDE_EFFECTS (exp));
> +  gcc_assert (!TREE_SIDE_EFFECTS (exp) || TREE_THIS_VOLATILE (exp));
>     return exp;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/overload/error8.C b/gcc/testsuite/g++.dg/overload/error8.C
> new file mode 100644
> index 00000000000..a7e745860e0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/overload/error8.C
> @@ -0,0 +1,9 @@
> +// PR c++/111160
> +// { dg-do compile { target c++11 } }
> +
> +class TheClass {}; // { dg-error "discards|bind|discards|bind" }
> +void the_func() {
> +  TheClass x;
> +  volatile TheClass y;
> +  (false ? x : x) = y; // { dg-error "ambiguous|ambiguous" }
> +}
> diff --git a/gcc/testsuite/g++.dg/overload/volatile2.C b/gcc/testsuite/g++.dg/overload/volatile2.C
> new file mode 100644
> index 00000000000..9f27357aed6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/overload/volatile2.C
> @@ -0,0 +1,12 @@
> +// PR c++/111160
> +// { dg-do compile { target c++11 } }
> +
> +struct TheClass {
> +  TheClass() {}
> +  TheClass(volatile TheClass& t) {}
> +  TheClass operator=(volatile TheClass& t) volatile { return t; }
> +};
> +void the_func() {
> +  volatile TheClass x, y, z;
> +  (false ? x : y) = z;
> +}
diff mbox series

Patch

diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 28648c14c6d..dfd4a3a948b 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -5969,7 +5969,7 @@  stabilize_expr (tree exp, tree* initp)
     }
   *initp = init_expr;
 
-  gcc_assert (!TREE_SIDE_EFFECTS (exp));
+  gcc_assert (!TREE_SIDE_EFFECTS (exp) || TREE_THIS_VOLATILE (exp));
   return exp;
 }
 
diff --git a/gcc/testsuite/g++.dg/overload/error8.C b/gcc/testsuite/g++.dg/overload/error8.C
new file mode 100644
index 00000000000..a7e745860e0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/overload/error8.C
@@ -0,0 +1,9 @@ 
+// PR c++/111160
+// { dg-do compile { target c++11 } }
+
+class TheClass {}; // { dg-error "discards|bind|discards|bind" }
+void the_func() {
+  TheClass x;
+  volatile TheClass y;
+  (false ? x : x) = y; // { dg-error "ambiguous|ambiguous" }
+}
diff --git a/gcc/testsuite/g++.dg/overload/volatile2.C b/gcc/testsuite/g++.dg/overload/volatile2.C
new file mode 100644
index 00000000000..9f27357aed6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/overload/volatile2.C
@@ -0,0 +1,12 @@ 
+// PR c++/111160
+// { dg-do compile { target c++11 } }
+
+struct TheClass {
+  TheClass() {}
+  TheClass(volatile TheClass& t) {}
+  TheClass operator=(volatile TheClass& t) volatile { return t; }
+};
+void the_func() {
+  volatile TheClass x, y, z;
+  (false ? x : y) = z;
+}