[analyzer] Replace StoreManager::CastRetrievedVal with SValBuilder::evalCast

[ASDenysPetrov]

Summary

Move logic from CastRetrievedVal to evalCast and replace CastRetrievedVal with evalCast. Also move guts from SimpleSValBuilder::dispatchCast inside evalCast.
OriginalTy become a default param, because evalCast intends to substitute dispatchCast, evalCastFromNonLoc and evalCastFromLoc which don't have OriginalTy param. OriginalTy provides additional information for casting, which is useful for some cases and useless for others. If OriginalTy.isNull() is true, then the cast performs based on CastTy only. Now evalCast operates in two ways. It retains all previous behavior and take over dispatchCast behavior.

From this patch use evalCast instead of dispatchCast, evalCastFromNonLoc and evalCastFromLoc functions. dispatchCast redirects to evalCast.

This patch ideally shall not change any behavior. (Should we mark it as [NFC]?)

This is an intermediate revision for D89055.

[steakhal]

Please, consider removing D95799 from the parent revisions if that is not a hard requirement of this patch.
It would be awesome to land your patches on dealing with cast problems in the close future.
Dealing with floating-point values is a plus, but maybe not a necessity.

[ASDenysPetrov]

In D96090#2544477, @steakhal wrote:

Please, consider removing D95799 from the parent revisions if that is not a hard requirement of this patch.

Actually D95799 is one of the steps to clean up CastRetrievedVal initially. It significantly reduce the next effort of replacing with evalCast. But if float cast fix blocks the patch stack, I can try to find a way to bypass it.

[ASDenysPetrov]

Updated. Unlinked parent revision D95799 from the stack. Made corresponding changes in this patch.
@steakhal I did it. The changes should be safer now.

[steakhal]

In D96090#2551664, @ASDenysPetrov wrote:

Updated. Unlinked parent revision D95799 from the stack. Made corresponding changes in this patch.
@steakhal I did it. The changes should be safer now.

In the upcoming days, I'm gonna schedule this on our CI. We will see the results.

[ASDenysPetrov]

@steakhal

In the upcoming days, I'm gonna schedule this on our CI. We will see the results.

Thank you. That would be great!

[mikhail.ramalho]

I'm glad to see these patches, the SMT API will benefit greatly from them!

[steakhal]

@ASDenysPetrov Could you please rebase this?
git apply does not seem to apply this patch on top of llvm/main.
If it depends on any parent revisions please document them as well.

[steakhal]

This patch preserves all previous reports as expected.
You can check it by yourself at https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D96090&items-per-page=50.

However, I still have some minor concerns inline.

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

101–103

I'm not sure if this FIXME is still applicable.

I'm also confused about having two functions doing effectively the same thing.
SimpleSValBuilder::dispatchCast is a virtual function, which just invokes a non-virtual function SValBuilder::evalCast.

Why should it be virtual in the first place?

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

727–728

You are resolving exactly this FIXME in this patch if I'm correct.
Shouldn't you update this comment?

867–869

Eh, I don't like comments preceding a single-statement block.
It might be a personal preference though.

[ASDenysPetrov]

In D96090#2568431, @steakhal wrote:

This patch preserves all previous reports as expected.

That's great results!

Could you please rebase this?

I'll update and rebase this patch soon.

If it depends on any parent revisions please document them as well.

It does. You can see this in the stack. Do I need to mention this somewhere else?

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

101–103

I want to make the patch smaller avoiding things that could be changed individually. We can remove dispatchCast in the next post-cleanup patch.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

727–728

I am not actually sure that the function I've made is good enough and is the single one. =)

867–869

I am trying to highlight the redirection to another cast function. If you think it's redundant, I'll remove it.

[steakhal]

In D96090#2572069, @ASDenysPetrov wrote:

In D96090#2568431, @steakhal wrote:

Could you please rebase this?

I'll update and rebase this patch soon.

If it depends on any parent revisions please document them as well.

It does. You can see this in the stack. Do I need to mention this somewhere else?

No, I probably missed that. My bad.

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

101–103

Seems reasonable to me.

[ASDenysPetrov]

Rebased revision on top of the main branch. Minor improvements.

[ASDenysPetrov]

@steakhal wrote:

This patch preserves all previous reports as expected.

