[analyzer] Refactor makeNull to makeNullWithWidth (NFC)

Usages of makeNull need to be deprecated in favor of makeNullWithWidth
for architectures where the pointer size should not be assumed. This can
occur when pointer sizes can be of different sizes, depending on address
space for example. See https://reviews.llvm.org/D118050 as an example.

This was uncovered initially in a downstream compiler project, and
tested through those systems tests.

steakhal performed systems testing across a large set of open source
projects.

Co-authored-by: steakhal
Resolves: https://github.com/llvm/llvm-project/issues/53664

Reviewed By: NoQ, steakhal

Differential Revision: https://reviews.llvm.org/D119601

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
index 1df47da..1f02ee8 100644 (file)
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -365,13 +365,21 @@ public:
   /// space.
   /// \param type pointer type.
   Loc makeNullWithType(QualType type) {
+    // We cannot use the `isAnyPointerType()`.
+    assert((type->isPointerType() || type->isObjCObjectPointerType() ||
+            type->isBlockPointerType() || type->isNullPtrType() ||
+            type->isReferenceType()) &&
+           "makeNullWithType must use pointer type");
+
+    // The `sizeof(T&)` is `sizeof(T)`, thus we replace the reference with a
+    // pointer. Here we assume that references are actually implemented by
+    // pointers under-the-hood.
+    type = type->isReferenceType()
+               ? Context.getPointerType(type->getPointeeType())
+               : type;
     return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type));
   }
 
-  Loc makeNull() {
-    return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth());
-  }
-
   Loc makeLoc(SymbolRef sym) {
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
   }

diff --git a/clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
index 4235c0c..e2b99fc 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -250,7 +250,7 @@ static void addCastTransition(const CallEvent &Call, DefinedOrUnknownSVal DV,
                                       CastSucceeds);
 
   SVal V = CastSucceeds ? C.getSValBuilder().evalCast(DV, CastToTy, CastFromTy)
-                        : C.getSValBuilder().makeNull();
+                        : C.getSValBuilder().makeNullWithType(CastToTy);
   C.addTransition(
       State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), V, false),
       getNoteTag(C, CastInfo, CastToTy, Object, CastSucceeds, IsKnownCast));
@@ -359,7 +359,9 @@ static void evalNullParamNullReturn(const CallEvent &Call,
   if (ProgramStateRef State = C.getState()->assume(DV, false))
     C.addTransition(State->BindExpr(Call.getOriginExpr(),
                                     C.getLocationContext(),
-                                    C.getSValBuilder().makeNull(), false),
+                                    C.getSValBuilder().makeNullWithType(
+                                        Call.getOriginExpr()->getType()),
+                                    false),
                     C.getNoteTag("Assuming null pointer is passed into cast",
                                  /*IsPrunable=*/true));
 }

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 238022d..63b1065 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2593,8 +2593,8 @@ MallocChecker::ReallocMemAux(CheckerContext &C, const CallEvent &Call,
 
   SValBuilder &svalBuilder = C.getSValBuilder();
 
-  DefinedOrUnknownSVal PtrEQ =
-    svalBuilder.evalEQ(State, arg0Val, svalBuilder.makeNull());
+  DefinedOrUnknownSVal PtrEQ = svalBuilder.evalEQ(
+      State, arg0Val, svalBuilder.makeNullWithType(arg0Expr->getType()));
 
   // Get the size argument.
   const Expr *Arg1 = CE->getArg(1);

diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index 0bde088..f43b8d9 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -945,7 +945,8 @@ bool RetainCountChecker::evalCall(const CallEvent &Call,
 
       // Assume that output is zero on the other branch.
       NullOutputState = NullOutputState->BindExpr(
-          CE, LCtx, C.getSValBuilder().makeNull(), /*Invalidate=*/false);
+          CE, LCtx, C.getSValBuilder().makeNullWithType(ResultTy),
+          /*Invalidate=*/false);
       C.addTransition(NullOutputState, &getCastFailTag());
 
       // And on the original branch assume that both input and

diff --git a/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
index c789a8d..d1b8db3 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -71,7 +71,8 @@ private:
   bool handleMoveCtr(const CallEvent &Call, CheckerContext &C,
                      const MemRegion *ThisRegion) const;
   bool updateMovedSmartPointers(CheckerContext &C, const MemRegion *ThisRegion,
-                                const MemRegion *OtherSmartPtrRegion) const;
+                                const MemRegion *OtherSmartPtrRegion,
+                                const CallEvent &Call) const;
   void handleBoolConversion(const CallEvent &Call, CheckerContext &C) const;
   bool handleComparisionOp(const CallEvent &Call, CheckerContext &C) const;
   bool handleOstreamOperator(const CallEvent &Call, CheckerContext &C) const;
