Message ID | 20240410164306.EFDB113942@imap1.dmz-prg2.suse.org |
---|---|
State | New |
Headers | show |
Series | c++/114409 - ANNOTATE_EXPR and templates | expand |
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
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.
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
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
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>();