diff mbox series

[3/3] c++/coro: prevent ICV_STATEMENT diagnostics in temp promotion [PR116502]

Message ID 20240828202119.17640-4-arsen@aarsen.me
State New
Headers show
Series [1/3] c++: stop altering co_awaits converted to void | expand

Commit Message

Arsen Arsenović Aug. 28, 2024, 7:59 p.m. UTC 
Hm, maybe-unused-1.C should be moved into the previous patch.  Could do
before pushing.
---------- >8 ----------
If such a diagnostic is necessary, it has already been emitted,
otherwise, it is not correct and emitting it here is inactionable by the
user, and bogus.

	PR c++/116502

gcc/cp/ChangeLog:

	* coroutines.cc (maybe_promote_temps): Convert temporary
	initializers to void without complaining.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/maybe-unused-1.C: New test.
	* g++.dg/coroutines/pr116502.C: New test.
---
 gcc/cp/coroutines.cc                          | 12 +++++--
 .../g++.dg/coroutines/maybe-unused-1.C        | 33 +++++++++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr116502.C    | 33 +++++++++++++++++++
 3 files changed, 75 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/maybe-unused-1.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116502.C

Comments

Jason Merrill Aug. 29, 2024, 4:09 p.m. UTC | #1 
On 8/28/24 3:59 PM, Arsen Arsenović wrote:
> Hm, maybe-unused-1.C should be moved into the previous patch.  Could do
> before pushing.

Let's combine these three into a single patch, they're all small and 
closely related.

> @@ -3147,7 +3147,13 @@ maybe_promote_temps (tree *stmt, void *d)
>   	 to run the initializer.
>   	 If the initializer is a conditional expression, we need to collect
>   	 and declare any promoted variables nested within it.  DTORs for such
> -	 variables must be run conditionally too.  */
> +	 variables must be run conditionally too.
> +
> +	 Since here we're synthetically processing code here, we've already
> +	 emitted any Wunused-result warnings.

If it's already been through convert_to_void for the warnings, why isn't 
it already void?

>>> 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.
> 
> Hmm, with the change in the OP I didn't get that warning, but that
> reason is compelling enough for me.

On second thought, I think we should split the INDIRECT_REF handling 
between REFERENCE_REF_P and actual * expressions, and recurse for 
warnings in the reference case (but don't return the value of the 
recursive call).  And drop the warn_nodiscard from the * case; we 
shouldn't ever get a nodiscard warning about *f();.

Jason
Arsen Arsenović Aug. 30, 2024, 12:09 a.m. UTC | #2 
Jason Merrill <jason@redhat.com> writes:

> On 8/28/24 3:59 PM, Arsen Arsenović wrote:
>> Hm, maybe-unused-1.C should be moved into the previous patch.  Could do
>> before pushing.
>
> Let's combine these three into a single patch, they're all small and closely
> related.

Sure, that's fine by me.

>> @@ -3147,7 +3147,13 @@ maybe_promote_temps (tree *stmt, void *d)
>>   	 to run the initializer.
>>   	 If the initializer is a conditional expression, we need to collect
>>   	 and declare any promoted variables nested within it.  DTORs for such
>> -	 variables must be run conditionally too.  */
>> +	 variables must be run conditionally too.
>> +
>> +	 Since here we're synthetically processing code here, we've already
>> +	 emitted any Wunused-result warnings.
>
> If it's already been through convert_to_void for the warnings, why isn't it
> already void?

The conversions making it void can get lost during the flattening and
transformation of statements containing CO_AWAIT_EXPRs (or, more
accurately, the CO_AWAIT_EXPRs get isolated without the conversion
making it void, it'd seem).

>>>> 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.
>> Hmm, with the change in the OP I didn't get that warning, but that
>> reason is compelling enough for me.
>
> On second thought, I think we should split the INDIRECT_REF handling between
> REFERENCE_REF_P and actual * expressions, and recurse for warnings in the
> reference case (but don't return the value of the recursive call).  And drop
> the warn_nodiscard from the * case; we shouldn't ever get a nodiscard warning
> about *f();.

This seems reasonable to me, but by not returning the value of the
recursive call, we might get double diagnostics.  Consider, for
instance: void h(int* x) { static_cast<int&>(*x); }

Here, stripping away the INDIRECT_REF leads and calling convert_to_void
on the NOP_EXPR results in a "statement has no effect" warning.  If we
then continue with executing the outer convert_to_void, we get that
diagnostic again (assuming you intended the convert_to_void to be
discarded).  I might be misunderstanding you, but I started with the
patch below:

modified   gcc/cp/cvt.cc
@@ -1403,7 +1403,15 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
 		    gcc_unreachable ();
 		}
 	  }
