diff mbox series

c++: overeager type completion in convert_to_void [PR111419]

Message ID 20230915160301.2245349-1-ppalka@redhat.com
State New
Headers show
Series c++: overeager type completion in convert_to_void [PR111419] | expand

Commit Message

Patrick Palka Sept. 15, 2023, 4:03 p.m. UTC
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 INDIRECT_REF or
VAR_DECL 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 accordingly.

	PR c++/111419

gcc/cp/ChangeLog:

	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
	complete_type if the type is volatile and the INDIRECT_REF
	isn't an implicit one.
	<case VAR_DECL>: Only call complete_type if the type is
	volatile.

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                                    | 11 +++++++----
 gcc/testsuite/g++.dg/cpp2a/concepts-requires36.C | 16 ++++++++++++++++
 gcc/testsuite/g++.dg/expr/discarded1.C           | 15 +++++++++++++++
 gcc/testsuite/g++.dg/expr/discarded1a.C          | 16 ++++++++++++++++
 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

Comments

Jason Merrill Sept. 16, 2023, 8:37 p.m. UTC | #1
On 9/15/23 12:03, 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 INDIRECT_REF or
> VAR_DECL 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 accordingly.
> 
> 	PR c++/111419
> 
> gcc/cp/ChangeLog:
> 
> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> 	complete_type if the type is volatile and the INDIRECT_REF
> 	isn't an implicit one.

Hmm, what does implicit have to do with it?  The expression forms listed 
in https://eel.is/c++draft/expr.context#2 include "id-expression"...

> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C b/gcc/testsuite/g++.dg/expr/discarded1a.C
> new file mode 100644
> index 00000000000..5516ff46fe9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -0,0 +1,16 @@
> +// 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-warning "implicit dereference will not access object" }
> +     // { dg-bogus "required from here" "" { target *-*-* } .-1 }

...so it seems to me this line should get the lvalue-rvalue conversion 
(and not the warning about no access).

> +  *c; // { dg-message "required from here" }
> +}
Patrick Palka Sept. 16, 2023, 9:41 p.m. UTC | #2
On Sat, 16 Sep 2023, Jason Merrill wrote:

> On 9/15/23 12:03, 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 INDIRECT_REF or
> > VAR_DECL 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 accordingly.
> > 
> > 	PR c++/111419
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> > 	complete_type if the type is volatile and the INDIRECT_REF
> > 	isn't an implicit one.
> 
> Hmm, what does implicit have to do with it?  The expression forms listed in
> https://eel.is/c++draft/expr.context#2 include "id-expression"...

When there's an implicit INDIRECT_REF, I reckoned the type of the
id-expression is really a reference type, which can't be cv-qualified?

> 
> > diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > new file mode 100644
> > index 00000000000..5516ff46fe9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > @@ -0,0 +1,16 @@
> > +// 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-warning "implicit dereference will not access object" }
> > +     // { dg-bogus "required from here" "" { target *-*-* } .-1 }
> 
> ...so it seems to me this line should get the lvalue-rvalue conversion (and
> not the warning about no access).
> 
> > +  *c; // { dg-message "required from here" }
> > +}
> 
>
Jason Merrill Sept. 18, 2023, 2:40 a.m. UTC | #3
On 9/16/23 17:41, Patrick Palka wrote:
> On Sat, 16 Sep 2023, Jason Merrill wrote:
> 
>> On 9/15/23 12:03, 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 INDIRECT_REF or
>>> VAR_DECL 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 accordingly.
>>>
>>> 	PR c++/111419
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
>>> 	complete_type if the type is volatile and the INDIRECT_REF
>>> 	isn't an implicit one.
>>
>> Hmm, what does implicit have to do with it?  The expression forms listed in
>> https://eel.is/c++draft/expr.context#2 include "id-expression"...
> 
> When there's an implicit INDIRECT_REF, I reckoned the type of the
> id-expression is really a reference type, which can't be cv-qualified?

A name can have reference type, but its use as an expression doesn't:
https://eel.is/c++draft/expr.type#1.sentence-1

>>> diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> b/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> new file mode 100644
>>> index 00000000000..5516ff46fe9
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
>>> @@ -0,0 +1,16 @@
>>> +// 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-warning "implicit dereference will not access object" }
>>> +     // { dg-bogus "required from here" "" { target *-*-* } .-1 }
>>
>> ...so it seems to me this line should get the lvalue-rvalue conversion (and
>> not the warning about no access).
>>
>>> +  *c; // { dg-message "required from here" }
>>> +}
Patrick Palka Sept. 18, 2023, 3:41 p.m. UTC | #4
On Sun, 17 Sep 2023, Jason Merrill wrote:

