| Message ID | orwn4gsrkk.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | Accept pmf-vbit-in-delta extra warning | expand |
On 2/17/23 23:02, Alexandre Oliva wrote: > > cp_build_binary_op, that issues -Waddress warnings, issues an extra > warning on arm targets, that g++.dg/warn/Waddress-5.C does not expect > when comparing a pointer-to-member-function literal with null. > > The reason for the extra warning is that, on arm targets, > TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta, which > causes a different path to be taken, that extracts the > pointer-to-function and the delta fields (minus the vbit) and compares > each one with zero. It's when comparing this pointer-to-function with > zero, in a recursive cp_build_binary_op, that another warning is > issued. > > I suppose there should be a way to skip the warning in this recursive > call, without disabling other warnings that might be issued there, but warning_sentinel ws (warn_address) ? > this patch only arranges for the test to tolerate the extra warning. > > Regstrapped on x86_64-linux-gnu. > Tested on arm-vxworks7 (gcc-12) and arm-eabi (trunk). Ok to install? OK > for gcc/testsuite/ChangeLog > > * g++.dg/warn/Waddress-5.C: Tolerate extra -Waddress warning. > --- > gcc/testsuite/g++.dg/warn/Waddress-5.C | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/gcc/testsuite/g++.dg/warn/Waddress-5.C b/gcc/testsuite/g++.dg/warn/Waddress-5.C > index b1287b2fac316..1de88076f7767 100644 > --- a/gcc/testsuite/g++.dg/warn/Waddress-5.C > +++ b/gcc/testsuite/g++.dg/warn/Waddress-5.C > @@ -23,7 +23,11 @@ void T (bool); > void warn_memptr_if () > { > // Exercise warnings for addresses of nonstatic member functions. > - if (&A::f == 0) // { dg-warning "the address '&A::f'" } > + // On targets with TARGET_PTRMEMFUNC_VBIT_LOCATION == > + // ptrmemfunc_vbit_in_delta, cp_build_binary_op recurses to compare > + // the pfn from the ptrmemfunc with null, so we get two warnings. > + // This matches both. ??? Should we disable one of them? > + if (&A::f == 0) // { dg-warning "A::f" } > T (0); > > if (&A::vf) // { dg-warning "-Waddress" } >
On Feb 17, 2023, Jason Merrill <jason@redhat.com> wrote: > On 2/17/23 23:02, Alexandre Oliva wrote: >> >> cp_build_binary_op, that issues -Waddress warnings, issues an extra >> warning on arm targets, that g++.dg/warn/Waddress-5.C does not expect >> when comparing a pointer-to-member-function literal with null. >> >> The reason for the extra warning is that, on arm targets, >> TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta, which >> causes a different path to be taken, that extracts the >> pointer-to-function and the delta fields (minus the vbit) and compares >> each one with zero. It's when comparing this pointer-to-function with >> zero, in a recursive cp_build_binary_op, that another warning is >> issued. >> >> I suppose there should be a way to skip the warning in this recursive >> call, without disabling other warnings that might be issued there, but > warning_sentinel ws (warn_address) Oh, yeah, that will suppress the expected warning for pfn0, but isn't there any risk whatsoever that it could suppress other -Waddress warnings for tree operands of pfn0? I see the cp_save_expr for side effects, but what if e.g. the pmfn we're testing is an array element, and the index expression tests another pmfn against NULL that should be warned about? Or something else that wouldn't have TREE_SIDE_EFFECTS, and would thus not go through cp_save_expr. Would we then warn for uses of both pfn0 and delta0? Here's what I'm going to test for these concerns. Ok to install if it bootstraps successfully, and my concerns are unfounded? [c++] suppress redundant null-addr warn in pfn from pmfn From: Alexandre Oliva <oliva@adacore.com> When TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta, when we warn about comparing a pointer-to-member-function with NULL, we also warn about comparing the pointer-to-function extracted from it with NULL, which is redundant. Suppress the redundant warning. for gcc/cp/ChangeLog * typeck.cc (cp_build_binary_op): Suppress redundant warning for pfn null test in pmfn test with vbit-in-delta. --- gcc/cp/typeck.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 4afb5e4f0d420..d5a3e501d8e91 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -5780,11 +5780,18 @@ cp_build_binary_op (const op_location_t &location, pfn0 = pfn_from_ptrmemfunc (op0); delta0 = delta_from_ptrmemfunc (op0); - e1 = cp_build_binary_op (location, - EQ_EXPR, - pfn0, - build_zero_cst (TREE_TYPE (pfn0)), - complain); + { + /* If we will warn below about a null-address compare + involving the orig_op0 ptrmemfunc, we'd likely also + warn about the pfn0's null-address compare, and + that would be redundant, so suppress it. */ + warning_sentinel ws (warn_address); + e1 = cp_build_binary_op (location, + EQ_EXPR, + pfn0, + build_zero_cst (TREE_TYPE (pfn0)), + complain); + } e2 = cp_build_binary_op (location, BIT_AND_EXPR, delta0,
On 2/22/23 11:03, Alexandre Oliva wrote: > On Feb 17, 2023, Jason Merrill <jason@redhat.com> wrote: > >> On 2/17/23 23:02, Alexandre Oliva wrote: >>> >>> cp_build_binary_op, that issues -Waddress warnings, issues an extra >>> warning on arm targets, that g++.dg/warn/Waddress-5.C does not expect >>> when comparing a pointer-to-member-function literal with null. >>> >>> The reason for the extra warning is that, on arm targets, >>> TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta, which >>> causes a different path to be taken, that extracts the >>> pointer-to-function and the delta fields (minus the vbit) and compares >>> each one with zero. It's when comparing this pointer-to-function with >>> zero, in a recursive cp_build_binary_op, that another warning is >>> issued. >>> >>> I suppose there should be a way to skip the warning in this recursive >>> call, without disabling other warnings that might be issued there, but > >> warning_sentinel ws (warn_address) > > Oh, yeah, that will suppress the expected warning for pfn0, but isn't > there any risk whatsoever that it could suppress other -Waddress > warnings for tree operands of pfn0? There shouldn't be an issue; those operands would have been considered for warning when building their subexpressions. > I see the cp_save_expr for side effects, but what if e.g. the pmfn we're > testing is an array element, and the index expression tests another pmfn > against NULL that should be warned about? Or something else that > wouldn't have TREE_SIDE_EFFECTS, and would thus not go through > cp_save_expr. Would we then warn for uses of both pfn0 and delta0? > > > Here's what I'm going to test for these concerns. Ok to install if it > bootstraps successfully, and my concerns are unfounded? OK. > [c++] suppress redundant null-addr warn in pfn from pmfn > > From: Alexandre Oliva <oliva@adacore.com> > > When TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta, when > we warn about comparing a pointer-to-member-function with NULL, we > also warn about comparing the pointer-to-function extracted from it > with NULL, which is redundant. Suppress the redundant warning. > > > for gcc/cp/ChangeLog > > * typeck.cc (cp_build_binary_op): Suppress redundant warning > for pfn null test in pmfn test with vbit-in-delta. > --- > gcc/cp/typeck.cc | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 4afb5e4f0d420..d5a3e501d8e91 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -5780,11 +5780,18 @@ cp_build_binary_op (const op_location_t &location, > > pfn0 = pfn_from_ptrmemfunc (op0); > delta0 = delta_from_ptrmemfunc (op0); > - e1 = cp_build_binary_op (location, > - EQ_EXPR, > - pfn0, > - build_zero_cst (TREE_TYPE (pfn0)), > - complain); > + { > + /* If we will warn below about a null-address compare > + involving the orig_op0 ptrmemfunc, we'd likely also > + warn about the pfn0's null-address compare, and > + that would be redundant, so suppress it. */ > + warning_sentinel ws (warn_address); > + e1 = cp_build_binary_op (location, > + EQ_EXPR, > + pfn0, > + build_zero_cst (TREE_TYPE (pfn0)), > + complain); > + } > e2 = cp_build_binary_op (location, > BIT_AND_EXPR, > delta0, > >
diff --git a/gcc/testsuite/g++.dg/warn/Waddress-5.C b/gcc/testsuite/g++.dg/warn/Waddress-5.C index b1287b2fac316..1de88076f7767 100644 --- a/gcc/testsuite/g++.dg/warn/Waddress-5.C +++ b/gcc/testsuite/g++.dg/warn/Waddress-5.C @@ -23,7 +23,11 @@ void T (bool); void warn_memptr_if () { // Exercise warnings for addresses of nonstatic member functions. - if (&A::f == 0) // { dg-warning "the address '&A::f'" } + // On targets with TARGET_PTRMEMFUNC_VBIT_LOCATION == + // ptrmemfunc_vbit_in_delta, cp_build_binary_op recurses to compare + // the pfn from the ptrmemfunc with null, so we get two warnings. + // This matches both. ??? Should we disable one of them? + if (&A::f == 0) // { dg-warning "A::f" } T (0); if (&A::vf) // { dg-warning "-Waddress" }