If this patch is technically full-baked and does not bring any harm, can we approve it as well to move forward?

[steakhal]

In D96090#2574526, @ASDenysPetrov wrote:

@steakhal wrote:

This patch preserves all previous reports as expected.

Even with or without Z3 refutation.

If this patch is technically full-baked and does not bring any harm, can we approve it as well to move forward?

I would like @NoQ or someone else to also review this.

[ASDenysPetrov]

@steakhal

I would like @NoQ or someone else to also review this.

Oh, I would be very pleased if our colleagues pay attention to my recent patches. But seems you are the only who helps me with this stuff 🙂

[NoQ]

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

539

This is one of the most important APIs of SValBuilder, alongside evalBinOp(). The newly added behavior is definitely obscure and needs explaining. So i think you should move this comment into the header, turn it into a doxygen comment, and elaborate what "less accurately" actually means here.

[NoQ]

So, like, I think it's a start. You introduced a single source of truth for casting SVals and it's good. But i'm worried about our API surface.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

539

Like, are you sure that the new behavior makes sense at all for call sites that aren't ex-CastRetrievedVal? When, and why, would a regular user use this functionality?

Maybe instead of obfuscating this functionality as a null pointer we should add an explicit flag and give it an understandable name? Even if it's bool CastRetrievedValBackdoorHackDoNotUse = false it's still much better than "umm we kind of expect it to work if you pass null here but we don't really know how".

564

No, don't add default:. All possible cases are currently handled. It breaks compiler warning about not all cases handled in the switch when new enum cases are added. Compile-time warnings are better than run-time warnings.

[ASDenysPetrov]

Rebased on main.

[ASDenysPetrov]

@NoQ
Thanks, I could finally draw your attention :)

When, and why, would a regular user use this functionality?

IMO there is no reason to use evalCast without an original type. But I'm not sure that you can get an original type in every case.

About CastRetrievedValBackdoorHackDoNotUse I need a strong advise or some discussion. Look.
Initially evalCast deduces a new SVal using original QualType and cast QualType in case when we've got a state and expression and are able to get the original type. E.g.:

...

SVal OldV = Context.getSVal(Expression);
QualType OriginalTy = Expr->getType();
SVal NewV = SVB.evalCast(OldV, CastTy, Expr->getType());
...

But dispatchCast, evalCastFromNonLoc, evalCastFromLoc operate using cast QualType only without knowing the original one.

SVal OldV = Context.getSVal(Expression);
SVal NewV = SVB.evalCastFromLoc(OldV.castAs<Loc>, CastTy);

Actually many cases don't need to know an exact original type. Some cases can extract the type from SVal (SymbolVal, ConcreteInt) Other cases need it from outside (MemRegionVal, LocAsInteger).
dispatchCast, evalCastFromNonLoc, evalCastFromLoc have a lot of similar code which is already in evalCast. They also calls inside evalCast function. I decided to move their functionality into a new splitted version of evalCast to decrease complexity of comprehension and try to substitute them in the future with the single evalCast. But substitusion needs to deal something with absent original type parameter. Then I decided to add support for`evalCast` to work in both modes. Practically, new evaCast has additional checks and casts when the original type is not null.

Now evalCastFromNonLoc and evalCastFromLoc are used in SimpleSValBuilder functions only. I think it would be better to add an explaination to the doc like OriginalTy.isNull() is a CastRetrievedVal backdoor hack. Do not use it. instead of new flags. I'll make a better description, but at first I'd try to investigate how to pass an original type to evalCastFromNonLoc and evalCastFromLoc in SimpleSValBuilder functions. Or maybe some other solution.

In sum I'll try to handle this and provide a better solution.

[steakhal]

In D96090#2607387, @ASDenysPetrov wrote:

Actually many cases don't need to know an exact original type. Some cases can extract the type from SVal (SymbolVal, ConcreteInt) Other cases need it from outside (MemRegionVal, LocAsInteger).
dispatchCast, evalCastFromNonLoc, evalCastFromLoc have a lot of similar code which is already in evalCast. They also calls inside evalCast function. I decided to move their functionality into a new splitted version of evalCast to decrease complexity of comprehension and try to substitute them in the future with the single evalCast. But substitusion needs to deal something with absent original type parameter. Then I decided to add support for`evalCast` to work in both modes. Practically, new evaCast has additional checks and casts when the original type is not null.

