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