[PATCH] D136603: [analyzer] Model cast after LValueToRValueBitCasts

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I am not sure, if the `ExprEngine::VisitCast` is the proper place to add these new modifications and call SValBuilder's evalCast. I think it might be better positioned in `RegionStoreManager::getBinding`. Considering, we already do a cast evaluation for certain kind of memregions there before returning with the stored value. (Actually, this is again a legacy hack, which is needed because we do not emit all SymbolCasts and thus we store the SVals with an improper type).

  if (const FieldRegion* FR = dyn_cast<FieldRegion>(R))
    return svalBuilder.evalCast(getBindingForField(B, FR), T, QualType{});
  
  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 svalBuilder.evalCast(getBindingForElement(B, ER), T, QualType{});
  }
  
  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 svalBuilder.evalCast(getBindingForObjCIvar(B, IVR), T, QualType{});
  }
  
  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 svalBuilder.evalCast(getBindingForVar(B, VR), T, QualType{});
  }

I think a new if block for `SymRegion` should be put here to continue the hack.



================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:301-315
+      ExplodedNodeSet Tmp;
+      evalLocation(Tmp, CastE, CastE, subExprNode, state, *Location, true);
+      if (Tmp.empty())
+        return;
+
+      // Proceed with the load.
+      StmtNodeBuilder Bldr(Tmp, Dst, *currBldrCtx);
----------------
I think it might be possible to refactor `evalLoad` by returning with a `tuple` or with a special `struct`. I guess you should return a vector of `SVal`s as one tuple member. Then you could reuse the function in both cases before calling `generateNode` on the result. But, this might be a code that is too complicated, I'll let you decide if this is worth the hassle to avoid the code repetition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136603