Seems reasonable to me.

Now evalCastFromNonLoc and evalCastFromLoc are used in SimpleSValBuilder functions only. I think it would be better to add an explaination to the doc like OriginalTy.isNull() is a CastRetrievedVal backdoor hack. Do not use it. instead of new flags. I'll make a better description, but at first I'd try to investigate how to pass an original type to evalCastFromNonLoc and evalCastFromLoc in SimpleSValBuilder functions. Or maybe some other solution.

What about not defaulting that parameter? That would better represent our expectation that it requires an original-type.
You could still pass a default constructed QualType at each callsite.

[ASDenysPetrov]

In D96090#2616666, @steakhal wrote:

You could still pass a default constructed QualType at each callsite.

We can try. At least it will be like this is a hack for the particular cases then, that will emphasis to pay attention to. Well, I'll present this version tomorrow.

[NoQ]

Actually many cases don't need to know an exact original type.

In all cases we're supposed to have an original type, whether we need it or not. Simply because we're simulating a typed language. If we don't have it it's a bug.

Some cases can extract the type from SVal

Which is generally impossible because integral casts aren't modeled correctly. Which, again, is a bug. I don't know whether we'll have a need to pass the type separately when all other parts of the static analyzer become type-correct; I suspect that we won't need it. But one way or another, my point is, one or more of the following is true: "we have the ability to provide the type in all cases", "we don't need to provide the type in any of the cases". None of these ideal situations leave room for an API that receives the type optionally. For this reason I suggest to either always have the caller supply the original type (even if means extra work on the caller side) or never have it supply the original type.

[ASDenysPetrov]

Updated.

In D96090#2618410, @NoQ wrote:

In all cases we're supposed to have an original type, whether we need it or not. Simply because we're simulating a typed language. If we don't have it it's a bug.

I agree. That's how evalCastFromNonLoc and evalCastFromLoc work now. Finally I want to replace them along with passing a correct original type. This is just one another step closer to this direction. I don't want patches be too complicated.

Some cases can extract the type from SVal

Which is generally impossible because integral casts aren't modeled correctly.

Maybe I'm missing smth, But how about this:

QualType T = V.castAs<nonloc::SymbolVal>().getSymbol()->getType();
QualType T = cast<SymbolicRegion>(V.castAs<loc::MemRegionVal>().getAsRegion()).getSymbol()->getType();
// Belowees are not exact types but well narrowed information which can be sufficient.
V.castAs<nonloc::ConcreteInt>() <-> OriginalTy->isIntegralOrEnumerationType() 
V.castAs<loc::ConcreteInt>() | V.castAs<loc::MemRegionVal>() | V.castAs<loc::GotoLabel>() <-> Loc::isLocType(CastTy)
V.castAs<nonloc::PointerToMemberData>() <-> OriginalTy->isMemberPointerType()  (I guess)
...

None of these ideal situations leave room for an API that receives the type optionally.

I've returned back OriginalTy param from a default to a regular one as @steakhal advised in D96090#2616666.

[ASDenysPetrov]

Just a ping for you, guys, to let me move in this direction.

[steakhal]

I don't know. I think it's already way better than it was.
I think we can reiterate this later.

I want to get the llvm_unreachable("Unknown SVal kind") statements back. Besides those, I have no objections to this patch.
It preserves the old semantics, so it should be safe to land.
What do you think @NoQ?

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

565–566

You probably don't want to remove this. The same applies to similar ones.

[ASDenysPetrov]

@steakhal thanks for the approval.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

565–566

Yep. I'd keep it here, beacuse of compiler warnings of no-return function.

[NoQ]

This looks amazing, thanks a lot.

I have one final question: can we already remove dispatchCast()&Co.? I don't see any other call sites anymore. Looks like it becomes dead code after your patch.

Which is fairly shocking and mind blowing that we've organically developed two independent implementations of casting, one for RegionStore and one for everything else. I'm super happy that we're cleaning this up.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

565–566

compiler warnings of no-return function.

Aha ok, in this case it's much better to keep llvm_unreachable outside the switch, like here and unlike the one in evalCastKind functions. This way we're getting compile-time warnings for unhandled enum cases.

[NoQ]

