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

[xazax.hun]

Summary

This is a fix for https://llvm.org/bugs/show_bug.cgi?id=25414

This patch is intended to improve the modelling of std::nullptr_t.

[NoQ]

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]

Improved test cases.

[xazax.hun]

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]

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

[xazax.hun]

In D15007#298086, @dcoughlin wrote:

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]

• Added the test case Devin suggested.
• Implemented the fix Devin suggested.
• Updated to latest trunk.

[dcoughlin]

LGTM. Thanks Gabor!

cfe/trunk/lib/StaticAnalyzer/Core/SValBuilder.cpp

 nonloc::ConcreteInt SValBuilder::makeBoolVal(const CXXBoolLiteralExpr *boolean){
   return makeTruthVal(boolean->getValue());
 }
 
 DefinedOrUnknownSVal
 SValBuilder::getRegionValueSymbolVal(const TypedValueRegion* region) {
   QualType T = region->getValueType();
 
+  if (T->isNullPtrType())
+    return makeZeroVal(T);
+  
   if (!SymbolManager::canSymbolicate(T))
     return UnknownVal();
 
   SymbolRef sym = SymMgr.getRegionValueSymbol(region);
 
   if (Loc::isLocType(T))
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
 
   return nonloc::SymbolVal(sym);
 }
 
 DefinedOrUnknownSVal SValBuilder::conjureSymbolVal(const void *SymbolTag,
                                                    const Expr *Ex,
                                                    const LocationContext *LCtx,
                                                    unsigned Count) {
   QualType T = Ex->getType();
 
+  if (T->isNullPtrType())
+    return makeZeroVal(T);
+
   // Compute the type of the result. If the expression is not an R-value, the
   // result should be a location.
   QualType ExType = Ex->getType();
   if (Ex->isGLValue())
     T = LCtx->getAnalysisDeclContext()->getASTContext().getPointerType(ExType);
 
   return conjureSymbolVal(SymbolTag, Ex, LCtx, T, Count);
 }
 
 DefinedOrUnknownSVal SValBuilder::conjureSymbolVal(const void *symbolTag,
                                                    const Expr *expr,
                                                    const LocationContext *LCtx,
                                                    QualType type,
                                                    unsigned count) {
+  if (type->isNullPtrType())
+    return makeZeroVal(type);
+
   if (!SymbolManager::canSymbolicate(type))
     return UnknownVal();
 
   SymbolRef sym = SymMgr.conjureSymbol(expr, LCtx, type, count, symbolTag);
 
   if (Loc::isLocType(type))
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
 
   return nonloc::SymbolVal(sym);
 }
 
 
 DefinedOrUnknownSVal SValBuilder::conjureSymbolVal(const Stmt *stmt,
                                                    const LocationContext *LCtx,
                                                    QualType type,
                                                    unsigned visitCount) {
+  if (type->isNullPtrType())
+    return makeZeroVal(type);
+
   if (!SymbolManager::canSymbolicate(type))
     return UnknownVal();
 
   SymbolRef sym = SymMgr.conjureSymbol(stmt, LCtx, type, visitCount);
 
   if (Loc::isLocType(type))
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
 
   return nonloc::SymbolVal(sym);
 }
 
 DefinedOrUnknownSVal
 SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
                                       const LocationContext *LCtx,
                                       unsigned VisitCount) {
   QualType T = E->getType();
   assert(Loc::isLocType(T));
   assert(SymbolManager::canSymbolicate(T));
+  if (T->isNullPtrType())
+    return makeZeroVal(T);
 
   SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
 DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag,
                                               const MemRegion *region,
                                               const Expr *expr, QualType type,
DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag,
   return nonloc::SymbolVal(sym);
 }
 
 DefinedOrUnknownSVal
 SValBuilder::getDerivedRegionValueSymbolVal(SymbolRef parentSymbol,
                                              const TypedValueRegion *region) {
   QualType T = region->getValueType();
 
+  if (T->isNullPtrType())
+    return makeZeroVal(T);
+
   if (!SymbolManager::canSymbolicate(T))
     return UnknownVal();
 
   SymbolRef sym = SymMgr.getDerivedSymbol(parentSymbol, region);
 
   if (Loc::isLocType(T))
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
 

cfe/trunk/test/Analysis/nullptr.cpp

-// RUN: %clang_cc1 -std=c++11 -Wno-conversion-null -analyze -analyzer-checker=core -analyzer-store region -verify %s
+// RUN: %clang_cc1 -std=c++11 -Wno-conversion-null -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store region -verify %s
+
+void clang_analyzer_eval(int);
 
 // test to see if nullptr is detected as a null pointer
 void foo1(void) {
   char  *np = nullptr;
   *np = 0;  // expected-warning{{Dereference of null pointer}}
 }
 
 // check if comparing nullptr to nullptr is detected properly
int materializeTempExpr() {
   return s.a;
 }
 
 typedef decltype(nullptr) nullptr_t;
 void testMaterializeTemporaryExprWithNullPtr() {
   // Create MaterializeTemporaryExpr with a nullptr inside.
   const nullptr_t &r = nullptr;
 }
+
+int getSymbol();
+
+struct X {
+  virtual void f() {}
+};
+
+void invokeF(X* x) {
+  x->f(); // expected-warning{{Called C++ object pointer is null}}
+}
+
+struct Type {
+  decltype(nullptr) x;
+};
+
+void shouldNotCrash() {
+  decltype(nullptr) p;
+  if (getSymbol())
+    invokeF(p); // expected-warning{{Function call argument is an uninit}}
+  if (getSymbol())
+    invokeF(nullptr);
+  if (getSymbol()) {
+    X *x = Type().x;
+    x->f(); // expected-warning{{Called C++ object pointer is null}}
+  }
+}
+
+void f(decltype(nullptr) p) {
+  int *q = nullptr;
+  clang_analyzer_eval(p == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(q == 0); // expected-warning{{TRUE}}
+}
+
+decltype(nullptr) returnsNullPtrType();
+void fromReturnType() {
+  ((X *)returnsNullPtrType())->f(); // expected-warning{{Called C++ object pointer is null}}
+}