@@ -379,11 +380,13 @@ bool SmartPtrModeling::evalCall(const CallEvent &Call,
     if (!ThisRegion)
       return false;
 
+    QualType ThisType = cast<CXXMethodDecl>(Call.getDecl())->getThisType();
+
     if (CC->getDecl()->isMoveConstructor())
       return handleMoveCtr(Call, C, ThisRegion);
 
     if (Call.getNumArgs() == 0) {
-      auto NullVal = C.getSValBuilder().makeNull();
+      auto NullVal = C.getSValBuilder().makeNullWithType(ThisType);
       State = State->set<TrackedRegionMap>(ThisRegion, NullVal);
 
       C.addTransition(
@@ -640,7 +643,8 @@ void SmartPtrModeling::handleRelease(const CallEvent &Call,
                             *InnerPointVal);
   }
 
-  auto ValueToUpdate = C.getSValBuilder().makeNull();
+  QualType ThisType = cast<CXXMethodDecl>(Call.getDecl())->getThisType();
+  auto ValueToUpdate = C.getSValBuilder().makeNullWithType(ThisType);
   State = State->set<TrackedRegionMap>(ThisRegion, ValueToUpdate);
 
   C.addTransition(State, C.getNoteTag([ThisRegion](PathSensitiveBugReport &BR,
@@ -738,13 +742,15 @@ bool SmartPtrModeling::handleAssignOp(const CallEvent &Call,
   if (!ThisRegion)
     return false;
 
+  QualType ThisType = cast<CXXMethodDecl>(Call.getDecl())->getThisType();
+
   const MemRegion *OtherSmartPtrRegion = OC->getArgSVal(0).getAsRegion();
   // In case of 'nullptr' or '0' assigned
   if (!OtherSmartPtrRegion) {
     bool AssignedNull = Call.getArgSVal(0).isZeroConstant();
     if (!AssignedNull)
       return false;
-    auto NullVal = C.getSValBuilder().makeNull();
+    auto NullVal = C.getSValBuilder().makeNullWithType(ThisType);
     State = State->set<TrackedRegionMap>(ThisRegion, NullVal);
     C.addTransition(State, C.getNoteTag([ThisRegion](PathSensitiveBugReport &BR,
                                                      llvm::raw_ostream &OS) {
@@ -758,7 +764,7 @@ bool SmartPtrModeling::handleAssignOp(const CallEvent &Call,
     return true;
   }
 
-  return updateMovedSmartPointers(C, ThisRegion, OtherSmartPtrRegion);
+  return updateMovedSmartPointers(C, ThisRegion, OtherSmartPtrRegion, Call);
 }
 
 bool SmartPtrModeling::handleMoveCtr(const CallEvent &Call, CheckerContext &C,
@@ -767,17 +773,19 @@ bool SmartPtrModeling::handleMoveCtr(const CallEvent &Call, CheckerContext &C,
   if (!OtherSmartPtrRegion)
     return false;
 
-  return updateMovedSmartPointers(C, ThisRegion, OtherSmartPtrRegion);
+  return updateMovedSmartPointers(C, ThisRegion, OtherSmartPtrRegion, Call);
 }
 
 bool SmartPtrModeling::updateMovedSmartPointers(
     CheckerContext &C, const MemRegion *ThisRegion,
-    const MemRegion *OtherSmartPtrRegion) const {
+    const MemRegion *OtherSmartPtrRegion, const CallEvent &Call) const {
   ProgramStateRef State = C.getState();
+  QualType ThisType = cast<CXXMethodDecl>(Call.getDecl())->getThisType();
   const auto *OtherInnerPtr = State->get<TrackedRegionMap>(OtherSmartPtrRegion);
   if (OtherInnerPtr) {
     State = State->set<TrackedRegionMap>(ThisRegion, *OtherInnerPtr);
-    auto NullVal = C.getSValBuilder().makeNull();
+
+    auto NullVal = C.getSValBuilder().makeNullWithType(ThisType);
     State = State->set<TrackedRegionMap>(OtherSmartPtrRegion, NullVal);
     bool IsArgValNull = OtherInnerPtr->isZeroConstant();
 
@@ -803,7 +811,8 @@ bool SmartPtrModeling::updateMovedSmartPointers(
   } else {
     // In case we dont know anything about value we are moving from
     // remove the entry from map for which smart pointer got moved to.
-    auto NullVal = C.getSValBuilder().makeNull();
+    // For unique_ptr<A>, Ty will be 'A*'.
+    auto NullVal = C.getSValBuilder().makeNullWithType(ThisType);
     State = State->remove<TrackedRegionMap>(ThisRegion);
     State = State->set<TrackedRegionMap>(OtherSmartPtrRegion, NullVal);
     C.addTransition(State, C.getNoteTag([OtherSmartPtrRegion,
@@ -830,6 +839,8 @@ void SmartPtrModeling::handleBoolConversion(const CallEvent &Call,
   const MemRegion *ThisRegion =
       cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
+  QualType ThisType = cast<CXXMethodDecl>(Call.getDecl())->getThisType();
+
   SVal InnerPointerVal;
   if (const auto *InnerValPtr = State->get<TrackedRegionMap>(ThisRegion)) {
     InnerPointerVal = *InnerValPtr;
@@ -868,7 +879,7 @@ void SmartPtrModeling::handleBoolConversion(const CallEvent &Call,
     std::tie(NotNullState, NullState) =
         State->assume(InnerPointerVal.castAs<DefinedOrUnknownSVal>());
 
-    auto NullVal = C.getSValBuilder().makeNull();
+    auto NullVal = C.getSValBuilder().makeNullWithType(ThisType);
     // Explicitly tracking the region as null.
     NullState = NullState->set<TrackedRegionMap>(ThisRegion, NullVal);
 

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 26218b8..099e70a 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -549,8 +549,9 @@ void StreamChecker::evalFreopen(const FnDescription *Desc,
       State->BindExpr(CE, C.getLocationContext(), *StreamVal);
   // Generate state for NULL return value.
   // Stream switches to OpenFailed state.
-  ProgramStateRef StateRetNull = State->BindExpr(CE, C.getLocationContext(),
-                                                 C.getSValBuilder().makeNull());
+  ProgramStateRef StateRetNull =
+      State->BindExpr(CE, C.getLocationContext(),
+                      C.getSValBuilder().makeNullWithType(CE->getType()));
 
   StateRetNotNull =
       StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index 302a971..21fd46e 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -460,7 +460,8 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
             continue;
           } else {
             // If the cast fails on a pointer, bind to 0.
-            state = state->BindExpr(CastE, LCtx, svalBuilder.makeNull());
+            state = state->BindExpr(CastE, LCtx,
+                                    svalBuilder.makeNullWithType(resultType));
           }
         } else {
           // If we don't know if the cast succeeded, conjure a new symbol.
@@ -498,7 +499,7 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
         continue;
       }
       case CK_NullToPointer: {
-        SVal V = svalBuilder.makeNull();
+        SVal V = svalBuilder.makeNullWithType(CastE->getType());
         state = state->BindExpr(CastE, LCtx, V);
         Bldr.generateNode(CastE, Pred, state);
         continue;

diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index cad5f66..9d062dd 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2410,7 +2410,7 @@ RegionStoreManager::setImplicitDefaultValue(RegionBindingsConstRef B,
   SVal V;
 
   if (Loc::isLocType(T))
-    V = svalBuilder.makeNull();
+    V = svalBuilder.makeNullWithType(T);
   else if (T->isIntegralOrEnumerationType())
     V = svalBuilder.makeZeroVal(T);
   else if (T->isStructureOrClassType() || T->isArrayType()) {

diff --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index bb3261b..a4a65ad 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -61,7 +61,7 @@ SValBuilder::SValBuilder(llvm::BumpPtrAllocator &alloc, ASTContext &context,
 
 DefinedOrUnknownSVal SValBuilder::makeZeroVal(QualType type) {
   if (Loc::isLocType(type))
-    return makeNull();
+    return makeNullWithType(type);
 
   if (type->isIntegralOrEnumerationType())
     return makeIntVal(0, type);
@@ -359,7 +359,7 @@ Optional<SVal> SValBuilder::getConstantVal(const Expr *E) {
     return makeBoolVal(cast<ObjCBoolLiteralExpr>(E));
 
   case Stmt::CXXNullPtrLiteralExprClass:
-    return makeNull();
+    return makeNullWithType(E->getType());
 
   case Stmt::CStyleCastExprClass:
   case Stmt::CXXFunctionalCastExprClass:
@@ -399,7 +399,7 @@ Optional<SVal> SValBuilder::getConstantVal(const Expr *E) {
 
     if (Loc::isLocType(E->getType()))
       if (E->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNotNull))
-        return makeNull();
+        return makeNullWithType(E->getType());
 
     return None;
   }

diff --git a/clang/test/Analysis/cast-value-notes.cpp b/clang/test/Analysis/cast-value-notes.cpp
index a095863..19a9f66 100644 (file)
--- a/clang/test/Analysis/cast-value-notes.cpp
+++ b/clang/test/Analysis/cast-value-notes.cpp
@@ -1,9 +1,22 @@
 // RUN: %clang_analyze_cc1 -std=c++14 \
 // RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN:  -analyzer-output=text -verify %s
+// RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
 
 #include "Inputs/llvm.h"
 
+// The amggcn triple case uses an intentionally different address space.
+// The core.NullDereference checker intentionally ignores checks
+// that use address spaces, so the case is differentiated here.
+//
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+#define DEVICE __attribute__((address_space(3)))
+
 namespace clang {
 struct Shape {
   template <typename T>
@@ -21,12 +34,30 @@ class Circle : public Shape {};
 using namespace llvm;
 using namespace clang;
 
+void clang_analyzer_printState();
+
+#if defined(DEFAULT_TRIPLE)
 void evalReferences(const Shape &S) {
   const auto &C = dyn_cast<Circle>(S);
   // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
+  clang_analyzer_printState();
+  // DEFAULT-CHECK: "dynamic_types": [
+  // DEFAULT-CHECK-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#elif defined(AMDGCN_TRIPLE)
+void evalReferences(const Shape &S) {
+  const auto &C = dyn_cast<DEVICE Circle>(S);
+  clang_analyzer_printState();
+  // AMDGCN-CHECK: "dynamic_types": [
+  // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
 }
+#else
+#error Target must be specified, and must be pinned
+#endif
 
 void evalNonNullParamNonNullReturnReference(const Shape &S) {
   const auto *C = dyn_cast_or_null<Circle>(S);