[analyzer] Use Optional as a return type of StoreManager::castRegion

[ASDenysPetrov]

Summary

Make StoreManager::castRegion function usage safier. Replace const MemRegion * with Optional<const MemRegion *>. Simplified one of related test cases due to suggestions in D101635.

[ASDenysPetrov]

Added a simplified test case.

[ASDenysPetrov]

Fixed syntax complaints which caused a test fail.

[NoQ]

Looks great, thanks!

I guess another option is to put loc::MemRegionVal() inside castRegion(). This way the return type Optional<loc::MemRegionVal> unambigously tells that the region is always non-null if present (protected by the assertion in the constructor of loc::MemRegionVal).

[ASDenysPetrov]

@NoQ

I guess another option is to put loc::MemRegionVal() inside castRegion(). This way the return type Optional<loc::MemRegionVal> unambigously tells that the region is always non-null if present (protected by the assertion in the constructor of loc::MemRegionVal).

IMO we should keep const MemRegion * for castRegion as is, since it's intended for cast and should behave as cast. I'd better add a kinda wrapper SValBuilder::getCastedMemRegionVal. WDYT?

[NoQ]

In D103319#2788304, @ASDenysPetrov wrote:

WDYT?

Fair point!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h

public:
   /// valid only if Failed flag is set to false.
   SVal attemptDownCast(SVal Base, QualType DerivedPtrType, bool &Failed);
 
   const ElementRegion *GetElementZeroRegion(const SubRegion *R, QualType T);
 
   /// castRegion - Used by ExprEngine::VisitCast to handle casts from
   ///  a MemRegion* to a specific location type.  'R' is the region being
   ///  casted and 'CastToTy' the result type of the cast.
-  const MemRegion *castRegion(const MemRegion *region, QualType CastToTy);
+  Optional<const MemRegion *> castRegion(const MemRegion *region,
+                                         QualType CastToTy);
 
   virtual StoreRef removeDeadBindings(Store store, const StackFrameContext *LCtx,
                                       SymbolReaper &SymReaper) = 0;
 
   virtual bool includedInBindings(Store store,
                                   const MemRegion *region) const = 0;
 
   /// If the StoreManager supports it, increment the reference count of

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

if (IsUnknownOriginalType) {
       // pointers as well.
       // FIXME: We really need a single good function to perform casts for us
       // correctly every time we need it.
       const MemRegion *R = V.getRegion();
       if (CastTy->isPointerType() && !CastTy->isVoidPointerType()) {
         if (const auto *SR = dyn_cast<SymbolicRegion>(R)) {
           QualType SRTy = SR->getSymbol()->getType();
           if (!hasSameUnqualifiedPointeeType(SRTy, CastTy)) {
-            R = StateMgr.getStoreManager().castRegion(SR, CastTy);
-            return loc::MemRegionVal(R);
+            if (auto OptR = StateMgr.getStoreManager().castRegion(SR, CastTy))
+              return loc::MemRegionVal(*OptR);
           }
         }
       }
       // Next fixes pointer dereference using type different from its initial
       // one. See PR37503 and PR49007 for details.
       if (const auto *ER = dyn_cast<ElementRegion>(R)) {
-        if ((R = StateMgr.getStoreManager().castRegion(ER, CastTy)))
-          return loc::MemRegionVal(R);
+        if (auto OptR = StateMgr.getStoreManager().castRegion(ER, CastTy))
+          return loc::MemRegionVal(*OptR);
       }
 
       return V;
     }
 
     if (OriginalTy->isIntegralOrEnumerationType() ||
         OriginalTy->isBlockPointerType() || OriginalTy->isFunctionPointerType())
       return V;
if (Loc::isLocType(CastTy)) {
     //  int f1_a(struct FPRec* foo) {
     //    int x;
     //    (*foo->my_func)(&x);
     //    return bar(x)+1; // no-warning
     //  }
 
     // Get the result of casting a region to a different type.
     const MemRegion *R = V.getRegion();
-    if ((R = StateMgr.getStoreManager().castRegion(R, CastTy)))
-      return loc::MemRegionVal(R);
+    if (auto OptR = StateMgr.getStoreManager().castRegion(R, CastTy))
+      return loc::MemRegionVal(*OptR);
   }
 
   // Pointer to whatever else.
   // FIXME: There can be gross cases where one casts the result of a
   // function (that returns a pointer) to some other value that happens to
   // fit within that pointer value.  We currently have no good way to model
   // such operations.  When this happens, the underlying operation is that
   // the caller is reasoning about bits.  Conceptually we are layering a
