| Message ID | 20230918161202.2884010-2-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] c++: overeager type completion in convert_to_void [PR111419] | expand |
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
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 } 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 }