Which is fairly shocking and mind blowing that we've organically developed two independent implementations of casting, one for RegionStore and one for everything else.

Wait, no, nvm, please disregard this. It wasn't like this forever, i just happened to catch code in an intermediate state after D90157. Either way, it's definitely getting much better, and either way, i'm curious if dispatchCast can now be eliminated.

[ASDenysPetrov]

@NoQ
Many thanks for your evaluation!

i'm curious if dispatchCast can now be eliminated.

Already approved a month ago D97277 :-) That's the next step.

[ASDenysPetrov]

Already approved a month ago D97277 :-) That's the next step.

In addition to the above:

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

68

@NoQ
I've mentioned the need in elimination.

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

protected:
   SVal evalCastSubKind(nonloc::LocAsInteger V, QualType CastTy,
                        QualType OriginalTy);
   SVal evalCastSubKind(nonloc::SymbolVal V, QualType CastTy,
                        QualType OriginalTy);
   SVal evalCastSubKind(nonloc::PointerToMember V, QualType CastTy,
                        QualType OriginalTy);
 
 public:
   // FIXME: Make these protected again once RegionStoreManager correctly
   // handles loads from different bound value types.
   virtual SVal dispatchCast(SVal val, QualType castTy) = 0;
 
 public:
   SValBuilder(llvm::BumpPtrAllocator &alloc, ASTContext &context,
               ProgramStateManager &stateMgr)
       : Context(context), BasicVals(context, alloc),
         SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
         StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
         ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
 
   virtual ~SValBuilder() = default;
 
   bool haveSameType(const SymExpr *Sym1, const SymExpr *Sym2) {
     return haveSameType(Sym1->getType(), Sym2->getType());
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
     // FIXME: Remove the second disjunct when we support symbolic
     // truncation/extension.
     return (Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2) ||
             (Ty1->isIntegralOrEnumerationType() &&
              Ty2->isIntegralOrEnumerationType()));
   }
 
-  SVal evalCast(SVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCast(SVal V, QualType CastTy, QualType OriginalTy = QualType{});
 
   // Handles casts of type CK_IntegralCast.
   SVal evalIntegralCast(ProgramStateRef state, SVal val, QualType castTy,
                         QualType originalType);
 
   virtual SVal evalMinus(NonLoc val) = 0;
 
   virtual SVal evalComplement(NonLoc val) = 0;

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

public:
   /// iterBindings - Iterate over the bindings in the Store.
   virtual void iterBindings(Store store, BindingsHandler& f) = 0;
 
 protected:
   const ElementRegion *MakeElementRegion(const SubRegion *baseRegion,
                                          QualType pointeeTy,
                                          uint64_t index = 0);
 
-  /// CastRetrievedVal - Used by subclasses of StoreManager to implement
-  ///  implicit casts that arise from loads from regions that are reinterpreted
-  ///  as another region.
-  SVal CastRetrievedVal(SVal val, const TypedValueRegion *region,
-                        QualType castTy);
-
 private:
   SVal getLValueFieldOrIvar(const Decl *decl, SVal base);
 };
 
 inline StoreRef::StoreRef(Store store, StoreManager & smgr)
     : store(store), mgr(smgr) {
   if (store)
     mgr.incrementReferenceCount(store);

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

else
       return UnknownVal();
   }
 
   // FIXME: handle Vector types.
   if (RTy->isVectorType())
     return UnknownVal();
 
   if (const FieldRegion* FR = dyn_cast<FieldRegion>(R))
