[PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]

[PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]

[Patrick Palka]

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?

-- >8 --

Here convert_to_void always completes the type of an indirection or
id-expression, but according to [expr.context] an lvalue-to-rvalue
conversion is applied to a discarded-value expression only if "the
expression is a glvalue of volatile-qualified type".  This patch
restricts convert_to_void's type completion to match.

	PR c++/111419

gcc/cp/ChangeLog:

	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
	complete_type if the type is volatile.
	<case VAR_DECL>: Only call complete_type if the type is volatile.
	Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-requires36.C: New test.
	* g++.dg/expr/discarded1.C: New test.
	* g++.dg/expr/discarded1a.C: New test.
---
 gcc/cp/cvt.cc                                   | 10 ++++++----
 .../g++.dg/cpp2a/concepts-requires36.C          | 16 ++++++++++++++++
 gcc/testsuite/g++.dg/expr/discarded1.C          | 15 +++++++++++++++
 gcc/testsuite/g++.dg/expr/discarded1a.C         | 17 +++++++++++++++++
 4 files changed, 54 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
 create mode 100644 gcc/testsuite/g++.dg/expr/discarded1.C
 create mode 100644 gcc/testsuite/g++.dg/expr/discarded1a.C

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index c6b52f07050..4424670356c 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1250,9 +1250,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
     case INDIRECT_REF:
       {
 	tree type = TREE_TYPE (expr);
-	int is_reference = TYPE_REF_P (TREE_TYPE (TREE_OPERAND (expr, 0)));
 	int is_volatile = TYPE_VOLATILE (type);
-	int is_complete = COMPLETE_TYPE_P (complete_type (type));
+	if (is_volatile)
+	  complete_type (type);
+	int is_complete = COMPLETE_TYPE_P (type);
 
 	/* Can't load the value if we don't know the type.  */
 	if (is_volatile && !is_complete)
@@ -1412,9 +1413,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
       {
 	/* External variables might be incomplete.  */
 	tree type = TREE_TYPE (expr);
-	int is_complete = COMPLETE_TYPE_P (complete_type (type));
 
-	if (TYPE_VOLATILE (type) && !is_complete && (complain & tf_warning))
+	if (TYPE_VOLATILE (type)
+	    && !COMPLETE_TYPE_P (complete_type (type))
+	    && (complain & tf_warning))
 	  switch (implicit)
 	    {
 	      case ICV_CAST:
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
new file mode 100644
index 00000000000..8d3a4fcd2aa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
@@ -0,0 +1,16 @@
+// PR c++/111419
+// { dg-do compile { target c++20 } }
+
+template<class F>
+concept invocable = requires(F& f) { f(); };
+
+template<class F>
+concept deref_invocable = requires(F& f) { *f(); };
+
+struct Incomplete;
+
+template<class T>
+struct Holder { T t; };
+
+static_assert(invocable<Holder<Incomplete>& ()>);
+static_assert(deref_invocable<Holder<Incomplete>* ()>);
diff --git a/gcc/testsuite/g++.dg/expr/discarded1.C b/gcc/testsuite/g++.dg/expr/discarded1.C
new file mode 100644
index 00000000000..c0c22e9e95b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/discarded1.C
@@ -0,0 +1,15 @@
+// PR c++/111419
+
+struct Incomplete;
+
+template<class T> struct Holder { T t; }; // { dg-bogus "incomplete" }
+
+extern Holder<Incomplete> a;
+extern Holder<Incomplete>& b;
+extern Holder<Incomplete>* c;
+
+int main() {
+  a;
+  b;
+  *c;
+}
diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
new file mode 100644
index 00000000000..1c4dff4553e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
@@ -0,0 +1,17 @@
+// A version of discarded1.C using volatile types.
+// PR c++/111419
+
+struct Incomplete;
+
+template<class T, int> struct Holder { T t; }; // { dg-error "incomplete" }
+
+extern volatile Holder<Incomplete, 0> a;
+extern volatile Holder<Incomplete, 1>& b;
+extern volatile Holder<Incomplete, 2>* c;
+
+int main() {
+  a; // { dg-message "required from here" }
+  b; // { dg-message "required from here" }
+  // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
+  *c; // { dg-message "required from here" }
+}
-- 
2.42.0.216.gbda494f404

[PATCH v2 2/2] c++: convert_to_void and volatile references

[Patrick Palka]

Jason pointed out that even implicit loads of volatile references need
to undergo lvalue-to-rvalue conversion, but we currently emit a warning
in this case and discard the load.  This patch changes this behavior so
that we don't issue a warning, and preserve the load.

gcc/cp/ChangeLog:

	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
	for an implicit load of a volatile reference.  Simplify as if
	is_reference is false.  Check REFERENCE_REF_P in the test
	guarding the -Wunused-value diagnostic.

gcc/testsuite/ChangeLog:

	* g++.dg/expr/discarded1a.C: No longer expect warning for
	implicit load of a volatile reference.
	* g++.old-deja/g++.bugs/900428_01.C: Likewise.
	* g++.dg/expr/volatile2.C: New test.
---
 gcc/cp/cvt.cc                                 | 56 ++-----------------
 gcc/testsuite/g++.dg/expr/discarded1a.C       |  1 -
 gcc/testsuite/g++.dg/expr/volatile2.C         | 12 ++++
 .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++-----
 4 files changed, 30 insertions(+), 65 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index 4424670356c..1cb6c1222c2 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
       {
 	tree type = TREE_TYPE (expr);
 	int is_volatile = TYPE_VOLATILE (type);
-	if (is_volatile)
-	  complete_type (type);
-	int is_complete = COMPLETE_TYPE_P (type);
 
 	/* Can't load the value if we don't know the type.  */
-	if (is_volatile && !is_complete)
+	if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
           {
             if (complain & tf_warning)
 	      switch (implicit)
@@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
 		    gcc_unreachable ();
 		}
           }
-	/* Don't load the value if this is an implicit dereference, or if
-	   the type needs to be handled by ctors/dtors.  */
-	else if (is_volatile && is_reference)
-          {
-            if (complain & tf_warning)
-	      switch (implicit)
-		{
-	      	  case ICV_CAST:
-		    warning_at (loc, 0, "conversion to void will not access "
-				"object of type %qT", type);
-		    break;
-		  case ICV_SECOND_OF_COND:
-		    warning_at (loc, 0, "implicit dereference will not access "
-				"object of type %qT in second operand of "
-				"conditional expression", type);
-		    break;
-		  case ICV_THIRD_OF_COND:
-		    warning_at (loc, 0, "implicit dereference will not access "
-				"object of type %qT in third operand of "
-				"conditional expression", type);
-		    break;
-		  case ICV_RIGHT_OF_COMMA:
-		    warning_at (loc, 0, "implicit dereference will not access "
-				"object of type %qT in right operand of "
-				"comma operator", type);
-		    break;
-		  case ICV_LEFT_OF_COMMA:
-		    warning_at (loc, 0, "implicit dereference will not access "
-				"object of type %qT in left operand of comma "
-				"operator", type);
-		    break;
-		  case ICV_STATEMENT:
-		    warning_at (loc, 0, "implicit dereference will not access "
-				"object of type %qT in statement",  type);
-		     break;
-		  case ICV_THIRD_IN_FOR:
-		    warning_at (loc, 0, "implicit dereference will not access "
-				"object of type %qT in for increment expression",
-				type);
-		    break;
-		  default:
-		    gcc_unreachable ();
-		}
-          }
+	/* Don't load the value if the type needs to be handled by cdtors.  */
 	else if (is_volatile && TREE_ADDRESSABLE (type))
 	  {
 	    if (complain & tf_warning)
@@ -1386,7 +1340,7 @@ 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_volatile || !COMPLETE_TYPE_P (type))
           {
             /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
                operation is stripped off. Note that we don't warn about
@@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
             if (warn_unused_value
 		&& implicit != ICV_CAST
                 && (complain & tf_warning)
-                && !warning_suppressed_p (expr, OPT_Wunused_value)
-                && !is_reference)
+		&& !warning_suppressed_p (expr, OPT_Wunused_value)
+		&& !REFERENCE_REF_P (expr))
               warning_at (loc, OPT_Wunused_value, "value computed is not used");
             expr = TREE_OPERAND (expr, 0);
 	    if (TREE_CODE (expr) == CALL_EXPR
diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
index 1c4dff4553e..4754f32f716 100644
--- a/gcc/testsuite/g++.dg/expr/discarded1a.C
+++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
@@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
 int main() {
   a; // { dg-message "required from here" }
   b; // { dg-message "required from here" }
-  // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
   *c; // { dg-message "required from here" }
 }
diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C b/gcc/testsuite/g++.dg/expr/volatile2.C
new file mode 100644
index 00000000000..62e609aee2f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/volatile2.C
@@ -0,0 +1,12 @@
+// Verify we preserve implicit loads of volatile references in a
+// discarded-value expression.
+// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized" }
+// { dg-final { scan-tree-dump {\*a} "original" } }
+// { dg-final { scan-tree-dump {\*a} "optimized" } }
+
+struct A { int m; };
+extern volatile A& a;
+
+int main() {
+  a;
+}
diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
index a806ef0706e..2b82c64ff73 100644
--- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
+++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
@@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r, volatile int &vr)
   (void)(i ? r : j);	        // ok, no warning
   (void)((void)1, r);	        // ok, no warning
 
-  vr;				// { dg-warning "" } reference not accessed
-  (void)vr;			// { dg-warning "" } reference not accessed
-  (void)(i ? vj : vr);	        // { dg-warning "" } reference not accessed
-  (void)(i ? vr : vj);	        // { dg-warning "" } reference not accessed
-  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
+  vr;				// ok, no warning
+  (void)vr;			// ok, no warning
+  (void)(i ? vj : vr);	        // ok, no warning
+  (void)(i ? vr : vj);	        // ok, no warning
+  (void)((void)1, vr);          // ok, no warning
   
   *ip_fn ();			// ok, no warning
   *vip_fn ();			// ok, no warning
   ir_fn ();			// ok, no warning
-  vir_fn ();			// { dg-warning "" } reference not accessed
+  vir_fn ();			// ok, no warning
 }
 
 struct S;
@@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r, volatile T &vr)
   (void)(i ? r : j);	        // ok, no warning
   (void)((void)1, r);	        // ok, no warning
 
-  vr;				// { dg-warning "" } reference not accessed
-  (void)vr;			// { dg-warning "" } reference not accessed
-  (void)(i ? vj : vr);	        // { dg-warning "" } reference not accessed
-  (void)(i ? vr : vj);	        // { dg-warning "" } reference not accessed
-  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
+  vr;				// ok, no warning
+  (void)vr;			// ok, no warning
+  (void)(i ? vj : vr);	        // ok, no warning
+  (void)(i ? vr : vj);	        // ok, no warning
+  (void)((void)1, vr);          // ok, no warning
   
   *tp_fn ();			// ok, no warning
   *vtp_fn ();			// ok, no warning
   tr_fn ();			// ok, no warning
-  vtr_fn ();			// ok, no warning{ dg-warning "" } reference not accessed
+  vtr_fn ();			// ok, no warning
 }
 
 void extern_test ()