> On 9/16/23 17:41, Patrick Palka wrote:
> > On Sat, 16 Sep 2023, Jason Merrill wrote:
> > 
> > > On 9/15/23 12:03, 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 INDIRECT_REF or
> > > > VAR_DECL 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 accordingly.
> > > > 
> > > > 	PR c++/111419
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> > > > 	complete_type if the type is volatile and the INDIRECT_REF
> > > > 	isn't an implicit one.
> > > 
> > > Hmm, what does implicit have to do with it?  The expression forms listed
> > > in
> > > https://eel.is/c++draft/expr.context#2 include "id-expression"...
> > 
> > When there's an implicit INDIRECT_REF, I reckoned the type of the
> > id-expression is really a reference type, which can't be cv-qualified?
> 
> A name can have reference type, but its use as an expression doesn't:
> https://eel.is/c++draft/expr.type#1.sentence-1

Ah, makes sense..

> 
> > > > diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > > > b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > > > new file mode 100644
> > > > index 00000000000..5516ff46fe9
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > > > @@ -0,0 +1,16 @@
> > > > +// 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-warning "implicit dereference will not access object" }
> > > > +     // { dg-bogus "required from here" "" { target *-*-* } .-1 }
> > > 
> > > ...so it seems to me this line should get the lvalue-rvalue conversion
> > > (and
> > > not the warning about no access).

Sounds good, like so?  I also added a test to verify such loads don't
get discarded in the generated code.

-- >8 --

Subject: [PATCH] c++: overeager type completion in convert_to_void [PR111419]

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

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

	PR c++/111419

gcc/cp/ChangeLog:

	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
	complete_type if the type is volatile.  Remove warning for
	implicit indirection of a volatile reference.  Simplify as if
	is_reference=false.  Check REFERENCE_REF_P in the -Wunused-value
	branch.
	<case VAR_DECL>: Only call complete_type if the type is volatile.

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.
	* g++.dg/expr/volatile2.C: New test.
	* g++.old-deja/g++.bugs/900428_01.C: No longer expect
	warnings for implicit loads of volatile references.
