On 8/28/24 3:59 PM, Arsen Arsenović wrote:
This patch fixes a gcc-15 regression (PR116502) and an inelegance in my
earlier patch related to converting CO_AWAIT_EXPRs to void.
This wasn't noticed anywhere AFAICT, but it was a bit unfortunate.
The other two patches fix an actual regression as well as provide
handling for converting INDIRECT_REFs wrapping CO_AWAIT_EXPRs (which I
noticed was lacking while triaging the regression).
WRT INDIRECT_REFs, I've considered making convert_to_void recurse on the
INDIRECT_REF case, so that special handling doesn't need to be added
when some expression might end up getting wrapped in a INDIRECT_REF. Is
there a reason why we don't do that?
Primarily because that isn't what the language says. And for instance
if I have
int *volatile p;
*p;
I think just recursing will wrongly warn about not accessing the value of p.
Better, I think, to teach maybe_warn_nodiscard to look through
CO_AWAIT_EXPR.
I've started a regstrap with that
recursion added in order to see whether it is something to possibly
explore. The change being regstrapped looks like so:
diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index ce26cb8ef30..8f874f4f4dc 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1432,11 +1432,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
tsubst_flags_t complain)
&& !is_reference)
warning_at (loc, OPT_Wunused_value, "value computed is not
used");
expr = TREE_OPERAND (expr, 0);
- if (TREE_CODE (expr) == CALL_EXPR
- && (complain & tf_warning))
- maybe_warn_nodiscard (expr, implicit);
- else if (TREE_CODE (expr) == CO_AWAIT_EXPR)
- convert_co_await_to_void (expr, implicit, complain);
+ return convert_to_void (expr, implicit, complain);
}
break;
At any rate, the patchset was tested as-is on x86_64-pc-linux-gnu also.
If the above shouldn't be done for whatever reason, is the current set
OK for trunk?
TIA, have a lovely evening.
---------- >8 ----------
As of r15-2318-g2664c1bf83855b, we started altering co_await expressions
cast to void so that their constituent await-resume expression is also
cast to void. This leads to the TREE_TYPE of the CO_AWAIT_EXPR being
wrong, because it should be the same as the await-resume expression, but
it ended up being void. While this appears not to cause trouble
currently, it could induce bugs in the future, without being necessary.
gcc/cp/ChangeLog:
* coroutines.cc (co_await_get_resume_call): Don't return a
pointer to the await-resume expression.
* cp-tree.h (co_await_get_resume_call): Change return type to a
non-pointer 'tree'.
* cvt.cc (convert_to_void): Adjust according to the above.
---
gcc/cp/coroutines.cc | 4 ++--
gcc/cp/cp-tree.h | 2 +-
gcc/cp/cvt.cc | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index f243fe9adae2..166978ab464b 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -728,14 +728,14 @@ coro_get_destroy_function (tree decl)
/* Given a CO_AWAIT_EXPR AWAIT_EXPR, return its resume call. */
-tree*
+tree
co_await_get_resume_call (tree await_expr)
{
gcc_checking_assert (TREE_CODE (await_expr) == CO_AWAIT_EXPR);
tree vec = TREE_OPERAND (await_expr, 3);
if (!vec)
return nullptr;
- return &TREE_VEC_ELT (vec, 2);
+ return TREE_VEC_ELT (vec, 2);
}
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 2eeb5e3e8b16..417ed4919c39 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -8776,7 +8776,7 @@ extern tree coro_get_actor_function (tree);
extern tree coro_get_destroy_function (tree);
extern tree coro_get_ramp_function (tree);
-extern tree* co_await_get_resume_call (tree await_expr);
+extern tree co_await_get_resume_call (tree await_expr);
/* contracts.cc */
diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index 7b4bd8a9dc44..b6b35ef9cacf 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1505,8 +1505,8 @@ convert_to_void (tree expr, impl_conv_void implicit,
tsubst_flags_t complain)
case CO_AWAIT_EXPR:
{
auto awr = co_await_get_resume_call (expr);
- if (awr && *awr)
- *awr = convert_to_void (*awr, implicit, complain);
+ if (awr)
+ awr = convert_to_void (awr, implicit, complain);
After this change you don't need the assignment at all, it's a dead store.
break;