@@ -160,5 +160,5 @@ void extern_test ()
   esr;				// ok, no warning
   vesr;				// { dg-warning "" } incomplete not accessed
   etr;				// ok, no warning
-  vetr;				// { dg-warning "" } reference not accessed
+  vetr;				// ok, no warning
 }
-- 
2.42.0.216.gbda494f404

Re: [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]

[Patrick Palka]

On Mon, 18 Sep 2023, Patrick Palka wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> -- >8 --
> 
> Here convert_to_void always completes the type of an indirection or
> id-expression, but according to [expr.context] an lvalue-to-rvalue
> conversion is applied to a discarded-value expression only if "the
> expression is a glvalue of volatile-qualified type".  This patch
> restricts convert_to_void's type completion to match.
> 
> 	PR c++/111419
> 
> gcc/cp/ChangeLog:
> 
> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> 	complete_type if the type is volatile.
> 	<case VAR_DECL>: Only call complete_type if the type is volatile.
> 	Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-requires36.C: New test.
> 	* g++.dg/expr/discarded1.C: New test.
> 	* g++.dg/expr/discarded1a.C: New test.
> ---
>  gcc/cp/cvt.cc                                   | 10 ++++++----
>  .../g++.dg/cpp2a/concepts-requires36.C          | 16 ++++++++++++++++
>  gcc/testsuite/g++.dg/expr/discarded1.C          | 15 +++++++++++++++
>  gcc/testsuite/g++.dg/expr/discarded1a.C         | 17 +++++++++++++++++
>  4 files changed, 54 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>  create mode 100644 gcc/testsuite/g++.dg/expr/discarded1.C
>  create mode 100644 gcc/testsuite/g++.dg/expr/discarded1a.C
> 
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index c6b52f07050..4424670356c 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1250,9 +1250,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>      case INDIRECT_REF:
>        {
>  	tree type = TREE_TYPE (expr);
> -	int is_reference = TYPE_REF_P (TREE_TYPE (TREE_OPERAND (expr, 0)));

Oops, this removal of is_reference should happen in the follow-up patch
instead, consider that fixed.

>  	int is_volatile = TYPE_VOLATILE (type);
> -	int is_complete = COMPLETE_TYPE_P (complete_type (type));
> +	if (is_volatile)
> +	  complete_type (type);
> +	int is_complete = COMPLETE_TYPE_P (type);
>  
>  	/* Can't load the value if we don't know the type.  */
>  	if (is_volatile && !is_complete)
> @@ -1412,9 +1413,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>        {
>  	/* External variables might be incomplete.  */
>  	tree type = TREE_TYPE (expr);
> -	int is_complete = COMPLETE_TYPE_P (complete_type (type));
>  
> -	if (TYPE_VOLATILE (type) && !is_complete && (complain & tf_warning))
> +	if (TYPE_VOLATILE (type)
> +	    && !COMPLETE_TYPE_P (complete_type (type))
> +	    && (complain & tf_warning))
>  	  switch (implicit)
>  	    {
>  	      case ICV_CAST:
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
> new file mode 100644
> index 00000000000..8d3a4fcd2aa
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
> @@ -0,0 +1,16 @@
> +// PR c++/111419
> +// { dg-do compile { target c++20 } }
> +
> +template<class F>
> +concept invocable = requires(F& f) { f(); };
> +
> +template<class F>
> +concept deref_invocable = requires(F& f) { *f(); };
> +
> +struct Incomplete;
> +
> +template<class T>
> +struct Holder { T t; };
> +
> +static_assert(invocable<Holder<Incomplete>& ()>);
> +static_assert(deref_invocable<Holder<Incomplete>* ()>);
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1.C b/gcc/testsuite/g++.dg/expr/discarded1.C
> new file mode 100644
> index 00000000000..c0c22e9e95b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/discarded1.C
> @@ -0,0 +1,15 @@
> +// PR c++/111419
> +
> +struct Incomplete;
> +
> +template<class T> struct Holder { T t; }; // { dg-bogus "incomplete" }
> +
> +extern Holder<Incomplete> a;
> +extern Holder<Incomplete>& b;
> +extern Holder<Incomplete>* c;
> +
> +int main() {
> +  a;
> +  b;
> +  *c;
> +}
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> new file mode 100644
> index 00000000000..1c4dff4553e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -0,0 +1,17 @@
> +// A version of discarded1.C using volatile types.
> +// PR c++/111419
> +
> +struct Incomplete;
> +
> +template<class T, int> struct Holder { T t; }; // { dg-error "incomplete" }
> +
> +extern volatile Holder<Incomplete, 0> a;
> +extern volatile Holder<Incomplete, 1>& b;
> +extern volatile Holder<Incomplete, 2>* c;
> +
> +int main() {
> +  a; // { dg-message "required from here" }
> +  b; // { dg-message "required from here" }
> +  // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
> +  *c; // { dg-message "required from here" }
> +}
> -- 
> 2.42.0.216.gbda494f404
> 
> 

Re: [PATCH v2 2/2] c++: convert_to_void and volatile references

[Patrick Palka]

On Mon, 18 Sep 2023, Patrick Palka wrote:

> Jason pointed out that even implicit loads of volatile references need
> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> in this case and discard the load.  This patch changes this behavior so
> that we don't issue a warning, and preserve the load.
> 
> gcc/cp/ChangeLog:
> 
> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
> 	for an implicit load of a volatile reference.  Simplify as if
> 	is_reference is false.  Check REFERENCE_REF_P in the test
> 	guarding the -Wunused-value diagnostic.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/expr/discarded1a.C: No longer expect warning for
> 	implicit load of a volatile reference.
> 	* g++.old-deja/g++.bugs/900428_01.C: Likewise.
> 	* g++.dg/expr/volatile2.C: New test.
> ---
>  gcc/cp/cvt.cc                                 | 56 ++-----------------
>  gcc/testsuite/g++.dg/expr/discarded1a.C       |  1 -
>  gcc/testsuite/g++.dg/expr/volatile2.C         | 12 ++++
>  .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++-----
>  4 files changed, 30 insertions(+), 65 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
> 
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index 4424670356c..1cb6c1222c2 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>        {
>  	tree type = TREE_TYPE (expr);
>  	int is_volatile = TYPE_VOLATILE (type);
> -	if (is_volatile)
> -	  complete_type (type);
> -	int is_complete = COMPLETE_TYPE_P (type);
>  
>  	/* Can't load the value if we don't know the type.  */
> -	if (is_volatile && !is_complete)
> +	if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
>            {
>              if (complain & tf_warning)
>  	      switch (implicit)
> @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>  		    gcc_unreachable ();
>  		}
>            }
> -	/* Don't load the value if this is an implicit dereference, or if
> -	   the type needs to be handled by ctors/dtors.  */
> -	else if (is_volatile && is_reference)
> -          {
> -            if (complain & tf_warning)
> -	      switch (implicit)
> -		{
> -	      	  case ICV_CAST:
> -		    warning_at (loc, 0, "conversion to void will not access "
> -				"object of type %qT", type);
> -		    break;
> -		  case ICV_SECOND_OF_COND:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in second operand of "
> -				"conditional expression", type);
> -		    break;
> -		  case ICV_THIRD_OF_COND:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in third operand of "
> -				"conditional expression", type);
> -		    break;
> -		  case ICV_RIGHT_OF_COMMA:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in right operand of "
> -				"comma operator", type);
> -		    break;
> -		  case ICV_LEFT_OF_COMMA:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in left operand of comma "
> -				"operator", type);
> -		    break;
> -		  case ICV_STATEMENT:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in statement",  type);
> -		     break;
> -		  case ICV_THIRD_IN_FOR:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in for increment expression",
> -				type);
> -		    break;
> -		  default:
> -		    gcc_unreachable ();
> -		}
> -          }
> +	/* Don't load the value if the type needs to be handled by cdtors.  */
>  	else if (is_volatile && TREE_ADDRESSABLE (type))
>  	  {
>  	    if (complain & tf_warning)
> @@ -1386,7 +1340,7 @@ 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_volatile || !COMPLETE_TYPE_P (type))