---
 gcc/cp/cvt.cc                                 | 60 +++----------------
 .../g++.dg/cpp2a/concepts-requires36.C        | 16 +++++
 gcc/testsuite/g++.dg/expr/discarded1.C        | 15 +++++
 gcc/testsuite/g++.dg/expr/discarded1a.C       | 16 +++++
 gcc/testsuite/g++.dg/expr/volatile2.C         | 11 ++++
 .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++----
 6 files changed, 79 insertions(+), 65 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
 create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index c6b52f07050..7e413f87334 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1250,12 +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));
 
 	/* 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)
@@ -1297,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)
@@ -1385,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) || TREE_ADDRESSABLE (type))
           {
             /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
                operation is stripped off. Note that we don't warn about
@@ -1396,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
@@ -1412,9 +1367,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..83cea806ad3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
@@ -0,0 +1,16 @@
+// A 'volatile' version of discarded1.C.
+// 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" }
+  *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..ab4325ff916
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/volatile2.C
@@ -0,0 +1,11 @@
+// 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" } }
+
+extern volatile struct A { int m; } &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
 }
Patrick Palka Sept. 18, 2023, 4:11 p.m. UTC | #5
On Mon, 18 Sep 2023, Patrick Palka wrote:

> On Sun, 17 Sep 2023, Jason Merrill wrote:
> 
> > On 9/16/23 17:41, Patrick Palka wrote:
> > > On Sat, 16 Sep 2023, Jason Merrill wrote:
> > > 
> > > > On 9/15/23 12:03, 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 INDIRECT_REF or
> > > > > VAR_DECL 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 accordingly.
> > > > > 
> > > > > 	PR c++/111419
> > > > > 
> > > > > gcc/cp/ChangeLog:
> > > > > 
> > > > > 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> > > > > 	complete_type if the type is volatile and the INDIRECT_REF
> > > > > 	isn't an implicit one.
> > > > 
> > > > Hmm, what does implicit have to do with it?  The expression forms listed
> > > > in
> > > > https://eel.is/c++draft/expr.context#2 include "id-expression"...
> > > 
> > > When there's an implicit INDIRECT_REF, I reckoned the type of the
> > > id-expression is really a reference type, which can't be cv-qualified?
> > 
> > A name can have reference type, but its use as an expression doesn't:
> > https://eel.is/c++draft/expr.type#1.sentence-1
> 
> Ah, makes sense..
> 
> > 
> > > > > diff --git a/gcc/testsuite/g++.dg/expr/discarded1a.C
> > > > > b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > > > > new file mode 100644
> > > > > index 00000000000..5516ff46fe9
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> > > > > @@ -0,0 +1,16 @@
> > > > > +// 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-warning "implicit dereference will not access object" }
> > > > > +     // { dg-bogus "required from here" "" { target *-*-* } .-1 }
> > > > 
> > > > ...so it seems to me this line should get the lvalue-rvalue conversion
> > > > (and
> > > > not the warning about no access).
> 
> Sounds good, like so?  I also added a test to verify such loads don't
> get discarded in the generated code.

On second thought, it seems better to split this into two patches, one
that correctly restricts type completion, and the next that makes us
correctly handle warning/code generation of volatile references.
Patch series incoming...

> 
> -- >8 --
> 
> Subject: [PATCH] c++: overeager type completion in convert_to_void [PR111419]
> 
> 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 accordingly.
> 
> Jason pointed out that implicit loads of volatile references also need
> to undergo lvalue-to-rvalue conversion, but we currently emit a warning
> in this case and discard the load.  This patch also changes this behavior
> so that we preserve the load and don't issue a warning.
> 
> 	PR c++/111419
> 
> gcc/cp/ChangeLog:
> 
> 	* cvt.cc (convert_to_void) <case INDIRECT_REF>: Only call
> 	complete_type if the type is volatile.  Remove warning for
> 	implicit indirection of a volatile reference.  Simplify as if
> 	is_reference=false.  Check REFERENCE_REF_P in the -Wunused-value
> 	branch.
> 	<case VAR_DECL>: Only call complete_type if the type is volatile.
> 
> 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.
> 	* g++.dg/expr/volatile2.C: New test.
> 	* g++.old-deja/g++.bugs/900428_01.C: No longer expect
> 	warnings for implicit loads of volatile references.
> ---
>  gcc/cp/cvt.cc                                 | 60 +++----------------
>  .../g++.dg/cpp2a/concepts-requires36.C        | 16 +++++
>  gcc/testsuite/g++.dg/expr/discarded1.C        | 15 +++++
>  gcc/testsuite/g++.dg/expr/discarded1a.C       | 16 +++++
>  gcc/testsuite/g++.dg/expr/volatile2.C         | 11 ++++
>  .../g++.old-deja/g++.bugs/900428_01.C         | 26 ++++----
>  6 files changed, 79 insertions(+), 65 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
>  create mode 100644 gcc/testsuite/g++.dg/expr/volatile2.C
> 
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index c6b52f07050..7e413f87334 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1250,12 +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));
>  
>  	/* 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)
> @@ -1297,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)
> @@ -1385,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) || TREE_ADDRESSABLE (type))
>            {
>              /* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
>                 operation is stripped off. Note that we don't warn about
> @@ -1396,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
> @@ -1412,9 +1367,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..83cea806ad3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
> @@ -0,0 +1,16 @@
> +// A 'volatile' version of discarded1.C.
> +// 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" }
> +  *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..ab4325ff916
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/volatile2.C
> @@ -0,0 +1,11 @@
> +// 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" } }
> +
> +extern volatile struct A { int m; } &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
> 
>
diff mbox series

Patch

diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
index c6b52f07050..5e2c01476e3 100644
--- a/gcc/cp/cvt.cc
+++ b/gcc/cp/cvt.cc
@@ -1252,10 +1252,12 @@  convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
 	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 && !is_reference)
+	  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 && !is_reference && !is_complete)
           {
             if (complain & tf_warning)
 	      switch (implicit)
@@ -1412,9 +1414,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..5516ff46fe9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/expr/discarded1a.C
@@ -0,0 +1,16 @@ 
+// 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-warning "implicit dereference will not access object" }
+     // { dg-bogus "required from here" "" { target *-*-* } .-1 }
+  *c; // { dg-message "required from here" }
+}