This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Improve modelling of nullptr_t in the analyzer. Fix PR25414.
ClosedPublic

Authored by xazax.hun on Nov 26 2015, 1:34 AM.

Diff Detail

Event Timeline

xazax.hun updated this revision to Diff 41213.Nov 26 2015, 1:34 AM
xazax.hun retitled this revision from to [analyzer] Improve modelling of nullptr_t in the analyzer. Fix PR25414..
xazax.hun updated this object.
xazax.hun added reviewers: zaks.anna, dcoughlin.
xazax.hun added subscribers: cfe-commits, dkrupp.
NoQ added a subscriber: NoQ.Nov 26 2015, 2:17 AM

Wow, useful stuff!

There's a little problem with the shouldNotCrash() test: the first warning on invokeF() on line 107 generates a sink, and the rest of the function never gets executed. It's probably a good idea to split it into three separate tests. Then it'd also warn on line 110.

xazax.hun updated this revision to Diff 41221.Nov 26 2015, 2:24 AM

Improved test cases.

In D15007#297183, @NoQ wrote:

There's a little problem with the shouldNotCrash() test: the first warning on invokeF() on line 107 generates a sink, and the rest of the function never gets executed. It's probably a good idea to split it into three separate tests. Then it'd also warn on line 110.

Thank you for pointing this out! I originally used separate test files and did not notice this problem after merging the into the regression test suite.

dcoughlin edited edge metadata.Nov 28 2015, 5:18 PM

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).

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!
}

Thank you for spotting this, I will definitely try the approach you suggested.

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.

I am not entirely sure what the correct behavior is here. According to the standard (2.14.7): "std::nullptr_t is a distinct type that is neither a pointer type nor a pointer to member type; rather, a prvalue of this type is a null pointer constant and can be converted to a null pointer value or null member pointer value." My interpretation is that, the std::nullptr_t does not have a state, it just converts to a null pointer value regardless what value is at the location of the nullptr_t. Regardless what the standard says it looks like the value of std::nullptr_t can be (ab)used in the current clang implementation.

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).

I will look into this thank you.

xazax.hun updated this revision to Diff 41494.Dec 1 2015, 5:21 AM
xazax.hun edited edge metadata.
  • Added the test case Devin suggested.
  • Implemented the fix Devin suggested.
  • Updated to latest trunk.
dcoughlin accepted this revision.Dec 4 2015, 6:58 AM
dcoughlin edited edge metadata.

LGTM. Thanks Gabor!

This revision is now accepted and ready to land.Dec 4 2015, 6:58 AM
This revision was automatically updated to reflect the committed changes.