... and the TREE_ADDRESSABLE test should not have been removed, consider
that fixed.  (The original patch doesn't have this mistake, I just
messed up splitting the patch into two.)

>            {
>              /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
>                 operation is stripped off. Note that we don't warn about
> @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>              if (warn_unused_value
>  		&& implicit != ICV_CAST
>                  && (complain & tf_warning)
> -                && !warning_suppressed_p (expr, OPT_Wunused_value)
> -                && !is_reference)
> +		&& !warning_suppressed_p (expr, OPT_Wunused_value)
> +		&& !REFERENCE_REF_P (expr))
>                warning_at (loc, OPT_Wunused_value, "value computed is not used");
>              expr = TREE_OPERAND (expr, 0);
>  	    if (TREE_CODE (expr) == CALL_EXPR
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> index 1c4dff4553e..4754f32f716 100644
> --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
>  int main() {
>    a; // { dg-message "required from here" }
>    b; // { dg-message "required from here" }
> -  // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
>    *c; // { dg-message "required from here" }
>  }
> diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C b/gcc/testsuite/g++.dg/expr/volatile2.C
> new file mode 100644
> index 00000000000..62e609aee2f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> @@ -0,0 +1,12 @@
> +// Verify we preserve implicit loads of volatile references in a
> +// discarded-value expression.
> +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized" }
> +// { dg-final { scan-tree-dump {\*a} "original" } }
> +// { dg-final { scan-tree-dump {\*a} "optimized" } }
> +
> +struct A { int m; };
> +extern volatile A& a;
> +
> +int main() {
> +  a;
> +}
> diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> index a806ef0706e..2b82c64ff73 100644
> --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r, volatile int &vr)
>    (void)(i ? r : j);	        // ok, no warning
>    (void)((void)1, r);	        // ok, no warning
>  
> -  vr;				// { dg-warning "" } reference not accessed
> -  (void)vr;			// { dg-warning "" } reference not accessed
> -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not accessed
> -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not accessed
> -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
> +  vr;				// ok, no warning
> +  (void)vr;			// ok, no warning
> +  (void)(i ? vj : vr);	        // ok, no warning
> +  (void)(i ? vr : vj);	        // ok, no warning
> +  (void)((void)1, vr);          // ok, no warning
>    
>    *ip_fn ();			// ok, no warning
>    *vip_fn ();			// ok, no warning
>    ir_fn ();			// ok, no warning
> -  vir_fn ();			// { dg-warning "" } reference not accessed
> +  vir_fn ();			// ok, no warning
>  }
>  
>  struct S;
> @@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r, volatile T &vr)
>    (void)(i ? r : j);	        // ok, no warning
>    (void)((void)1, r);	        // ok, no warning
>  
> -  vr;				// { dg-warning "" } reference not accessed
> -  (void)vr;			// { dg-warning "" } reference not accessed
> -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not accessed
> -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not accessed
> -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
> +  vr;				// ok, no warning
> +  (void)vr;			// ok, no warning
> +  (void)(i ? vj : vr);	        // ok, no warning
> +  (void)(i ? vr : vj);	        // ok, no warning
> +  (void)((void)1, vr);          // ok, no warning
>    
>    *tp_fn ();			// ok, no warning
>    *vtp_fn ();			// ok, no warning
>    tr_fn ();			// ok, no warning
> -  vtr_fn ();			// ok, no warning{ dg-warning "" } reference not accessed
> +  vtr_fn ();			// ok, no warning
>  }
>  
>  void extern_test ()
> @@ -160,5 +160,5 @@ void extern_test ()
>    esr;				// ok, no warning
>    vesr;				// { dg-warning "" } incomplete not accessed
>    etr;				// ok, no warning
> -  vetr;				// { dg-warning "" } reference not accessed
> +  vetr;				// ok, no warning
>  }
> -- 
> 2.42.0.216.gbda494f404
> 
> 

