dcoughlin requested changes to this revision.
dcoughlin added a subscriber: zaks.anna.
dcoughlin added a comment.
This revision now requires changes to proceed.
Rafael: Thanks for the patch! @NoQ, @zaks.anna, and I spoke about this off-line
yesterday.
While this patch improves the modeling of pointer arithmetic, we're worried
about losing valuable alarms that rely on the existing behavior.
Here is a case where the analyzer would warn before your patch but doesn't with
it:
void foo() {
int *p = 0;
int q = *(p + 5); // expected-warning {{Dereference of null pointer}}
}
The existing diagnostic machinery relies on the fact that the analyzer treats
`p + 5` as 0 to report the bad dereference. This comes up more often than you
might think because the analyzer is sometimes quite aggressive about promoting
a symbol constrained to 0 to be a concrete value of 0. For example:
void bar(int *p) {
if (p)
return;
int q = *(p + 5); // expected-warning {{Dereference of null pointer}}
}
It would be good to add test cases for these diagnostics!
I think you can preserve the existing (although missing from the test suite)
good diagnostics and still improve the modeling by skipping the
addition/subtraction if the LHS is a concrete int with value 0. Doing so would
be a very minor change to this patch.
Modeling the pointer arithmetic when the LHS is 0 while still keeping the
diagnostics will likely be a more involved effort, with ramifications in
multiple parts of the analyzer. We could discuss that, if you'd like to tackle
it! But it would probably be good for you to get a couple more patches under
your belt before taking that on.
================
Comment at: test/Analysis/inlining/inline-defensive-checks.c:144
+
+// Intuitively the following tests should all warn about a null dereference,
+// since the object pointer the operations are based on can be null.
----------------
The analyzer doesn't warn on these on purpose.
Throughout the analyzer, we have a broad heuristic that says: "if the
programmer compares a pointer to NULL, then the analyzer should explicitly
consider the case that the pointer is NULL". It will perform a case split in
the symbolic execution: one case for when the value is definitely NULL and one
case for one it is definitely not NULL. As a heuristic this works reasonably
well: if the programmer bothered to add a check for NULL then they likely
though the value could be NULL.
However, when context-sensitive analysis via inlining was added, this heuristic
broke down for functions that called other functions with what we call "inlined
defensive checks" or null.
Here is an example:
```
void hasInlinedDefensiveCheck(int *p) {
if (!p)
return;
// Do something useful
}
void foo(int *param) {
hasInlinedDefensiveCheck(param);
*param = 7;
}
```
In this case the warning about `*param = 7` is a false positive from foo's
point of view because foo may have a strong invariant that param is not null;
it doesn't care that hasInlinedDefensiveCheck() may have other callers that
might call it with null. For this reason, we suppress reports about null
pointer dereferences when we can detect an inlined defense check.
https://reviews.llvm.org/D37478
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
