[PATCH] D15007: [analyzer] Improve modelling of nullptr_t in the analyzer. Fix PR25414.

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Gabor, thanks for taking a look at this!

I'm not sure `RegionStore::getBinding()` is the right place to put this logic.

First, not all dereferences of a `std::nullptr_t` value go through `getBinding()`, so, for example, the following snippet triggers the assertion even with your patch:

  decltype(nullptr) returnsNullPtrType();
  void fromReturnType() {
    ((X *)returnsNullPtrType())->f(); // Crash in analysis!
  }

Second, not all locations of type `std::nullptr_t` contain `0`. For example, the following prints "1" on my machine:

  #include <iostream>
  
  int main() {
    decltype(nullptr) x;
    std::cout << (bool)x << "\n";
  }

That is, a default-initialized (garbage) nullptr_t may not be '0'. Prior to your patch the analyzer would warn here about `x` being uninitialized (when the analyzer is compiled without asserts) -- but with your patch we lose that warning.

What do you think about moving your new logic to the symbol value creation methods in SValBuilder? More specifically, you could add the check for nullptr_t to `getRegionValueSymbolVal()`, `conjureSymbolVal()` (both variants), `getConjuredHeapSymbolVal()`, and `getDerivedRegionValueSymbolVal()`. This would let the analyzer warn when it knows there is a garbage value in a location of type nullptr_t but still optimistically assume that the value is is '0' when the value would otherwise be symbolic.

With this approach we would retain uninitialization warnings about accesses via definitely uninitialized storage:

  void fromUninitializedLocal() {
    decltype(nullptr) p;
    ((X *)p)->f(); // Warn about p being uninitialized
  }
  
  void fromUninitializedLocalStruct() {
    Type t;
    ((X *)t.x)->f(); // Warn about t.x being uninitialized.
  }

but would warn about about a null access in `returnsNullPtrType()` and would keep the behavior you specify in your `f()` test because `p` is a parameter (and thus its value is symbolic).


http://reviews.llvm.org/D15007