-    return CastRetrievedVal(getBindingForField(B, FR), FR, T);
+    return svalBuilder.evalCast(getBindingForField(B, FR), T);
 
   if (const ElementRegion* ER = dyn_cast<ElementRegion>(R)) {
     // FIXME: Here we actually perform an implicit conversion from the loaded
     // value to the element type.  Eventually we want to compose these values
     // more intelligently.  For example, an 'element' can encompass multiple
     // bound regions (e.g., several bound bytes), or could be a subset of
     // a larger value.
-    return CastRetrievedVal(getBindingForElement(B, ER), ER, T);
+    return svalBuilder.evalCast(getBindingForElement(B, ER), T);
   }
 
   if (const ObjCIvarRegion *IVR = dyn_cast<ObjCIvarRegion>(R)) {
     // FIXME: Here we actually perform an implicit conversion from the loaded
     // value to the ivar type.  What we should model is stores to ivars
     // that blow past the extent of the ivar.  If the address of the ivar is
     // reinterpretted, it is possible we stored a different value that could
     // fit within the ivar.  Either we need to cast these when storing them
     // or reinterpret them lazily (as we do here).
-    return CastRetrievedVal(getBindingForObjCIvar(B, IVR), IVR, T);
+    return svalBuilder.evalCast(getBindingForObjCIvar(B, IVR), T);
   }
 
   if (const VarRegion *VR = dyn_cast<VarRegion>(R)) {
     // FIXME: Here we actually perform an implicit conversion from the loaded
     // value to the variable type.  What we should model is stores to variables
     // that blow past the extent of the variable.  If the address of the
     // variable is reinterpretted, it is possible we stored a different value
     // that could fit within the variable.  Either we need to cast these when
     // storing them or reinterpret them lazily (as we do here).
-    return CastRetrievedVal(getBindingForVar(B, VR), VR, T);
+    return svalBuilder.evalCast(getBindingForVar(B, VR), T);
   }
 
   const SVal *V = B.lookup(R, BindingKey::Direct);
 
   // Check if the region has a binding.
   if (V)
     return *V;
 

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

 }
 
 //===----------------------------------------------------------------------===//
 // Cast methods.
 // `evalCast` is the main method
 // `evalCastKind` and `evalCastSubKind` are helpers
 //===----------------------------------------------------------------------===//
 
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
+                           QualType OriginalTy /*= QualType{}*/) {
+  if (CastTy.isNull())
+    return V;
+
   CastTy = Context.getCanonicalType(CastTy);
+
+  if (!OriginalTy.isNull()) {
     OriginalTy = Context.getCanonicalType(OriginalTy);
+
     if (CastTy == OriginalTy)
       return V;
 
-  // FIXME: Move this check to the most appropriate evalCastKind/evalCastSubKind
-  // function.
-  // For const casts, casts to void, just propagate the value.
+    // FIXME: Move this check to the most appropriate
+    // evalCastKind/evalCastSubKind function. For const casts, casts to void,
+    // just propagate the value.
     if (!CastTy->isVariableArrayType() && !OriginalTy->isVariableArrayType())
       if (shouldBeModeledWithNoOp(Context, Context.getPointerType(CastTy),
                                   Context.getPointerType(OriginalTy)))
         return V;
+  }
 
   // Cast SVal according to kinds.
   switch (V.getBaseKind()) {
+  default:
+    llvm_unreachable("Unknown SVal kind");
   case SVal::UndefinedValKind:
     return evalCastKind(V.castAs<UndefinedVal>(), CastTy, OriginalTy);
   case SVal::UnknownValKind:
     return evalCastKind(V.castAs<UnknownVal>(), CastTy, OriginalTy);
   case SVal::LocKind:
     return evalCastKind(V.castAs<Loc>(), CastTy, OriginalTy);
   case SVal::NonLocKind:
     return evalCastKind(V.castAs<NonLoc>(), CastTy, OriginalTy);
-  default:
-    llvm_unreachable("Unknown SVal kind");
   }
 }
 
 SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
                                QualType OriginalTy) {
   return V;
 }
 
 SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
                                QualType OriginalTy) {
   return V;
 }
 
 SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
   switch (V.getSubKind()) {
+  default:
+    llvm_unreachable("Unknown SVal kind");
   case loc::ConcreteIntKind:
     return evalCastSubKind(V.castAs<loc::ConcreteInt>(), CastTy, OriginalTy);
   case loc::GotoLabelKind:
     return evalCastSubKind(V.castAs<loc::GotoLabel>(), CastTy, OriginalTy);
   case loc::MemRegionValKind:
     return evalCastSubKind(V.castAs<loc::MemRegionVal>(), CastTy, OriginalTy);
-  default:
-    llvm_unreachable("Unknown SVal kind");
   }
 }
 
 SVal SValBuilder::evalCastKind(NonLoc V, QualType CastTy, QualType OriginalTy) {
   switch (V.getSubKind()) {
+  default:
+    llvm_unreachable("Unknown SVal kind");
   case nonloc::CompoundValKind:
     return evalCastSubKind(V.castAs<nonloc::CompoundVal>(), CastTy, OriginalTy);
   case nonloc::ConcreteIntKind:
     return evalCastSubKind(V.castAs<nonloc::ConcreteInt>(), CastTy, OriginalTy);
   case nonloc::LazyCompoundValKind:
     return evalCastSubKind(V.castAs<nonloc::LazyCompoundVal>(), CastTy,
                            OriginalTy);
   case nonloc::LocAsIntegerKind:
     return evalCastSubKind(V.castAs<nonloc::LocAsInteger>(), CastTy,
                            OriginalTy);
   case nonloc::SymbolValKind:
     return evalCastSubKind(V.castAs<nonloc::SymbolVal>(), CastTy, OriginalTy);
   case nonloc::PointerToMemberKind:
     return evalCastSubKind(V.castAs<nonloc::PointerToMember>(), CastTy,
                            OriginalTy);
-  default:
-    llvm_unreachable("Unknown SVal kind");
   }
 }
 
 SVal SValBuilder::evalCastSubKind(loc::ConcreteInt V, QualType CastTy,
                                   QualType OriginalTy) {
   // Pointer to bool.
   if (CastTy->isBooleanType())
     return makeTruthVal(V.getValue().getBoolValue(), CastTy);
if (CastTy->isBooleanType())
     return makeTruthVal(true, CastTy);
 
   // Pointer to integer.
   if (CastTy->isIntegralOrEnumerationType()) {
     const unsigned BitWidth = Context.getIntWidth(CastTy);
     return makeLocAsInteger(V, BitWidth);
   }
 
+  if (!OriginalTy.isNull()) {
     // Array to pointer.
     if (isa<ArrayType>(OriginalTy))
       if (CastTy->isPointerType() || CastTy->isReferenceType())
         return UnknownVal();
+  }
 
   // Pointer to any pointer.
   if (Loc::isLocType(CastTy))
     return V;
 
   // Pointer to whatever else.
   return UnknownVal();
 }
