[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

These assertions are fundamental, so we can't remove them; i believe we messed up modeling at some point. I'll pick this up to address some nasty regressions quickly; i managed to reproduce the crashes and i already have 4 creduces running.



================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:743-750
+  QualType RegionType = DynType.getType();
+  if (RegionType->isPointerType())
+    RegionType = RegionType->getPointeeType();
+  else
+    RegionType = RegionType.getNonReferenceType();
+
+  assert(!RegionType.isNull() &&
----------------
I don't think this does anything:
```lang=c++
   505 QualType Type::getPointeeType() const {
   506   if (const auto *PT = getAs<PointerType>())
   507     return PT->getPointeeType();
   508   if (const auto *OPT = getAs<ObjCObjectPointerType>())
   509     return OPT->getPointeeType();
   510   if (const auto *BPT = getAs<BlockPointerType>())
   511     return BPT->getPointeeType();
   512   if (const auto *RT = getAs<ReferenceType>())
   513     return RT->getPointeeType();
   514   if (const auto *MPT = getAs<MemberPointerType>())
   515     return MPT->getPointeeType();
   516   if (const auto *DT = getAs<DecayedType>())
   517     return DT->getPointeeType();
   518   return {};
   519 }
```
This getter usually works very reliably for both pointers and references.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:118-122
+  for (const auto &Elem : Map) {
+    const MemRegion *MR = Elem.first;
+    if (MR && !SR.isLiveRegion(MR))
+      State = State->remove<DynamicCastMap>(MR);
+  }
----------------
We shouldn't put null regions into our maps.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:329-336
+  // FIXME: Make this assertion great again.
+  /* else {
     // We need to create a region no matter what. For sanity, make sure we don't
     // try to stuff a Loc into a non-pointer temporary region.
     assert(!InitValWithAdjustments.getAs<Loc>() ||
            Loc::isLocType(Result->getType()) ||
            Result->getType()->isMemberPointerType());
----------------
This usually fails when we mess up lvalue/rvalue vs. loc/nonloc invariants in our modeling.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1079-1080
+    // FIXME: Make this assertion great again.
+    /*assert(isValidBaseClass(RD, dyn_cast<TypedValueRegion>(Super), IsVirtual));
+    (void)&isValidBaseClass;*/
 
----------------
Ugh, i suspect that we can't pass through the original pointer in our cast modeling; we need to actually model pointer casts, which is annoying but necessary, given that the cast doesn't necessarily yield the same pointer value even in run-time (see multiple inheritance).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593