[PATCH] c++/114409 - ANNOTATE_EXPR and templates

[PATCH] c++/114409 - ANNOTATE_EXPR and templates

[Richard Biener]

The following fixes a mismatch in COMPOUND_EXPR handling in
tsubst_expr vs tsubst_stmt where the latter allows a stmt in
operand zero but the former doesn't.  This makes a difference
for the case at hand because when the COMPOUND_EXPR is wrapped
inside an ANNOTATE_EXPR it gets handled by tsubst_expr and when
not, tsubst_stmt successfully handles it and the contained
DECL_EXPR in operand zero.

The following makes handling of COMPOUND_EXPR in tsubst_expr
consistent with that of tsubst_stmt for the operand that doesn't
specify the result and thus the reason we choose either or the
other for substing.

Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

Thanks,
Richard.

	PR c++/114409
gcc/cp/
	* pt.cc (tsubst_expr): Recurse to COMPOUND_EXPR operand
	zero using tsubst_stmt, when that returns NULL return
	the subst operand one, mimicing what tsubst_stmt does.

gcc/testsuite/
	* g++.dg/pr114409.C: New testcase.
---
 gcc/cp/pt.cc                    | 5 ++++-
 gcc/testsuite/g++.dg/pr114409.C | 8 ++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/pr114409.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index bf4b89d8413..dae423a751f 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -20635,8 +20635,11 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 
     case COMPOUND_EXPR:
       {
-	tree op0 = tsubst_expr (TREE_OPERAND (t, 0), args,
+	tree op0 = tsubst_stmt (TREE_OPERAND (t, 0), args,
 				complain & ~tf_decltype, in_decl);
+	if (op0 == NULL_TREE)
+	  /* If the first operand was a statement, we're done with it.  */
+	  RETURN (RECUR (TREE_OPERAND (t, 1)));
 	RETURN (build_x_compound_expr (EXPR_LOCATION (t),
 				       op0,
 				       RECUR (TREE_OPERAND (t, 1)),
diff --git a/gcc/testsuite/g++.dg/pr114409.C b/gcc/testsuite/g++.dg/pr114409.C
new file mode 100644
index 00000000000..6343fe8d9f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr114409.C
@@ -0,0 +1,8 @@
+// { dg-do compile }
+
+template <int> int t() {
+#pragma GCC unroll 4
+    while (int ThisEntry = 0) { } // { dg-bogus "ignoring loop annotation" "" { xfail *-*-* } }
+    return 0;
+}
+int tt = t<1>();
-- 
2.35.3

Re: [PATCH] c++/114409 - ANNOTATE_EXPR and templates

[Jakub Jelinek]

On Wed, Apr 10, 2024 at 06:43:02PM +0200, Richard Biener wrote:
> The following fixes a mismatch in COMPOUND_EXPR handling in
> tsubst_expr vs tsubst_stmt where the latter allows a stmt in
> operand zero but the former doesn't.  This makes a difference
> for the case at hand because when the COMPOUND_EXPR is wrapped
> inside an ANNOTATE_EXPR it gets handled by tsubst_expr and when
> not, tsubst_stmt successfully handles it and the contained
> DECL_EXPR in operand zero.
> 
> The following makes handling of COMPOUND_EXPR in tsubst_expr
> consistent with that of tsubst_stmt for the operand that doesn't
> specify the result and thus the reason we choose either or the
> other for substing.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> 
> Thanks,
> Richard.
> 
> 	PR c++/114409
> gcc/cp/
> 	* pt.cc (tsubst_expr): Recurse to COMPOUND_EXPR operand
> 	zero using tsubst_stmt, when that returns NULL return
> 	the subst operand one, mimicing what tsubst_stmt does.
> 
> gcc/testsuite/
> 	* g++.dg/pr114409.C: New testcase.

I've posted https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114409#c16
for this already and Jason agreed to that version, so I just have to test it
tonight:
https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649165.html

	Jakub

Re: [PATCH] c++/114409 - ANNOTATE_EXPR and templates

[Richard Biener]

On Wed, 10 Apr 2024, Jakub Jelinek wrote:

> On Wed, Apr 10, 2024 at 06:43:02PM +0200, Richard Biener wrote:
> > The following fixes a mismatch in COMPOUND_EXPR handling in
> > tsubst_expr vs tsubst_stmt where the latter allows a stmt in
> > operand zero but the former doesn't.  This makes a difference
> > for the case at hand because when the COMPOUND_EXPR is wrapped
> > inside an ANNOTATE_EXPR it gets handled by tsubst_expr and when
> > not, tsubst_stmt successfully handles it and the contained
> > DECL_EXPR in operand zero.
> > 
> > The following makes handling of COMPOUND_EXPR in tsubst_expr
> > consistent with that of tsubst_stmt for the operand that doesn't
> > specify the result and thus the reason we choose either or the
> > other for substing.
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> > 
> > Thanks,
> > Richard.
> > 
> > 	PR c++/114409
> > gcc/cp/
> > 	* pt.cc (tsubst_expr): Recurse to COMPOUND_EXPR operand
> > 	zero using tsubst_stmt, when that returns NULL return
> > 	the subst operand one, mimicing what tsubst_stmt does.
> > 
> > gcc/testsuite/
> > 	* g++.dg/pr114409.C: New testcase.
> 
> I've posted https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114409#c16
> for this already and Jason agreed to that version, so I just have to test it
> tonight:
> https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649165.html

Ah, I saw the bugzilla patches and wanted this version to be sent
because I think the COMPOUND_EXPR inconsistency is odd.  So Jason,
please still have a look, not necessarily because of the bug
which can be fixed in multiple ways but because of that COMPOUND_EXPR
handling oddity (there are already some cases in tsubst_expr that
explicitly recurse with tsubst_stmt).

Richard.

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

Re: [PATCH] c++/114409 - ANNOTATE_EXPR and templates

[Jakub Jelinek]

On Wed, Apr 10, 2024 at 07:10:52PM +0200, Richard Biener wrote:
> Ah, I saw the bugzilla patches and wanted this version to be sent
> because I think the COMPOUND_EXPR inconsistency is odd.  So Jason,
> please still have a look, not necessarily because of the bug
> which can be fixed in multiple ways but because of that COMPOUND_EXPR
> handling oddity (there are already some cases in tsubst_expr that
> explicitly recurse with tsubst_stmt).

I think if COMPOUND_EXPR appears in a context where only expressions but not
statements are allowed (say one of the operands of PLUS_EXPR/MINUS_EXPR/...
and hundreds of other places), then the operands of that COMPOUND_EXPR
shouldn't be statements either, so we should be using tsubst_expr rather
than tsubst_stmt on it for the recursion on the first operand and it should
never return NULL.  For statements, it can return NULL when the statement
is acutally emitted with add_stmt and so nothing more needs to be kept.
tsubst_stmt ends with
    default:
      gcc_assert (!STATEMENT_CODE_P (TREE_CODE (t)));
           
      RETURN (tsubst_expr (t, args, complain, in_decl));
so if something isn't handled by tsubst_stmt, it will handle it using
tsubst_expr.  But COMPOUND_EXPR is I think intentionally handled by both.
({ ... }) is handled separately in the STMT_EXPR tsubst_expr case, where
it calls tsubst_stmt after preparing stuff.

	Jakub

Re: [PATCH] c++/114409 - ANNOTATE_EXPR and templates

[Jason Merrill]

On 4/10/24 13:10, Richard Biener wrote:
> On Wed, 10 Apr 2024, Jakub Jelinek wrote:
> 
>> On Wed, Apr 10, 2024 at 06:43:02PM +0200, Richard Biener wrote:
>>> The following fixes a mismatch in COMPOUND_EXPR handling in
>>> tsubst_expr vs tsubst_stmt where the latter allows a stmt in
>>> operand zero but the former doesn't.  This makes a difference
>>> for the case at hand because when the COMPOUND_EXPR is wrapped
>>> inside an ANNOTATE_EXPR it gets handled by tsubst_expr and when
>>> not, tsubst_stmt successfully handles it and the contained
>>> DECL_EXPR in operand zero.
>>>
>>> The following makes handling of COMPOUND_EXPR in tsubst_expr
>>> consistent with that of tsubst_stmt for the operand that doesn't
>>> specify the result and thus the reason we choose either or the
>>> other for substing.
>>>
>>> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
>>>
>>> Thanks,
>>> Richard.
>>>
>>> 	PR c++/114409
>>> gcc/cp/
>>> 	* pt.cc (tsubst_expr): Recurse to COMPOUND_EXPR operand
>>> 	zero using tsubst_stmt, when that returns NULL return
>>> 	the subst operand one, mimicing what tsubst_stmt does.
>>>
>>> gcc/testsuite/
>>> 	* g++.dg/pr114409.C: New testcase.
>>
>> I've posted https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114409#c16
>> for this already and Jason agreed to that version, so I just have to test it
>> tonight:
>> https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649165.html
> 
> Ah, I saw the bugzilla patches and wanted this version to be sent
> because I think the COMPOUND_EXPR inconsistency is odd.  So Jason,
> please still have a look, not necessarily because of the bug
> which can be fixed in multiple ways but because of that COMPOUND_EXPR
> handling oddity (there are already some cases in tsubst_expr that
> explicitly recurse with tsubst_stmt).

The difference between tsubst_stmt and tsubst_expr handling of 
COMPOUND_EXPR seems consistent with the general difference between the 
two functions, so I think this change isn't needed.  The two existing 
uses of tsubst_stmt in tsubst_expr are statement-expressions (for the 
substatement) and transactions (strangely, non-statement transactions 
are handled in tsubst_stmt).

Jason