+static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
+  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
+         ty2->getPointeeType().getCanonicalType().getTypePtr();
+}
 
 SVal SValBuilder::evalCastSubKind(loc::MemRegionVal V, QualType CastTy,
                                   QualType OriginalTy) {
   // Pointer to bool.
   if (CastTy->isBooleanType()) {
     const MemRegion *R = V.getRegion();
     if (const FunctionCodeRegion *FTR = dyn_cast<FunctionCodeRegion>(R))
       if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(FTR->getDecl()))
         if (FD->isWeak())
           // FIXME: Currently we are using an extent symbol here,
           // because there are no generic region address metadata
           // symbols to use, only content metadata.
           return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
 
     if (const SymbolicRegion *SymR = R->getSymbolicBase())
       return makeNonLoc(SymR->getSymbol(), BO_NE,
                         BasicVals.getZeroWithPtrWidth(), CastTy);
     // Non-symbolic memory regions are always true.
     return makeTruthVal(true, CastTy);
   }
 
   // Try to cast to array
-  const auto *ArrayTy = dyn_cast<ArrayType>(OriginalTy.getCanonicalType());
+  const auto *ArrayTy =
+      OriginalTy.isNull() ? nullptr
+                          : dyn_cast<ArrayType>(OriginalTy.getCanonicalType());
 
   // Pointer to integer.
   if (CastTy->isIntegralOrEnumerationType()) {
     SVal Val = V;
     // Array to integer.
     if (ArrayTy) {
       // We will always decay to a pointer.
       QualType ElemTy = ArrayTy->getElementType();
       Val = StateMgr.ArrayToPointer(V, ElemTy);
       // FIXME: Keep these here for now in case we decide soon that we
       // need the original decayed type.
       //    QualType elemTy = cast<ArrayType>(originalTy)->getElementType();
       //    QualType pointerTy = C.getPointerType(elemTy);
     }
     const unsigned BitWidth = Context.getIntWidth(CastTy);
     return makeLocAsInteger(Val.castAs<Loc>(), BitWidth);
   }
 
   // Pointer to pointer.
   if (Loc::isLocType(CastTy)) {
+
+    if (OriginalTy.isNull()) {
+      // When retrieving symbolic pointer and expecting a non-void pointer,
+      // wrap them into element regions of the expected type if necessary.
+      // It is necessary to make sure that the retrieved value makes sense,
+      // because there's no other cast in the AST that would tell us to cast
+      // it to the correct pointer type. We might need to do that for non-void
+      // pointers as well.
+      // FIXME: We really need a single good function to perform casts for us
+      // correctly every time we need it.
+      if (CastTy->isPointerType() && !CastTy->isVoidPointerType()) {
+        const MemRegion *R = V.getRegion();
+        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);
+          }
+        }
+      }
+      return V;
+    }
+
     if (OriginalTy->isIntegralOrEnumerationType() ||
         OriginalTy->isBlockPointerType() || OriginalTy->isFunctionPointerType())
       return V;
 
     // Array to pointer.
     if (ArrayTy) {
       // Are we casting from an array to a pointer?  If so just pass on
       // the decayed value.
SVal SValBuilder::evalCastSubKind(nonloc::LocAsInteger V, QualType CastTy,
                                   QualType OriginalTy) {
   Loc L = V.getLoc();
 
   // Pointer as integer to bool.
   if (CastTy->isBooleanType())
     // Pass to Loc function.
     return evalCastKind(L, CastTy, OriginalTy);
 
-  if (Loc::isLocType(CastTy) && OriginalTy->isIntegralOrEnumerationType()) {
+  if (Loc::isLocType(CastTy) && !OriginalTy.isNull() &&
+      OriginalTy->isIntegralOrEnumerationType()) {
     if (const MemRegion *R = L.getAsRegion())
       if ((R = StateMgr.getStoreManager().castRegion(R, CastTy)))
         return loc::MemRegionVal(R);
     return L;
   }
 
   // Pointer as integer with region to integer/pointer.
-  if (const MemRegion *R = L.getAsRegion()) {
+  const MemRegion *R = L.getAsRegion();
+  if (!OriginalTy.isNull() && R) {
     if (CastTy->isIntegralOrEnumerationType())
-      // Pass to MemRegion function.
       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);
     }
   } else {
-    if (Loc::isLocType(CastTy))
+    if (Loc::isLocType(CastTy)) {
+      if (OriginalTy.isNull())
+        return evalCastSubKind(loc::MemRegionVal(R), CastTy, OriginalTy);
       return L;
+    }
 
+    SymbolRef SE = nullptr;
+    if (R) {
+      if (const SymbolicRegion *SR =
+              dyn_cast<SymbolicRegion>(R->StripCasts())) {
+        SE = SR->getSymbol();
+      }
+    }
+
+    if (!CastTy->isFloatingType() || !SE || SE->getType()->isFloatingType()) {
       // FIXME: Correctly support promotions/truncations.
       const unsigned CastSize = Context.getIntWidth(CastTy);
       if (CastSize == V.getNumBits())
         return V;
 
       return makeLocAsInteger(L, CastSize);
     }
+  }
 
   // Pointer as integer to whatever else.
   return UnknownVal();
 }
 
 SVal SValBuilder::evalCastSubKind(nonloc::SymbolVal V, QualType CastTy,
                                   QualType OriginalTy) {
   SymbolRef SE = V.getSymbol();
 
   // Symbol to bool.
-  if (CastTy->isBooleanType()) {
+  if (!OriginalTy.isNull() && CastTy->isBooleanType()) {
     // Non-float to bool.
     if (Loc::isLocType(OriginalTy) ||
         OriginalTy->isIntegralOrEnumerationType() ||
         OriginalTy->isMemberPointerType()) {
-      SymbolRef SE = V.getSymbol();
       BasicValueFactory &BVF = getBasicValueFactory();
       return makeNonLoc(SE, BO_NE, BVF.getValue(0, SE->getType()), CastTy);
     }
   } else {
     // Symbol to integer, float.
     QualType T = Context.getCanonicalType(SE->getType());
     // If types are the same or both are integers, ignore the cast.
     // FIXME: Remove this hack when we support symbolic truncation/extension.
     // HACK: If both castTy and T are integers, ignore the cast.  This is
     // not a permanent solution.  Eventually we want to precisely handle
     // extension/truncation of symbolic integers.  This prevents us from losing
     // precision when we assign 'x = y' and 'y' is symbolic and x and y are
     // different integer types.
     if (haveSameType(T, CastTy))
       return V;
     if (!Loc::isLocType(CastTy))
+      if (!OriginalTy.isNull() || !CastTy->isFloatingType() ||
+          T->isFloatingType())
         return makeNonLoc(SE, T, CastTy);
   }
 
   // Symbol to pointer and whatever else.
   return UnknownVal();
 }
 
 SVal SValBuilder::evalCastSubKind(nonloc::PointerToMember V, QualType CastTy,
                                   QualType OriginalTy) {
   // Member pointer to whatever.
   return V;
 }

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

SValBuilder *ento::createSimpleSValBuilder(llvm::BumpPtrAllocator &alloc,
                                            ProgramStateManager &stateMgr) {
   return new SimpleSValBuilder(alloc, context, stateMgr);
 }
 
 //===----------------------------------------------------------------------===//
 // Transfer function for Casts.
 //===----------------------------------------------------------------------===//
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::dispatchCast(SVal Val, QualType CastTy) {
-  assert(Val.getAs<Loc>() || Val.getAs<NonLoc>());
-  return Val.getAs<Loc>() ? evalCastFromLoc(Val.castAs<Loc>(), CastTy)
-                           : evalCastFromNonLoc(Val.castAs<NonLoc>(), CastTy);
+  return evalCast(Val, CastTy);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
   bool isLocType = Loc::isLocType(castTy);
   if (val.getAs<nonloc::PointerToMember>())
     return val;
 
   if (Optional<nonloc::LocAsInteger> LI = val.getAs<nonloc::LocAsInteger>()) {
     if (isLocType)
       return LI->getLoc();
SVal SimpleSValBuilder::evalCastFromNonLoc(NonLoc val, QualType castTy) {
   BasicVals.getAPSIntType(castTy).apply(i);
 
   if (isLocType)
     return makeIntLocVal(i);
   else
     return makeIntVal(i);
 }
 
+// FIXME: This function should be eliminated and replaced with `evalCast`
 SVal SimpleSValBuilder::evalCastFromLoc(Loc val, QualType castTy) {
 
   // Casts from pointers -> pointers, just return the lval.
   //
   // Casts from pointers -> references, just return the lval.  These
   //   can be introduced by the frontend for corner cases, e.g
   //   casting from va_list* to __builtin_va_list&.
   //

clang/lib/StaticAnalyzer/Core/Store.cpp

if (const auto *SR = dyn_cast<SymbolicRegion>(MR)) {
     return loc::MemRegionVal(GetElementZeroRegion(SR, TargetType));
   }
 
   // We failed if the region we ended up with has perfect type info.
   Failed = isa<TypedValueRegion>(MR);
   return UnknownVal();
 }
 
-static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
-  return ty1->getPointeeType().getCanonicalType().getTypePtr() ==
-         ty2->getPointeeType().getCanonicalType().getTypePtr();
-}
-
-/// CastRetrievedVal - Used by subclasses of StoreManager to implement
-///  implicit casts that arise from loads from regions that are reinterpreted
-///  as another region.
-SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
-                                    QualType castTy) {
-  if (castTy.isNull() || V.isUnknownOrUndef())
-    return V;
-
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-    SymbolRef Sym = V.getAsSymbol();
-    if (Sym && !Sym->getType()->isFloatingType())
-      return UnknownVal();
-  }
-
-  // When retrieving symbolic pointer and expecting a non-void pointer,
-  // wrap them into element regions of the expected type if necessary.
-  // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
-  // make sure that the retrieved value makes sense, because there's no other
-  // cast in the AST that would tell us to cast it to the correct pointer type.
-  // We might need to do that for non-void pointers as well.
-  // FIXME: We really need a single good function to perform casts for us
-  // correctly every time we need it.
-  if (castTy->isPointerType() && !castTy->isVoidPointerType())
-    if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(V.getAsRegion())) {
-      QualType sr = SR->getSymbol()->getType();
-      if (!hasSameUnqualifiedPointeeType(sr, castTy))
-          return loc::MemRegionVal(castRegion(SR, castTy));
-    }
-
-  return svalBuilder.dispatchCast(V, castTy);
-}
-
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
     return Base;
 
   Loc BaseL = Base.castAs<Loc>();
   const SubRegion* BaseR = nullptr;
 
   switch (BaseL.getSubKind()) {