Re: [PATCH v2 2/2] c++: convert_to_void and volatile references

[Jason Merrill]

On 9/18/23 12:12, Patrick Palka wrote:
> Jason pointed out that even implicit loads of volatile references need
> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> in this case and discard the load.  This patch changes this behavior so
> that we don't issue a warning, and preserve the load.
> 
> gcc/cp/ChangeLog:
> 
> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
> 	for an implicit load of a volatile reference.  Simplify as if
> 	is_reference is false.  Check REFERENCE_REF_P in the test
> 	guarding the -Wunused-value diagnostic.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/expr/discarded1a.C: No longer expect warning for
> 	implicit load of a volatile reference.
> 	* g++.old-deja/g++.bugs/900428_01.C: Likewise.
> 	* g++.dg/expr/volatile2.C: New test.
> ---
>   gcc/cp/cvt.cc                                 | 56 ++-----------------
>   gcc/testsuite/g++.dg/expr/discarded1a.C       |  1 -
>   gcc/testsuite/g++.dg/expr/volatile2.C         | 12 ++++
>   .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++-----
>   4 files changed, 30 insertions(+), 65 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
> 
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index 4424670356c..1cb6c1222c2 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>         {
>   	tree type = TREE_TYPE (expr);
>   	int is_volatile = TYPE_VOLATILE (type);
> -	if (is_volatile)
> -	  complete_type (type);
> -	int is_complete = COMPLETE_TYPE_P (type);
>   
>   	/* Can't load the value if we don't know the type.  */
> -	if (is_volatile && !is_complete)
> +	if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
>             {
>               if (complain & tf_warning)
>   	      switch (implicit)
> @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>   		    gcc_unreachable ();
>   		}
>             }
> -	/* Don't load the value if this is an implicit dereference, or if
> -	   the type needs to be handled by ctors/dtors.  */
> -	else if (is_volatile && is_reference)
> -          {
> -            if (complain & tf_warning)
> -	      switch (implicit)
> -		{
> -	      	  case ICV_CAST:
> -		    warning_at (loc, 0, "conversion to void will not access "
> -				"object of type %qT", type);
> -		    break;
> -		  case ICV_SECOND_OF_COND:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in second operand of "
> -				"conditional expression", type);
> -		    break;
> -		  case ICV_THIRD_OF_COND:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in third operand of "
> -				"conditional expression", type);
> -		    break;
> -		  case ICV_RIGHT_OF_COMMA:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in right operand of "
> -				"comma operator", type);
> -		    break;
> -		  case ICV_LEFT_OF_COMMA:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in left operand of comma "
> -				"operator", type);
> -		    break;
> -		  case ICV_STATEMENT:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in statement",  type);
> -		     break;
> -		  case ICV_THIRD_IN_FOR:
> -		    warning_at (loc, 0, "implicit dereference will not access "
> -				"object of type %qT in for increment expression",
> -				type);
> -		    break;
> -		  default:
> -		    gcc_unreachable ();
> -		}
> -          }
> +	/* Don't load the value if the type needs to be handled by cdtors.  */
>   	else if (is_volatile && TREE_ADDRESSABLE (type))
>   	  {
>   	    if (complain & tf_warning)
> @@ -1386,7 +1340,7 @@ 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_volatile || !COMPLETE_TYPE_P (type))
>             {
>               /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
>                  operation is stripped off. Note that we don't warn about
> @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>               if (warn_unused_value
>   		&& implicit != ICV_CAST
>                   && (complain & tf_warning)
> -                && !warning_suppressed_p (expr, OPT_Wunused_value)
> -                && !is_reference)
> +		&& !warning_suppressed_p (expr, OPT_Wunused_value)
> +		&& !REFERENCE_REF_P (expr))
>                 warning_at (loc, OPT_Wunused_value, "value computed is not used");
>               expr = TREE_OPERAND (expr, 0);
>   	    if (TREE_CODE (expr) == CALL_EXPR
> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> index 1c4dff4553e..4754f32f716 100644
> --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
>   int main() {
>     a; // { dg-message "required from here" }
>     b; // { dg-message "required from here" }
> -  // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
>     *c; // { dg-message "required from here" }
>   }
> diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C b/gcc/testsuite/g++.dg/expr/volatile2.C
> new file mode 100644
> index 00000000000..62e609aee2f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> @@ -0,0 +1,12 @@
> +// Verify we preserve implicit loads of volatile references in a
> +// discarded-value expression.
> +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized" }
> +// { dg-final { scan-tree-dump {\*a} "original" } }
> +// { dg-final { scan-tree-dump {\*a} "optimized" } }
> +
> +struct A { int m; };
> +extern volatile A& a;
> +
> +int main() {
> +  a;
> +}
> diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> index a806ef0706e..2b82c64ff73 100644
> --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r, volatile int &vr)
>     (void)(i ? r : j);	        // ok, no warning
>     (void)((void)1, r);	        // ok, no warning
>   
> -  vr;				// { dg-warning "" } reference not accessed
> -  (void)vr;			// { dg-warning "" } reference not accessed
> -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not accessed
> -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not accessed
> -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
> +  vr;				// ok, no warning
> +  (void)vr;			// ok, no warning
> +  (void)(i ? vj : vr);	        // ok, no warning
> +  (void)(i ? vr : vj);	        // ok, no warning
> +  (void)((void)1, vr);          // ok, no warning
>     
>     *ip_fn ();			// ok, no warning
>     *vip_fn ();			// ok, no warning
>     ir_fn ();			// ok, no warning
> -  vir_fn ();			// { dg-warning "" } reference not accessed
> +  vir_fn ();			// ok, no warning

