[analyzer] Fix assertion in SVals.h

[vabridgers]

Summary

Fix assertion in SVals.h apparently caused by
https://reviews.llvm.org/D89055.

clang:clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:596:
clang::ento::loc::MemRegionVal::MemRegionVal(const clang::ento::MemRegion *):

Assertion `r' failed.

Backtrace:
...

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:597:3
clang::QualType, clang::QualType)
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:773:18
clang::QualType, clang::QualType)
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:612:12
clang::QualType) clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:587:12
namespace)::RegionBindingsRef const&, clang::ento::Loc, clang::QualType)
clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1510:24

...

[ASDenysPetrov]

@vabridgers
Thank you for a good catch! The fix looks fairly reasonable for me!

[steakhal]

I don't know how did we miss this. I run your patch on several projects and it seemed good. Does anyone have an idea how to prevent such a silly mistake from happening again? I was thinking of coverage data, but that wouldn't be enough for this example.

clang/test/Analysis/casts.c

254–268

Please, @ASDenysPetrov investigate this.
I also think that this test case could be simplified, and a no-crash comment would be also appreciated at the corresponding line.

[NoQ]

In D101635#2731210, @steakhal wrote:

Does anyone have an idea how to prevent such a silly mistake from happening again?

Maybe use more optionals? I.e., castRegion may fail, so let's change it to return Optional<const MemRegion *> where the pointer is always non-null when present? In many cases it's probably unnecessary but when a function can return null for poorly predictable reasons (eg., like in this case, the region turning out to have symbolic offset after a potentially unlimited number of unwraps) it's probably worth it.

[ASDenysPetrov]

@steakhal, @NoQ
Thanks for your replies. I've made a patch according your suggestions D103319.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

if (IsUnknownOriginalType) {
             R = StateMgr.getStoreManager().castRegion(SR, CastTy);
             return loc::MemRegionVal(R);
           }
         }
       }
       // 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)) {
-        R = StateMgr.getStoreManager().castRegion(ER, CastTy);
+        if ((R = StateMgr.getStoreManager().castRegion(ER, CastTy)))
           return loc::MemRegionVal(R);
       }
 
       return V;
     }
 
     if (OriginalTy->isIntegralOrEnumerationType() ||
         OriginalTy->isBlockPointerType() || OriginalTy->isFunctionPointerType())
       return V;

clang/test/Analysis/casts.c

double no_crash_reinterpret_double_as_sym_ptr(double a, void * b) {
   return a * a;
 }
 
 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;
+}