if (CastTy->isBooleanType())
     // Pass to Loc function.
     return evalCastKind(L, CastTy, OriginalTy);
 
   const bool IsUnknownOriginalType = OriginalTy.isNull();
   // Pointer as integer to pointer.
   if (!IsUnknownOriginalType && Loc::isLocType(CastTy) &&
       OriginalTy->isIntegralOrEnumerationType()) {
     if (const MemRegion *R = L.getAsRegion())
-      if ((R = StateMgr.getStoreManager().castRegion(R, CastTy)))
-        return loc::MemRegionVal(R);
+      if (auto OptR = StateMgr.getStoreManager().castRegion(R, CastTy))
+        return loc::MemRegionVal(*OptR);
     return L;
   }
 
   // Pointer as integer with region to integer/pointer.
   const MemRegion *R = L.getAsRegion();
   if (!IsUnknownOriginalType && R) {
     if (CastTy->isIntegralOrEnumerationType())
       return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);
 
     if (Loc::isLocType(CastTy)) {
       assert(Loc::isLocType(OriginalTy) || OriginalTy->isFunctionType() ||
              CastTy->isReferenceType());
       // Delegate to store manager to get the result of casting a region to a
       // different type. If the MemRegion* returned is NULL, this expression
       // Evaluates to UnknownVal.
-      if ((R = StateMgr.getStoreManager().castRegion(R, CastTy)))
-        return loc::MemRegionVal(R);
+      if (auto OptR = StateMgr.getStoreManager().castRegion(R, CastTy))
+        return loc::MemRegionVal(*OptR);
     }
   } else {
     if (Loc::isLocType(CastTy)) {
       if (IsUnknownOriginalType)
         return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);
       return L;
     }
 

clang/lib/StaticAnalyzer/Core/Store.cpp

 
 const ElementRegion *StoreManager::GetElementZeroRegion(const SubRegion *R,
                                                         QualType T) {
   NonLoc idx = svalBuilder.makeZeroArrayIndex();
   assert(!T.isNull());
   return MRMgr.getElementRegion(T, idx, R, Ctx);
 }
 
-const MemRegion *StoreManager::castRegion(const MemRegion *R, QualType CastToTy) {
+Optional<const MemRegion *> StoreManager::castRegion(const MemRegion *R,
+                                                     QualType CastToTy) {
   ASTContext &Ctx = StateMgr.getContext();
 
   // Handle casts to Objective-C objects.
   if (CastToTy->isObjCObjectPointerType())
     return R->StripCasts();
 
   if (CastToTy->isBlockPointerType()) {
     // FIXME: We may need different solutions, depending on the symbol
     // involved.  Blocks can be casted to/from 'id', as they can be treated
     // as Objective-C objects.  This could possibly be handled by enhancing
     // our reasoning of downcasts of symbolic objects.
     if (isa<CodeTextRegion>(R) || isa<SymbolicRegion>(R))
       return R;
 
     // We don't know what to make of it.  Return a NULL region, which
     // will be interpreted as UnknownVal.
-    return nullptr;
+    return None;
   }
 
   // Now assume we are casting from pointer to pointer. Other cases should
   // already be handled.
   QualType PointeeTy = CastToTy->getPointeeType();
   QualType CanonPointeeTy = Ctx.getCanonicalType(PointeeTy);
 
   // Handle casts to void*.  We just pass the region through.
case MemRegion::ElementRegionKind: {
 
       const ElementRegion *elementR = cast<ElementRegion>(R);
       const RegionRawOffset &rawOff = elementR->getAsArrayOffset();
       const MemRegion *baseR = rawOff.getRegion();
 
       // If we cannot compute a raw offset, throw up our hands and return
       // a NULL MemRegion*.
       if (!baseR)
-        return nullptr;
+        return None;
 
       CharUnits off = rawOff.getOffset();
 
       if (off.isZero()) {
         // Edge case: we are at 0 bytes off the beginning of baseR.  We
         // check to see if type we are casting to is the same as the base
         // region.  If so, just return the base region.
         if (const auto *TR = dyn_cast<TypedValueRegion>(baseR)) {

clang/test/Analysis/casts.c

 }
 
 void no_crash_reinterpret_char_as_uchar(char ***a, int *b) {
   *(unsigned char **)a = (unsigned char *)b;
   if (**a == 0) // no-crash
     ;
 }
 
-// See PR50179.
-// Just don't crash.
-typedef struct taskS {
-  void *pJob;
-} taskS;
-
-typedef struct workS {
-  taskS *pTaskList;
-} workS;
-
-void *getTaskJob(unsigned jobId, workS *pWork, unsigned taskId) {
-  const taskS *pTask = pWork->pTaskList + taskId;
-  taskS task = *pTask;
-  return task.pJob;
+// PR50179.
+struct S {};
+void symbolic_offset(struct S *ptr, int i) {
+  const struct S *pS = ptr + i;
+  struct S s = *pS; // no-crash
 }