[PATCH] D115932: [Analyzer] Create and handle SymbolCast for pointer to integral conversion

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: steakhal, NoQ, ASDenysPetrov.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
martong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Currently, pointer to integral type casts are not modeled properly in
the symbol tree.
Below

  void foo(int *p) {
    12 - (long)p;
  }

the resulting symbol for the BinOP is an IntSymExpr. LHS is `12` and the RHS is
`&SymRegion{reg_$0<int * p>}`.

Even though, the `SVal` that is created for `(long)p` is correctly an
`LocAsInteger`, we loose this information when we build up the symbol tree for
`12 - (long)p`. To preserve the cast, we have to create a new node in
the symbol tree that represents it: the `SymbolCast`. This
is what this patch does.

This reverts D115149 <https://reviews.llvm.org/D115149>, except the test cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115932

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp


Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1204,7 +1204,23 @@
       return SVB.makeSymbolVal(S);
     }
 
-    // TODO: Support SymbolCast.
+    SVal VisitSymbolCast(const SymbolCast *Cast) {
+      SVal Castee = Visit(Cast->getOperand());
+      const auto &Context = SVB.getContext();
+      QualType CastTy = Cast->getType();
+
+      // Cast a pointer to an integral type.
+      if (Castee.getAs<Loc>() && !Loc::isLocType(CastTy)) {
+        const unsigned BitWidth = Context.getIntWidth(CastTy);
+        if (Castee.getAsSymbol())
+          return SVB.makeLocAsInteger(Castee.castAs<Loc>(), BitWidth);
+        if (auto X = Castee.getAs<loc::ConcreteInt>())
+          return SVB.makeIntVal(X->getValue());
+        // FIXME other cases?
+      }
+
+      return Base::VisitSymbolCast(Cast);
+    }
 
     SVal VisitSymIntExpr(const SymIntExpr *S) {
       auto I = Cached.find(S);
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -412,6 +412,15 @@
   SymbolRef symLHS = LHS.getAsSymbol();
   SymbolRef symRHS = RHS.getAsSymbol();
 
+  // FIXME This should be done in getAsSymbol. But then getAsSymbol should be
+  // the member function of SValBuilder (?)
+  if (symRHS)
+    if (auto RLocAsInt = RHS.getAs<nonloc::LocAsInteger>()) {
+      auto FromTy = symRHS->getType();
+      auto ToTy = RLocAsInt->getType(this->Context);
+      symRHS = this->getSymbolManager().getCastSymbol(symRHS, FromTy, ToTy);
+    }
+
   // TODO: When the Max Complexity is reached, we should conjure a symbol
   // instead of generating an Unknown value and propagate the taint info to it.
   const unsigned MaxComp = AnOpts.MaxSymbolComplexity;
@@ -459,23 +468,14 @@
     return evalBinOpLN(state, op, *LV, rhs.castAs<NonLoc>(), type);
   }
 
-  if (const Optional<Loc> RV = rhs.getAs<Loc>()) {
-    const auto IsCommutative = [](BinaryOperatorKind Op) {
-      return Op == BO_Mul || Op == BO_Add || Op == BO_And || Op == BO_Xor ||
-             Op == BO_Or;
-    };
+  if (Optional<Loc> RV = rhs.getAs<Loc>()) {
+    // Support pointer arithmetic where the addend is on the left
+    // and the pointer on the right.
 
-    if (IsCommutative(op)) {
-      // Swap operands.
-      return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
-    }
+    assert(op == BO_Add);
 
-    // If the right operand is a concrete int location then we have nothing
-    // better but to treat it as a simple nonloc.
-    if (auto RV = rhs.getAs<loc::ConcreteInt>()) {
-      const nonloc::ConcreteInt RhsAsLoc = makeIntVal(RV->getValue());
-      return evalBinOpNN(state, op, lhs.castAs<NonLoc>(), RhsAsLoc, type);
-    }
+    // Commute the operands.
+    return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
   }
 
   return evalBinOpNN(state, op, lhs.castAs<NonLoc>(), rhs.castAs<NonLoc>(),
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
@@ -141,6 +141,7 @@
   using SValVisitor<ImplClass, RetTy>::Visit;
   using SymExprVisitor<ImplClass, RetTy>::Visit;
   using MemRegionVisitor<ImplClass, RetTy>::Visit;
+  using Base = FullSValVisitor;
 };
 
 } // end namespace ento


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115932.395095.patch
Type: text/x-patch
Size: 3687 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211217/f9a265a1/attachment.bin>