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 &a == 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. (&a[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) == ARRAY_REF
+ || TREE_CODE (cop) == COMPONENT_REF)
+ {
+ unsigned opno = TREE_CODE (cop) == COMPONENT_REF;
+ cop = TREE_OPERAND (cop, opno);
+ }
1) Maybe 'while (handled_component_p (cop))'?
2) Why handle COMPONENT_REF differently? Operand 1 is the
FIELD_DECL, which doesn't have an address of its own.
This is because the address of a field is never null, regardless of
what the P in in &P->M points to.
True, though I'd change "invalid" to "undefined" in the comment for
decl_with_nonnull_addr_p.
(With the caveat mentioned in
the comment further up about the pointer used to access the member
being nonnull.) So this is diagnosed:
extern struct { int m; } *p;
bool b = &p->m == 0;
Using handled_component_p() in a loop would prevent that.
Would it? p isn't declared weak.
Maybe I misunderstood. This loop:
while (handled_component_p (cop))
cop = TREE_OPERAND (cop, 0);
would unwrap the COMPONENT_REF from cop and terminate with it set
to INDIRECT_REF for which decl_with_nonnull_addr_p() would return
false. But if you meant to keep the body as is and just change
the condition, that would work. If you think that's better,
e.g., because it would handle more cases, I'm all for it.
It's worth considering the other codes in handled_component_p, at least.
BIT_FIELD_REF we shouldn't see an address of. I don't think the C++
front end produces ARRAY_RANGE_REF. I'd think REALPART_EXPR and
IMAGPART_EXPR would be like COMPONENT_REF. I guess we would want to
look through VIEW_CONVERT_EXPR.
Okay, let me update the patch then and repost before committing.
For array_refs, the loop gets us the decl to mention in the warning.
But this should work too and looks cleaner:
cop = TREE_OPERAND (cop, 0);
/* Get the outermost array. */
while (TREE_CODE (cop) == ARRAY_REF)
cop = TREE_OPERAND (cop, 0);
/* Get the member (its address is never null). */
if (TREE_CODE (cop) == COMPONENT_REF)
cop = TREE_OPERAND (cop, 1);
Do you prefer the above instead?
Sure. OK with that change and the comment tweak above.
Thanks. Are you approving the whole patch (including the C FE
changes) or just the C++ part?
My impression was that Jeff approved the rest of the patch, so I was
only really looking at the C++ part.
That's odd (and worrisome). I didn't get that email for some
strange reason but I just found it in the archive. Let me also
respond to Jeff's question in my follow up.
Martin