This should still warn (and not load), a function call is not one of the 
forms in https://eel.is/c++draft/expr#context-2.  We still need the 
warnings you're removing for expression forms that aren't on that list.

>   }
>   
>   struct S;
> @@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r, volatile T &vr)
>     (void)(i ? r : j);	        // ok, no warning
>     (void)((void)1, r);	        // ok, no warning
>   
> -  vr;				// { dg-warning "" } reference not accessed
> -  (void)vr;			// { dg-warning "" } reference not accessed
> -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not accessed
> -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not accessed
> -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
> +  vr;				// ok, no warning
> +  (void)vr;			// ok, no warning
> +  (void)(i ? vj : vr);	        // ok, no warning
> +  (void)(i ? vr : vj);	        // ok, no warning
> +  (void)((void)1, vr);          // ok, no warning
>     
>     *tp_fn ();			// ok, no warning
>     *vtp_fn ();			// ok, no warning
>     tr_fn ();			// ok, no warning
> -  vtr_fn ();			// ok, no warning{ dg-warning "" } reference not accessed
> +  vtr_fn ();			// ok, no warning

Likewise.

>   }
>   
>   void extern_test ()
> @@ -160,5 +160,5 @@ void extern_test ()
>     esr;				// ok, no warning
>     vesr;				// { dg-warning "" } incomplete not accessed
>     etr;				// ok, no warning
> -  vetr;				// { dg-warning "" } reference not accessed
> +  vetr;				// ok, no warning
>   }

Re: [PATCH v2 1/2] c++: overeager type completion in convert_to_void [PR111419]

[Jason Merrill]

