Re: PING #2 [PATCH] warn for more impossible null pointer tests [PR102103]
On 9/30/21 1:35 PM, Joseph Myers wrote:
On Thu, 30 Sep 2021, Martin Sebor via Gcc-patches wrote:
Jason, since you approved the C++ changes, would you mind looking
over the C bits and if they look good to you giving me the green
light to commit the patch?
https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579693.html
The C changes are OK, with two instances of "for the address %qE will
never be NULL" fixed to refer to the address *of* %qE as elsewhere (those
are for IMAGPART_EXPR and REALPART_EXPR; C++ also has one "the address %qE
will never be NULL"), and the "pr??" in the tests filled in with an
actual PR number for the XFAILed cases.
Thanks for the careful review and the approval!
I remember having a reason for dropping the "of" in the two instances
in the C FE but after double-checking the output I see you're right
that it should be there. Good catch!
I believe the C++ instance is correct. It's issued for the address
of a member, as in the former of the two below:
struct S { int i; };
bool f ()
{
return ::i == 0; // the address ‘::i’
}
bool g (S *p)
{
return >i == 0; // the address of ‘S::i’
}
z.c: In function ‘bool f()’:
z.c:5:16: warning: the address ‘::i’ will never be NULL [-Waddress]
5 | return ::i == 0;
| ~~^~~~
z.c: In function ‘bool g(S*)’:
z.c:10:16: warning: the address of ‘S::i’ will never be NULL [-Waddress]
10 | return >i == 0;
| ~~^~~~
z.c:1:16: note: ‘S::i’ declared here
1 | struct S { int i; };
|^
I've beefed up the tests to verify the expected wording.
Thanks also for prompting me to open bugs for the xfailed tests,
something I tend to forget to do when it depends on the patch I'm
developing. I've raised pr102555 for the C FE folding defeating
the warning. The C++ bug that tracks the xfails in the C++ tests
is pr102378.
I've pushed the updated patch in r12-4059 after retesting the whole
thing on x86_64-linux.
Martin
Re: PING #2 [PATCH] warn for more impossible null pointer tests [PR102103]
On Thu, 30 Sep 2021, Martin Sebor via Gcc-patches wrote: > Jason, since you approved the C++ changes, would you mind looking > over the C bits and if they look good to you giving me the green > light to commit the patch? > > https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579693.html The C changes are OK, with two instances of "for the address %qE will never be NULL" fixed to refer to the address *of* %qE as elsewhere (those are for IMAGPART_EXPR and REALPART_EXPR; C++ also has one "the address %qE will never be NULL"), and the "pr??" in the tests filled in with an actual PR number for the XFAILed cases. -- Joseph S. Myers jos...@codesourcery.com
PING #2 [PATCH] warn for more impossible null pointer tests [PR102103]
Jason, since you approved the C++ changes, would you mind looking over the C bits and if they look good to you giving me the green light to commit the patch? https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579693.html Thanks in advance for your help! On 9/24/21 8:31 AM, Martin Sebor wrote: Ping: Jeff, with the C++ part approved, can you please confirm your approval with the C parts of the patch? https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579693.html On 9/21/21 6:34 PM, Martin Sebor wrote: On 9/21/21 3:40 PM, Jason Merrill wrote: The C++ changes are OK. Jeff, should I take your previous "Generally OK" as an approval for the rest of the patch as well? (It has not changed in v2.) I have just submitted a Glibc patch to suppress the new instances there. Martin
PING [PATCH] warn for more impossible null pointer tests [PR102103]
Ping: Jeff, with the C++ part approved, can you please confirm your approval with the C parts of the patch? https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579693.html On 9/21/21 6:34 PM, Martin Sebor wrote: On 9/21/21 3:40 PM, Jason Merrill wrote: The C++ changes are OK. Jeff, should I take your previous "Generally OK" as an approval for the rest of the patch as well? (It has not changed in v2.) I have just submitted a Glibc patch to suppress the new instances there. Martin
Re: [PATCH] warn for more impossible null pointer tests [PR102103]
On 9/21/21 20:34, Martin Sebor wrote:
On 9/21/21 3:40 PM, Jason Merrill wrote:
On 9/17/21 12:02, Martin Sebor wrote:
On 9/8/21 2:06 PM, Jason Merrill wrote:
On 9/2/21 7:53 PM, Martin Sebor wrote:
@@ -4622,28 +4622,94 @@ warn_for_null_address (location_t location,
tree op, tsubst_flags_t complain)
if (!warn_address
|| (complain & tf_warning) == 0
|| c_inhibit_evaluation_warnings != 0
- || warning_suppressed_p (op, OPT_Waddress))
+ || warning_suppressed_p (op, OPT_Waddress)
+ || processing_template_decl != 0)
Completely suppressing this warning in templates seems like a
regression; I'd think we could recognize many relevant cases before
instantiation. You just can't assume that ADDR_EXPR has the default
meaning if it has unknown type (i.e. because op0 is type-dependent).
I added the suppression to keep g++.dg/warn/pr101219.C from failing
but in hindsight I should have questioned the reasoning behind
the "no warning emitted here (no instantiation)" comment in the test.
I agree that it would be helpful to diagnose the type-independent
subset of the problem even in uninstantiated templates. Current
trunk doesn't (it never has), but with my patch and the suppression
above removed it does. I've updated the tests to expect it.
Please see the attached revision.
Martin
PS There are still more opportunities to issue -Waddress in templates
that this patch doesn't handle, e.g.,:
template bool f (T *p) { return == 0; }
Handling this will take more surgery.
PPS It seems that most other warnings (and even some errors, like
-Wnarrowing) are suppressed in uninstantiated templates as well,
even for non-dependent expressions. In the few test cases I looked
at Clang does better. It sounds like you'd like to see improvements
in this direction not just for -Waddress but in general. Just for
the avoidance of doubt, can you confirm that for future reference?
Yes, in general it's better to diagnose sooner.
+ if (TREE_CODE (cop) == NON_LVALUE_EXPR)
+ /* Unwrap the expression for C++ 98. */
+ cop = TREE_OPERAND (cop, 0);
What does this have to do with C++98?
The code is needed to avoid failures in C++ 98 in the test below
where COP is a NON_LVALUE_EXPR which isn't handled below otherwise.
I didn't investigate why that happens (it works fine if f() is
an ordinary member function).
void f (bool);
void g ()
{
struct A { virtual void vf (); };
f (::vf); // missing -Waddress in C++ 98 mode
}
+ if (TREE_CODE (cop) == PTRMEM_CST)
+ {
+ /* The address of a nonstatic data member is never null. */
+ warning_at (location, OPT_Waddress,
+ "the address %qE will never be NULL",
Capitalizing NULL when talking about pointers-to-members seems a bit
odd, but I guess it's fine.
I agree. My personal preference is for lowercase null (in all
languages) since that's the technical term for it. I used NULL
here only to conform to the existing style. I'm willing to
change all these warnings to either use null or to some form
that doesn't mention null (there are two in use, although if
I had my druthers I'd choose some other phrasing altogether).
Let me know if you would support such a change.
I don't feel strongly about it. I agree that lowercase or another
phrasing would be better, but probably better to avoid adding work for
the translators with the churn.
The C++ changes are OK.
Jeff, should I take your previous "Generally OK" as an approval
for the rest of the patch as well? (It has not changed in v2.)
I have just submitted a Glibc patch to suppress the new instances
there.
Martin
Re: [PATCH] warn for more impossible null pointer tests [PR102103]
On 9/21/21 3:40 PM, Jason Merrill wrote:
On 9/17/21 12:02, Martin Sebor wrote:
On 9/8/21 2:06 PM, Jason Merrill wrote:
On 9/2/21 7:53 PM, Martin Sebor wrote:
@@ -4622,28 +4622,94 @@ warn_for_null_address (location_t location,
tree op, tsubst_flags_t complain)
if (!warn_address
|| (complain & tf_warning) == 0
|| c_inhibit_evaluation_warnings != 0
- || warning_suppressed_p (op, OPT_Waddress))
+ || warning_suppressed_p (op, OPT_Waddress)
+ || processing_template_decl != 0)
Completely suppressing this warning in templates seems like a
regression; I'd think we could recognize many relevant cases before
instantiation. You just can't assume that ADDR_EXPR has the default
meaning if it has unknown type (i.e. because op0 is type-dependent).
I added the suppression to keep g++.dg/warn/pr101219.C from failing
but in hindsight I should have questioned the reasoning behind
the "no warning emitted here (no instantiation)" comment in the test.
I agree that it would be helpful to diagnose the type-independent
subset of the problem even in uninstantiated templates. Current
trunk doesn't (it never has), but with my patch and the suppression
above removed it does. I've updated the tests to expect it.
Please see the attached revision.
Martin
PS There are still more opportunities to issue -Waddress in templates
that this patch doesn't handle, e.g.,:
template bool f (T *p) { return == 0; }
Handling this will take more surgery.
PPS It seems that most other warnings (and even some errors, like
-Wnarrowing) are suppressed in uninstantiated templates as well,
even for non-dependent expressions. In the few test cases I looked
at Clang does better. It sounds like you'd like to see improvements
in this direction not just for -Waddress but in general. Just for
the avoidance of doubt, can you confirm that for future reference?
Yes, in general it's better to diagnose sooner.
+ if (TREE_CODE (cop) == NON_LVALUE_EXPR)
+ /* Unwrap the expression for C++ 98. */
+ cop = TREE_OPERAND (cop, 0);
What does this have to do with C++98?
The code is needed to avoid failures in C++ 98 in the test below
where COP is a NON_LVALUE_EXPR which isn't handled below otherwise.
I didn't investigate why that happens (it works fine if f() is
an ordinary member function).
void f (bool);
void g ()
{
struct A { virtual void vf (); };
f (::vf); // missing -Waddress in C++ 98 mode
}
+ if (TREE_CODE (cop) == PTRMEM_CST)
+ {
+ /* The address of a nonstatic data member is never null. */
+ warning_at (location, OPT_Waddress,
+ "the address %qE will never be NULL",
Capitalizing NULL when talking about pointers-to-members seems a bit
odd, but I guess it's fine.
I agree. My personal preference is for lowercase null (in all
languages) since that's the technical term for it. I used NULL
here only to conform to the existing style. I'm willing to
change all these warnings to either use null or to some form
that doesn't mention null (there are two in use, although if
I had my druthers I'd choose some other phrasing altogether).
Let me know if you would support such a change.
The C++ changes are OK.
Jeff, should I take your previous "Generally OK" as an approval
for the rest of the patch as well? (It has not changed in v2.)
I have just submitted a Glibc patch to suppress the new instances
there.
Martin
Re: [PATCH] warn for more impossible null pointer tests [PR102103]
On 9/17/21 12:02, Martin Sebor wrote:
On 9/8/21 2:06 PM, Jason Merrill wrote:
On 9/2/21 7:53 PM, Martin Sebor wrote:
@@ -4622,28 +4622,94 @@ warn_for_null_address (location_t location,
tree op, tsubst_flags_t complain)
if (!warn_address
|| (complain & tf_warning) == 0
|| c_inhibit_evaluation_warnings != 0
- || warning_suppressed_p (op, OPT_Waddress))
+ || warning_suppressed_p (op, OPT_Waddress)
+ || processing_template_decl != 0)
Completely suppressing this warning in templates seems like a
regression; I'd think we could recognize many relevant cases before
instantiation. You just can't assume that ADDR_EXPR has the default
meaning if it has unknown type (i.e. because op0 is type-dependent).
I added the suppression to keep g++.dg/warn/pr101219.C from failing
but in hindsight I should have questioned the reasoning behind
the "no warning emitted here (no instantiation)" comment in the test.
I agree that it would be helpful to diagnose the type-independent
subset of the problem even in uninstantiated templates. Current
trunk doesn't (it never has), but with my patch and the suppression
above removed it does. I've updated the tests to expect it.
Please see the attached revision.
Martin
PS There are still more opportunities to issue -Waddress in templates
that this patch doesn't handle, e.g.,:
template bool f (T *p) { return == 0; }
Handling this will take more surgery.
PPS It seems that most other warnings (and even some errors, like
-Wnarrowing) are suppressed in uninstantiated templates as well,
even for non-dependent expressions. In the few test cases I looked
at Clang does better. It sounds like you'd like to see improvements
in this direction not just for -Waddress but in general. Just for
the avoidance of doubt, can you confirm that for future reference?
Yes, in general it's better to diagnose sooner.
+ if (TREE_CODE (cop) == NON_LVALUE_EXPR)
+/* Unwrap the expression for C++ 98. */
+cop = TREE_OPERAND (cop, 0);
What does this have to do with C++98?
+ if (TREE_CODE (cop) == PTRMEM_CST)
+{
+ /* The address of a nonstatic data member is never null. */
+ warning_at (location, OPT_Waddress,
+ "the address %qE will never be NULL",
Capitalizing NULL when talking about pointers-to-members seems a bit
odd, but I guess it's fine.
The C++ changes are OK.
Jason
Re: [PATCH] warn for more impossible null pointer tests [PR102103]
On 9/8/21 2:06 PM, Jason Merrill wrote:
On 9/2/21 7:53 PM, Martin Sebor wrote:
@@ -4622,28 +4622,94 @@ warn_for_null_address (location_t location,
tree op, tsubst_flags_t complain)
if (!warn_address
|| (complain & tf_warning) == 0
|| c_inhibit_evaluation_warnings != 0
- || warning_suppressed_p (op, OPT_Waddress))
+ || warning_suppressed_p (op, OPT_Waddress)
+ || processing_template_decl != 0)
Completely suppressing this warning in templates seems like a
regression; I'd think we could recognize many relevant cases before
instantiation. You just can't assume that ADDR_EXPR has the default
meaning if it has unknown type (i.e. because op0 is type-dependent).
I added the suppression to keep g++.dg/warn/pr101219.C from failing
but in hindsight I should have questioned the reasoning behind
the "no warning emitted here (no instantiation)" comment in the test.
I agree that it would be helpful to diagnose the type-independent
subset of the problem even in uninstantiated templates. Current
trunk doesn't (it never has), but with my patch and the suppression
above removed it does. I've updated the tests to expect it.
Please see the attached revision.
Martin
PS There are still more opportunities to issue -Waddress in templates
that this patch doesn't handle, e.g.,:
template bool f (T *p) { return == 0; }
Handling this will take more surgery.
PPS It seems that most other warnings (and even some errors, like
-Wnarrowing) are suppressed in uninstantiated templates as well,
even for non-dependent expressions. In the few test cases I looked
at Clang does better. It sounds like you'd like to see improvements
in this direction not just for -Waddress but in general. Just for
the avoidance of doubt, can you confirm that for future reference?
Jason
Enhance -Waddress to detect more suspicious expressions [PR102103].
Resolves:
PR c/102103 - missing warning comparing array address to null
gcc/ChangeLog:
* doc/invoke.texi (-Waddress): Update.
* gcc/gengtype.c (write_types): Avoid -Waddress.
gcc/c-family/ChangeLog:
* c-common.c (decl_with_nonnull_addr_p): Handle members.
Check and perform warning suppression.
(c_common_truthvalue_conversion): Enhance warning suppression.
gcc/c/ChangeLog:
* c-typeck.c (maybe_warn_for_null_address): New function.
(build_binary_op): Call it.
gcc/cp/ChangeLog:
* typeck.c (warn_for_null_address): Enhance.
(cp_build_binary_op): Call it also for member pointers.
gcc/fortran/ChangeLog:
* gcc/fortran/array.c: Remove an unnecessary test.
* gcc/fortran/trans-array.c: Same.
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/constexpr-array-ptr10.C: Suppress a valid warning.
* g++.dg/warn/Wreturn-local-addr-6.C: Correct a cast.
* gcc.dg/Waddress.c: Expect a warning.
* c-c++-common/Waddress-3.c: New test.
* c-c++-common/Waddress-4.c: New test.
* g++.dg/warn/Waddress-5.C: New test.
* g++.dg/warn/Waddress-6.C: New test.
* g++.dg/warn/pr101219.C: Expect a warning.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index c6757f093ac..249da7c7f0f 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3393,14 +3393,16 @@ c_wrap_maybe_const (tree expr, bool non_const)
return expr;
}
-/* Return whether EXPR is a declaration whose address can never be
- NULL. */
+/* Return whether EXPR is a declaration whose address can never be NULL.
+ The address of the first struct member could be NULL only if it were
+ accessed through a NULL pointer, and such an access would be invalid. */
bool
decl_with_nonnull_addr_p (const_tree expr)
{
return (DECL_P (expr)
- && (TREE_CODE (expr) == PARM_DECL
+ && (TREE_CODE (expr) == FIELD_DECL
+ || TREE_CODE (expr) == PARM_DECL
|| TREE_CODE (expr) == LABEL_DECL
|| !DECL_WEAK (expr)));
}
@@ -3488,13 +3490,17 @@ c_common_truthvalue_conversion (location_t location, tree expr)
case ADDR_EXPR:
{
tree inner = TREE_OPERAND (expr, 0);
- if (decl_with_nonnull_addr_p (inner))
+ if (decl_with_nonnull_addr_p (inner)
+ /* Check both EXPR and INNER for suppression. */
+ && !warning_suppressed_p (expr, OPT_Waddress)
+ && !warning_suppressed_p (inner, OPT_Waddress))
{
- /* Common Ada programmer's mistake. */
+ /* Common Ada programmer's mistake. */
warning_at (location,
OPT_Waddress,
"the address of %qD will always evaluate as %",
inner);
+ suppress_warning (inner, OPT_Waddress);
return truthvalue_true_node;
}
break;
@@ -3627,8 +3633,17 @@ c_common_truthvalue_conversion (location_t location, tree expr)
break;
/* If this isn't narrowing the argument, we can ignore it. */
if (TYPE_PRECISION (totype) >= TYPE_PRECISION (fromtype))
- return c_common_truthvalue_conversion (location,
- TREE_OPERAND (expr, 0));
+ {
+ tree op0 = TREE_OPERAND (expr, 0);
+ if ((TREE_CODE (fromtype) == POINTER_TYPE
+ && TREE_CODE (totype) == INTEGER_TYPE)
+ ||
Re: [PATCH] warn for more impossible null pointer tests [PR102103]
On 9/2/21 7:53 PM, Martin Sebor wrote: @@ -4622,28 +4622,94 @@ warn_for_null_address (location_t location, tree op, tsubst_flags_t complain) if (!warn_address || (complain & tf_warning) == 0 || c_inhibit_evaluation_warnings != 0 - || warning_suppressed_p (op, OPT_Waddress)) + || warning_suppressed_p (op, OPT_Waddress) + || processing_template_decl != 0) Completely suppressing this warning in templates seems like a regression; I'd think we could recognize many relevant cases before instantiation. You just can't assume that ADDR_EXPR has the default meaning if it has unknown type (i.e. because op0 is type-dependent). Jason
Re: [PATCH] warn for more impossible null pointer tests [PR102103]
Attached is an updated patch with Jason's suggested change to use
handled_component_p(), retested on x86_64-linux and with Glibc.
Adding more tests led to more changes but hopefully also a better
end result.
I've changed the warning suppression from a cast to void* to one
to intptr_t, in part because that's what Clang does, and in part
because I couldn't get it to work consistently between C and C++
(the C front end seems to introduce a NOP_EXPR in some cases even
when there is no cast in the source). In hindsight, a cast to
void* doesn't seem like the most intuitive way to avoid this sort
of warning.
Jeff, I didn't get your reply to my first post for some reason so
let me copy your question here and answer it below:
how does this interact with targets that allow objects at address
0? We have a few targets like that and that makes me wonder if
we should be suppressing some, if not all, of these warnings for
targets that turn on -fno-delete-null-pointer-checks?
I didn't see any code related to -Waddress that tries to handle those
targets. I have very little experience with them but I do know that
AVR makes it possible to pin an object to a fixed address by means
of attribute address. The test case below triggers -Waddress (both
with and without my changes):
__attribute__ ((address (0))) int x;
int f (void)
{
if ( == 0) return -1; // -Waddress
int *p =
int t = *p;
if (!p) __builtin_abort (); // folded to false
return t;
}
But the optimized code doesn't have the second test so I'm not sure
that the address attribute here does what I think it does (I'd
expect the test to be folded to true). If you have a better test
case or other targets for me to try I can look into it some more.
Martin
On 9/2/21 8:39 AM, Martin Sebor wrote:
On 9/2/21 7:43 AM, Jason Merrill wrote:
On 9/1/21 6:27 PM, Martin Sebor wrote:
On 9/1/21 3:39 PM, Jason Merrill wrote:
On 9/1/21 4:33 PM, Martin Sebor wrote:
On 9/1/21 1:21 PM, Jason Merrill wrote:
On 8/31/21 10:08 PM, Martin Sebor wrote:
A Coverity run recently uncovered a latent bug in GCC that GCC
should
be able to detect itself: comparing the address of a declared object
for equality to null, similar to:
int f (void)
{
int a[2][2];
return == 0;
}
GCC issues -Waddress for this code, but the bug Coverity found was
actually closer to the following:
int f (void)
{
int a[2][2];
return a[0] == 0;
}
where the hapless author (yours truly) meant to compare the value
of a[0][0] (as in r12-3268).
This variant is not diagnosed even though the bug in it is the same
and I'd expect more likely to occur in practice. ([0] == 0 isn't
diagnosed either, though that's a less likely mistake to make).
The attached patch enhances -Waddress to detect this variant along
with a number of other similar instances of the problem, including
comparing the address of array members to null.
Besides these, the patch also issues -Waddress for null equality
tests of pointer-plus expressions such as in:
int g (int i)
{
return a[0] + i == 0;
}
and in C++ more instances of pointers to members.
Testing on x86_64-linux, besides a few benign issues in GCC sources
a regression test, run shows a failure in gcc.dg/Waddress.c. That's
a test added after GCC for some reason stopped warning for one of
the basic cases that other tools warn about (comparing an array to
null). I suspect the change was unintentional because GCC still
warns for other very similar expressions. The reporter who also
submitted the test in pr36299 argued that the warning wasn't
helpful because tests for arrays sometimes come from macros, and
the test was committed after it was noted that GCC no longer warned
for the reporter's simple case. While it's certainly true that
the warning can be triggered by the null equality tests in macros
(the patch exposed two such instances in GCC) they are easy to
avoid (the patch adds a an additional escape hatch). At the same
time, as is evident from the Coverity bug report and from the two
issues the enhancement exposes in the FORTRAN front end (even if
benign), issuing the warning in these cases does help find bugs
or mistaken assumptions. With that, I've changed the test to
expect the restored -Waddress warning instead.
Testing with Glibc exposed a couple of harmless comparisons of
arrays a large macro in vfprintf-internal.c. I'll submit a fix
to avoid the -Waddress instances if/when this enhancement is
approved.
Testing with Binutils/GDB also turned up a couple of pointless
comparison of arrays to null and a couple of uses in macros that
can be trivially suppressed.
Martin
PS Clang issues a warning for some of the same null pointer tests
the patch diagnoses, including gcc.dg/Waddress.c, except under at
least three different options: some under -Wpointer-bool-conversion,
others under -Wtautological-pointer-compare, and others still under
-Wtautological-compare.
+ while (TREE_CODE (cop) ==
