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
}
--
2.42.0.216.gbda494f404