-	if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE (type))
+
+	if (is_reference)
+	  {
+            expr = TREE_OPERAND (expr, 0);
+	    convert_to_void (expr, implicit, complain);
+	    break;
+	  }
+
+	if (!is_volatile || !is_complete || TREE_ADDRESSABLE (type))
           {
             /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
                operation is stripped off. Note that we don't warn about
@@ -1417,10 +1428,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
                 && !warning_suppressed_p (expr, OPT_Wunused_value)
                 && !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);
           }
 
 	break;

... this results in a double diagnostic.  Altering the is_reference case
to be:

	if (is_reference)
	  {
	    expr = convert_to_void (TREE_OPERAND (expr, 0), implicit, complain);
	    break;
	  }

... prevents the double diagnostic, but in that case, we'd probably not
get any more work later as EXPR would already be void, so it seems
redundant.
diff mbox series

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 166978ab464b..780dc506825b 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3147,7 +3147,13 @@  maybe_promote_temps (tree *stmt, void *d)
 	 to run the initializer.
 	 If the initializer is a conditional expression, we need to collect
 	 and declare any promoted variables nested within it.  DTORs for such
-	 variables must be run conditionally too.  */
+	 variables must be run conditionally too.
+
+	 Since here we're synthetically processing code here, we've already
+	 emitted any Wunused-result warnings.  Below, however, we call
+	 finish_expr_stmt, which will convert its operand to void, and could
+	 result in such a diagnostic being emitted.  To avoid that, convert to
+	 void ahead of time.  */
       if (t->var)
 	{
 	  tree var = t->var;
@@ -3157,7 +3163,7 @@  maybe_promote_temps (tree *stmt, void *d)
 	  if (TREE_CODE (t->init) == COND_EXPR)
 	    process_conditional (t, vlist);
 	  else
-	    finish_expr_stmt (t->init);
+	    finish_expr_stmt (convert_to_void (t->init, ICV_STATEMENT, tf_none));
 	  if (tree cleanup = cxx_maybe_build_cleanup (var, tf_warning_or_error))
 	    {
 	      tree cl = build_stmt (sloc, CLEANUP_STMT, expr_list, cleanup, var);
@@ -3176,7 +3182,7 @@  maybe_promote_temps (tree *stmt, void *d)
 	  if (TREE_CODE (t->init) == COND_EXPR)
 	    process_conditional (t, vlist);
 	  else
-	    finish_expr_stmt (t->init);
+	    finish_expr_stmt (convert_to_void (t->init, ICV_STATEMENT, tf_none));
 	  if (expr_list)
 	    {
 	      if (TREE_CODE (expr_list) != STATEMENT_LIST)
diff --git a/gcc/testsuite/g++.dg/coroutines/maybe-unused-1.C b/gcc/testsuite/g++.dg/coroutines/maybe-unused-1.C
new file mode 100644
index 000000000000..68d59d83e8eb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/maybe-unused-1.C
@@ -0,0 +1,33 @@ 
+// https://gcc.gnu.org/PR116502
+#include <coroutine>
+
+struct SuspendNever {
+  bool await_ready() noexcept;
+  void await_suspend(std::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct Coroutine;
+
+struct PromiseType {
+  Coroutine get_return_object();
+  SuspendNever initial_suspend();
+  SuspendNever final_suspend() noexcept;
+  void return_void();
+  void unhandled_exception();
+};
+
+struct Coroutine {
+  using promise_type = PromiseType;
+};
+
+struct Awaiter {
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  [[nodiscard]] int& await_resume();
+};
+
+Coroutine foo()
+{
+  co_await Awaiter {}; // { dg-warning "Wunused-result" }
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr116502.C b/gcc/testsuite/g++.dg/coroutines/pr116502.C
new file mode 100644
index 000000000000..95cc0bc8a983
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr116502.C
@@ -0,0 +1,33 @@ 
+// https://gcc.gnu.org/PR116502
+#include <coroutine>
+
+struct SuspendNever {
+  bool await_ready() noexcept;
+  void await_suspend(std::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct Coroutine;
+
+struct PromiseType {
+  Coroutine get_return_object();
+  SuspendNever initial_suspend();
+  SuspendNever final_suspend() noexcept;
+  void return_void();
+  void unhandled_exception();
+};
+
+struct Coroutine {
+  using promise_type = PromiseType;
+};
+
+struct Awaiter {
+  bool await_ready();
+  void await_suspend(std::coroutine_handle<>);
+  [[nodiscard]] int& await_resume();
+};
+
+Coroutine foo()
+{
+  (void)co_await Awaiter {};
+}