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

  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
  

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
> 
>
  

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
>   }
  

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
> >   }
> 
>
  

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