On 9/18/23 12:19, Patrick Palka wrote:
> On Mon, 18 Sep 2023, Patrick Palka wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>> trunk?
>>
>> -- >8 --
>>
>> Here convert_to_void always completes the type of an indirection or
>> id-expression, but according to [expr.context] an lvalue-to-rvalue
>> conversion is applied to a discarded-value expression only if "the
>> expression is a glvalue of volatile-qualified type".  This patch
>> restricts convert_to_void's type completion to match.
>>
>> 	PR c++/111419
>>
>> gcc/cp/ChangeLog:
>>
>> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
>> 	complete_type if the type is volatile.
>> 	<case VAR_DECL>: Only call complete_type if the type is volatile.
>> 	Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/cpp2a/concepts-requires36.C: New test.
>> 	* g++.dg/expr/discarded1.C: New test.
>> 	* g++.dg/expr/discarded1a.C: New test.
>> ---
>>   gcc/cp/cvt.cc                                   | 10 ++++++----
>>   .../g++.dg/cpp2a/concepts-requires36.C          | 16 ++++++++++++++++
>>   gcc/testsuite/g++.dg/expr/discarded1.C          | 15 +++++++++++++++
>>   gcc/testsuite/g++.dg/expr/discarded1a.C         | 17 +++++++++++++++++
>>   4 files changed, 54 insertions(+), 4 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>>   create mode 100644 gcc/testsuite/g++.dg/expr/discarded1.C
>>   create mode 100644 gcc/testsuite/g++.dg/expr/discarded1a.C
>>
>> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
>> index c6b52f07050..4424670356c 100644
>> --- a/gcc/cp/cvt.cc
>> +++ b/gcc/cp/cvt.cc
>> @@ -1250,9 +1250,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>>       case INDIRECT_REF:
>>         {
>>   	tree type = TREE_TYPE (expr);
>> -	int is_reference = TYPE_REF_P (TREE_TYPE (TREE_OPERAND (expr, 0)));
> 
> Oops, this removal of is_reference should happen in the follow-up patch
> instead, consider that fixed.

OK with that fixed.

>>   	int is_volatile = TYPE_VOLATILE (type);
>> -	int is_complete = COMPLETE_TYPE_P (complete_type (type));
>> +	if (is_volatile)
>> +	  complete_type (type);
>> +	int is_complete = COMPLETE_TYPE_P (type);
>>   
>>   	/* Can't load the value if we don't know the type.  */
>>   	if (is_volatile && !is_complete)
>> @@ -1412,9 +1413,10 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>>         {
>>   	/* External variables might be incomplete.  */
>>   	tree type = TREE_TYPE (expr);
>> -	int is_complete = COMPLETE_TYPE_P (complete_type (type));
>>   
>> -	if (TYPE_VOLATILE (type) && !is_complete && (complain & tf_warning))
>> +	if (TYPE_VOLATILE (type)
>> +	    && !COMPLETE_TYPE_P (complete_type (type))
>> +	    && (complain & tf_warning))
>>   	  switch (implicit)
>>   	    {
>>   	      case ICV_CAST:
>> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>> new file mode 100644
>> index 00000000000..8d3a4fcd2aa
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C
>> @@ -0,0 +1,16 @@
>> +// PR c++/111419
>> +// { dg-do compile { target c++20 } }
>> +
>> +template<class F>
>> +concept invocable = requires(F& f) { f(); };
>> +
>> +template<class F>
>> +concept deref_invocable = requires(F& f) { *f(); };
>> +
>> +struct Incomplete;
>> +
>> +template<class T>
>> +struct Holder { T t; };
>> +
>> +static_assert(invocable<Holder<Incomplete>& ()>);
>> +static_assert(deref_invocable<Holder<Incomplete>* ()>);
>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1.C b/gcc/testsuite/g++.dg/expr/discarded1.C
>> new file mode 100644
>> index 00000000000..c0c22e9e95b
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/expr/discarded1.C
>> @@ -0,0 +1,15 @@
>> +// PR c++/111419
>> +
>> +struct Incomplete;
>> +
>> +template<class T> struct Holder { T t; }; // { dg-bogus "incomplete" }
>> +
>> +extern Holder<Incomplete> a;
>> +extern Holder<Incomplete>& b;
>> +extern Holder<Incomplete>* c;
>> +
>> +int main() {
>> +  a;
>> +  b;
>> +  *c;
>> +}
>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
>> new file mode 100644
>> index 00000000000..1c4dff4553e
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
>> @@ -0,0 +1,17 @@
>> +// A version of discarded1.C using volatile types.
>> +// PR c++/111419
>> +
>> +struct Incomplete;
>> +
>> +template<class T, int> struct Holder { T t; }; // { dg-error "incomplete" }
>> +
>> +extern volatile Holder<Incomplete, 0> a;
>> +extern volatile Holder<Incomplete, 1>& b;
>> +extern volatile Holder<Incomplete, 2>* c;
>> +
>> +int main() {
>> +  a; // { dg-message "required from here" }
>> +  b; // { dg-message "required from here" }
>> +  // { dg-warning "implicit dereference will not access object" "" { target *-*-* } .-1 }
>> +  *c; // { dg-message "required from here" }
>> +}
>> -- 
>> 2.42.0.216.gbda494f404
>>
>>
> 

Re: [PATCH v2 2/2] c++: convert_to_void and volatile references

[Patrick Palka]

On Mon, 18 Sep 2023, Jason Merrill wrote:

> On 9/18/23 12:12, Patrick Palka wrote:
> > Jason pointed out that even implicit loads of volatile references need
> > to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> > in this case and discard the load.  This patch changes this behavior so
> > that we don't issue a warning, and preserve the load.
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
> > 	for an implicit load of a volatile reference.  Simplify as if
> > 	is_reference is false.  Check REFERENCE_REF_P in the test
> > 	guarding the -Wunused-value diagnostic.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/expr/discarded1a.C: No longer expect warning for
> > 	implicit load of a volatile reference.
> > 	* g++.old-deja/g++.bugs/900428_01.C: Likewise.
> > 	* g++.dg/expr/volatile2.C: New test.
> > ---
> >   gcc/cp/cvt.cc                                 | 56 ++-----------------
> >   gcc/testsuite/g++.dg/expr/discarded1a.C       |  1 -
> >   gcc/testsuite/g++.dg/expr/volatile2.C         | 12 ++++
> >   .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++-----
> >   4 files changed, 30 insertions(+), 65 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
> > 
> > diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> > index 4424670356c..1cb6c1222c2 100644
> > --- a/gcc/cp/cvt.cc
> > +++ b/gcc/cp/cvt.cc
> > @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> >         {
> >   	tree type = TREE_TYPE (expr);
> >   	int is_volatile = TYPE_VOLATILE (type);
> > -	if (is_volatile)
> > -	  complete_type (type);
> > -	int is_complete = COMPLETE_TYPE_P (type);
> >     	/* Can't load the value if we don't know the type.  */
> > -	if (is_volatile && !is_complete)
> > +	if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
> >             {
> >               if (complain & tf_warning)
> >   	      switch (implicit)
> > @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> >   		    gcc_unreachable ();
> >   		}
> >             }
> > -	/* Don't load the value if this is an implicit dereference, or if
> > -	   the type needs to be handled by ctors/dtors.  */
> > -	else if (is_volatile && is_reference)
> > -          {
> > -            if (complain & tf_warning)
> > -	      switch (implicit)
> > -		{
> > -	      	  case ICV_CAST:
> > -		    warning_at (loc, 0, "conversion to void will not access "
> > -				"object of type %qT", type);
> > -		    break;
> > -		  case ICV_SECOND_OF_COND:
> > -		    warning_at (loc, 0, "implicit dereference will not access
> > "
> > -				"object of type %qT in second operand of "
> > -				"conditional expression", type);
> > -		    break;
> > -		  case ICV_THIRD_OF_COND:
> > -		    warning_at (loc, 0, "implicit dereference will not access
> > "
> > -				"object of type %qT in third operand of "
> > -				"conditional expression", type);
> > -		    break;
> > -		  case ICV_RIGHT_OF_COMMA:
> > -		    warning_at (loc, 0, "implicit dereference will not access
> > "
> > -				"object of type %qT in right operand of "
> > -				"comma operator", type);
> > -		    break;
> > -		  case ICV_LEFT_OF_COMMA:
> > -		    warning_at (loc, 0, "implicit dereference will not access
> > "
> > -				"object of type %qT in left operand of comma "
> > -				"operator", type);
> > -		    break;
> > -		  case ICV_STATEMENT:
> > -		    warning_at (loc, 0, "implicit dereference will not access
> > "
> > -				"object of type %qT in statement",  type);
> > -		     break;
> > -		  case ICV_THIRD_IN_FOR:
> > -		    warning_at (loc, 0, "implicit dereference will not access
> > "
> > -				"object of type %qT in for increment
> > expression",
> > -				type);
> > -		    break;
> > -		  default:
> > -		    gcc_unreachable ();
> > -		}
> > -          }
> > +	/* Don't load the value if the type needs to be handled by cdtors.  */
> >   	else if (is_volatile && TREE_ADDRESSABLE (type))
> >   	  {
> >   	    if (complain & tf_warning)
> > @@ -1386,7 +1340,7 @@ 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_volatile || !COMPLETE_TYPE_P (type))
> >             {
> >               /* Emit a warning (if enabled) when the "effect-less"
> > INDIRECT_REF
> >                  operation is stripped off. Note that we don't warn about
> > @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit,
> > tsubst_flags_t complain)
> >               if (warn_unused_value
> >   		&& implicit != ICV_CAST
> >                   && (complain & tf_warning)
> > -                && !warning_suppressed_p (expr, OPT_Wunused_value)
> > -                && !is_reference)
> > +		&& !warning_suppressed_p (expr, OPT_Wunused_value)
> > +		&& !REFERENCE_REF_P (expr))
> >                 warning_at (loc, OPT_Wunused_value, "value computed is not
> > used");
> >               expr = TREE_OPERAND (expr, 0);
> >   	    if (TREE_CODE (expr) == CALL_EXPR
> > diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > index 1c4dff4553e..4754f32f716 100644
> > --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
> >   int main() {
> >     a; // { dg-message "required from here" }
> >     b; // { dg-message "required from here" }
> > -  // { dg-warning "implicit dereference will not access object" "" { target
> > *-*-* } .-1 }
> >     *c; // { dg-message "required from here" }
> >   }
> > diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C
> > b/gcc/testsuite/g++.dg/expr/volatile2.C
> > new file mode 100644
> > index 00000000000..62e609aee2f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> > @@ -0,0 +1,12 @@
> > +// Verify we preserve implicit loads of volatile references in a
> > +// discarded-value expression.
> > +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized"
> > }
> > +// { dg-final { scan-tree-dump {\*a} "original" } }
> > +// { dg-final { scan-tree-dump {\*a} "optimized" } }
> > +
> > +struct A { int m; };
> > +extern volatile A& a;
> > +
> > +int main() {
> > +  a;
> > +}
> > diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > index a806ef0706e..2b82c64ff73 100644
> > --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
> > @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r,
> > volatile int &vr)
> >     (void)(i ? r : j);	        // ok, no warning
> >     (void)((void)1, r);	        // ok, no warning
> >   -  vr;				// { dg-warning "" } reference not
> > accessed
> > -  (void)vr;			// { dg-warning "" } reference not accessed
> > -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not
> > accessed
> > -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not
> > accessed
> > -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
> > +  vr;				// ok, no warning
> > +  (void)vr;			// ok, no warning
> > +  (void)(i ? vj : vr);	        // ok, no warning
> > +  (void)(i ? vr : vj);	        // ok, no warning
> > +  (void)((void)1, vr);          // ok, no warning
> >         *ip_fn ();			// ok, no warning
> >     *vip_fn ();			// ok, no warning
> >     ir_fn ();			// ok, no warning
> > -  vir_fn ();			// { dg-warning "" } reference not accessed
> > +  vir_fn ();			// ok, no warning
> 
> This should still warn (and not load), a function call is not one of the forms
> in https://eel.is/c++draft/expr#context-2.  We still need the warnings you're
> removing for expression forms that aren't on that list.

D'oh, will fix.  It's surprising at first glance that *vip_fn() behaves
differently from vir_fn().

> 
> >   }
> >     struct S;
> > @@ -128,16 +128,16 @@ void complete_test (int i, T *p, volatile T *vp, T &r,
> > volatile T &vr)
> >     (void)(i ? r : j);	        // ok, no warning
> >     (void)((void)1, r);	        // ok, no warning
> >   -  vr;				// { dg-warning "" } reference not
> > accessed
> > -  (void)vr;			// { dg-warning "" } reference not accessed
> > -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not
> > accessed
> > -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not
> > accessed
> > -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
> > +  vr;				// ok, no warning
> > +  (void)vr;			// ok, no warning
> > +  (void)(i ? vj : vr);	        // ok, no warning
> > +  (void)(i ? vr : vj);	        // ok, no warning
> > +  (void)((void)1, vr);          // ok, no warning
> >         *tp_fn ();			// ok, no warning
> >     *vtp_fn ();			// ok, no warning
> >     tr_fn ();			// ok, no warning
> > -  vtr_fn ();			// ok, no warning{ dg-warning "" } reference
> > not accessed
> > +  vtr_fn ();			// ok, no warning
> 
> Likewise.
> 
> >   }
> >     void extern_test ()
> > @@ -160,5 +160,5 @@ void extern_test ()
> >     esr;				// ok, no warning
> >     vesr;				// { dg-warning "" } incomplete not
> > accessed
> >     etr;				// ok, no warning
> > -  vetr;				// { dg-warning "" } reference not
> > accessed
> > +  vetr;				// ok, no warning
> >   }
> 
> 

Re: [PATCH v2 2/2] c++: convert_to_void and volatile references

[Jason Merrill]

On 9/19/23 13:53, Patrick Palka wrote:
> On Mon, 18 Sep 2023, Jason Merrill wrote:
> 
>> On 9/18/23 12:12, Patrick Palka wrote:
>>> Jason pointed out that even implicit loads of volatile references need
>>> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
>>> in this case and discard the load.  This patch changes this behavior so
>>> that we don't issue a warning, and preserve the load.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Remove warning
>>> 	for an implicit load of a volatile reference.  Simplify as if
>>> 	is_reference is false.  Check REFERENCE_REF_P in the test
>>> 	guarding the -Wunused-value diagnostic.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/expr/discarded1a.C: No longer expect warning for
>>> 	implicit load of a volatile reference.
>>> 	* g++.old-deja/g++.bugs/900428_01.C: Likewise.
>>> 	* g++.dg/expr/volatile2.C: New test.
>>> ---
>>>    gcc/cp/cvt.cc                                 | 56 ++-----------------
>>>    gcc/testsuite/g++.dg/expr/discarded1a.C       |  1 -
>>>    gcc/testsuite/g++.dg/expr/volatile2.C         | 12 ++++
>>>    .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++-----
>>>    4 files changed, 30 insertions(+), 65 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
>>>
>>> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
>>> index 4424670356c..1cb6c1222c2 100644
>>> --- a/gcc/cp/cvt.cc
>>> +++ b/gcc/cp/cvt.cc
>>> @@ -1251,12 +1251,9 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>>          {
>>>    	tree type = TREE_TYPE (expr);
>>>    	int is_volatile = TYPE_VOLATILE (type);
>>> -	if (is_volatile)
>>> -	  complete_type (type);
>>> -	int is_complete = COMPLETE_TYPE_P (type);
>>>      	/* Can't load the value if we don't know the type.  */
>>> -	if (is_volatile && !is_complete)
>>> +	if (is_volatile && !COMPLETE_TYPE_P (complete_type (type)))
>>>              {
>>>                if (complain & tf_warning)
>>>    	      switch (implicit)
>>> @@ -1298,50 +1295,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>>    		    gcc_unreachable ();
>>>    		}
>>>              }
>>> -	/* Don't load the value if this is an implicit dereference, or if
>>> -	   the type needs to be handled by ctors/dtors.  */
>>> -	else if (is_volatile && is_reference)
>>> -          {
>>> -            if (complain & tf_warning)
>>> -	      switch (implicit)
>>> -		{
>>> -	      	  case ICV_CAST:
>>> -		    warning_at (loc, 0, "conversion to void will not access "
>>> -				"object of type %qT", type);
>>> -		    break;
>>> -		  case ICV_SECOND_OF_COND:
>>> -		    warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> -				"object of type %qT in second operand of "
>>> -				"conditional expression", type);
>>> -		    break;
>>> -		  case ICV_THIRD_OF_COND:
>>> -		    warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> -				"object of type %qT in third operand of "
>>> -				"conditional expression", type);
>>> -		    break;
>>> -		  case ICV_RIGHT_OF_COMMA:
>>> -		    warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> -				"object of type %qT in right operand of "
>>> -				"comma operator", type);
>>> -		    break;
>>> -		  case ICV_LEFT_OF_COMMA:
>>> -		    warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> -				"object of type %qT in left operand of comma "
>>> -				"operator", type);
>>> -		    break;
>>> -		  case ICV_STATEMENT:
>>> -		    warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> -				"object of type %qT in statement",  type);
>>> -		     break;
>>> -		  case ICV_THIRD_IN_FOR:
>>> -		    warning_at (loc, 0, "implicit dereference will not access
>>> "
>>> -				"object of type %qT in for increment
>>> expression",
>>> -				type);
>>> -		    break;
>>> -		  default:
>>> -		    gcc_unreachable ();
>>> -		}
>>> -          }
>>> +	/* Don't load the value if the type needs to be handled by cdtors.  */
>>>    	else if (is_volatile && TREE_ADDRESSABLE (type))
>>>    	  {
>>>    	    if (complain & tf_warning)
>>> @@ -1386,7 +1340,7 @@ 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_volatile || !COMPLETE_TYPE_P (type))
>>>              {
>>>                /* Emit a warning (if enabled) when the "effect-less"
>>> INDIRECT_REF
>>>                   operation is stripped off. Note that we don't warn about
>>> @@ -1397,8 +1351,8 @@ convert_to_void (tree expr, impl_conv_void implicit,
>>> tsubst_flags_t complain)
>>>                if (warn_unused_value
>>>    		&& implicit != ICV_CAST
>>>                    && (complain & tf_warning)
>>> -                && !warning_suppressed_p (expr, OPT_Wunused_value)
>>> -                && !is_reference)
>>> +		&& !warning_suppressed_p (expr, OPT_Wunused_value)
>>> +		&& !REFERENCE_REF_P (expr))
>>>                  warning_at (loc, OPT_Wunused_value, "value computed is not
>>> used");
>>>                expr = TREE_OPERAND (expr, 0);
>>>    	    if (TREE_CODE (expr) == CALL_EXPR
>>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> b/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> index 1c4dff4553e..4754f32f716 100644
>>> --- a/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> @@ -12,6 +12,5 @@ extern volatile Holder<Incomplete, 2>* c;
>>>    int main() {
>>>      a; // { dg-message "required from here" }
>>>      b; // { dg-message "required from here" }
>>> -  // { dg-warning "implicit dereference will not access object" "" { target
>>> *-*-* } .-1 }
>>>      *c; // { dg-message "required from here" }
>>>    }
>>> diff --git a/gcc/testsuite/g++.dg/expr/volatile2.C
>>> b/gcc/testsuite/g++.dg/expr/volatile2.C
>>> new file mode 100644
>>> index 00000000000..62e609aee2f
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
>>> @@ -0,0 +1,12 @@
>>> +// Verify we preserve implicit loads of volatile references in a
>>> +// discarded-value expression.
>>> +// { dg-additional-options "-O -fdump-tree-original -fdump-tree-optimized"
>>> }
>>> +// { dg-final { scan-tree-dump {\*a} "original" } }
>>> +// { dg-final { scan-tree-dump {\*a} "optimized" } }
>>> +
>>> +struct A { int m; };
>>> +extern volatile A& a;
>>> +
>>> +int main() {
>>> +  a;
>>> +}
>>> diff --git a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> index a806ef0706e..2b82c64ff73 100644
>>> --- a/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> +++ b/gcc/testsuite/g++.old-deja/g++.bugs/900428_01.C
>>> @@ -46,16 +46,16 @@ void int_test (int i, int *p, volatile int *vp, int &r,
>>> volatile int &vr)
>>>      (void)(i ? r : j);	        // ok, no warning
>>>      (void)((void)1, r);	        // ok, no warning
>>>    -  vr;				// { dg-warning "" } reference not
>>> accessed
>>> -  (void)vr;			// { dg-warning "" } reference not accessed
>>> -  (void)(i ? vj : vr);	        // { dg-warning "" } reference not
>>> accessed
>>> -  (void)(i ? vr : vj);	        // { dg-warning "" } reference not
>>> accessed
>>> -  (void)((void)1, vr);          // { dg-warning "" } reference not accessed
>>> +  vr;				// ok, no warning
>>> +  (void)vr;			// ok, no warning
>>> +  (void)(i ? vj : vr);	        // ok, no warning
>>> +  (void)(i ? vr : vj);	        // ok, no warning
>>> +  (void)((void)1, vr);          // ok, no warning
>>>          *ip_fn ();			// ok, no warning
>>>      *vip_fn ();			// ok, no warning
>>>      ir_fn ();			// ok, no warning
>>> -  vir_fn ();			// { dg-warning "" } reference not accessed
>>> +  vir_fn ();			// ok, no warning
>>
>> This should still warn (and not load), a function call is not one of the forms
>> in https://eel.is/c++draft/expr#context-2.  We still need the warnings you're
>> removing for expression forms that aren't on that list.
> 
> D'oh, will fix.  It's surprising at first glance that *vip_fn() behaves
> differently from vir_fn().

Yeah, the rationale was that in *vip_fn the last explicit semantic is 
the dereference, while in vir_fn() it's the call.  It is pretty subtle, 
though.

Jason