[PATCH] D37478: [analyzer] Implement pointer arithmetic on constants

[Devin Coughlin via Phabricator via cfe-commits]

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