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