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

[Balázs Benics via Phabricator via cfe-commits]

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

Casting a pointer to a suitably large integral type by reinterpret-cast
should result in the same value as by using the `__builtin_bit_cast()`.
The compiler exploits this: https://godbolt.org/z/zMP3sG683

However, the analyzer does not bind the same symbolic value to these
expressions, resulting in weird situations, such as failing equality
checks and even results in crashes: https://godbolt.org/z/oeMP7cj8q

Previously, the `&SymRegion` were not cast to `NonLoc`, which later in
the `evalBinOpLN()` caused a crash when the engine wanted to calculate
the offset at the BinaryOperator expression.
So, in this patch, I'm proposing to add an explicit cast when evaluating
an `LValueToRValueBitCast` cast expression. Other than casting the
resulting value to the given type before binding it to the expression it
does the same thing that we do for `LValueToRValue` casts.

Here is a snippet of code, annotated by the previous and new dump values.
`LocAsInteger` wraps the `SymRegion` in only two cases:

  void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
    clang_analyzer_dump(p);     // remained: &SymRegion{reg_$0<void * p>}
    clang_analyzer_dump(array); // remained: {{&SymRegion{reg_$1<char (*)[8] array>}
    clang_analyzer_dump((unsigned long)p);
    // remained: {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
    clang_analyzer_dump(__builtin_bit_cast(unsigned long, p));     <--------- change #1
    // previously: {{&SymRegion{reg_$0<void * p>}}}
    // now:        {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
    clang_analyzer_dump((unsigned long)array); // remained: {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
    clang_analyzer_dump(__builtin_bit_cast(unsigned long, array)); <--------- change #2
    // previously: {{&SymRegion{reg_$1<char (*)[8] array>}}}
    // now:        {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
  }

PS: I'm not sure how/when we should get rid of the `LocAsInteger` and
represent this by a `SymbolCast`.
Maybe @ASDenysPetrov or @martong could help me review this.

This is a follow-up of D105017 <https://reviews.llvm.org/D105017>, where I thought it should be enough to
model `LValueToRValueBitCast` the same way as we do with `LValueToRValueCast`.
I still think the same, but we really should have some `NonLoc` value here, IMO.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136603

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/ptr-arith.cpp


Index: clang/test/Analysis/ptr-arith.cpp
===================================================================
--- clang/test/Analysis/ptr-arith.cpp
+++ clang/test/Analysis/ptr-arith.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -Wno-unused-value -std=c++14 -analyzer-checker=core,debug.ExprInspection,alpha.core.PointerArithm -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused-value -std=c++14 -verify %s -triple x86_64-pc-linux-gnu \
+// RUN:    -analyzer-checker=core,debug.ExprInspection,alpha.core.PointerArithm
 
 template <typename T> void clang_analyzer_dump(T);
 
@@ -141,3 +142,22 @@
   return bits->b; // no-warning
 }
 } // namespace Bug_55934
+
+void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
+  clang_analyzer_dump(p);
+  clang_analyzer_dump(array);
+  // expected-warning at -2 {{&SymRegion{reg_$0<void * p>}}}
+  // expected-warning at -2 {{&SymRegion{reg_$1<char (*)[8] array>}}}
+  clang_analyzer_dump((unsigned long)p);
+  clang_analyzer_dump(__builtin_bit_cast(unsigned long, p));
+  // expected-warning at -2 {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
+  // expected-warning at -2 {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
+  clang_analyzer_dump((unsigned long)array);
+  clang_analyzer_dump(__builtin_bit_cast(unsigned long, array));
+  // expected-warning at -2 {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
+  // expected-warning at -2 {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
+}
+
+unsigned long ptr_arithmetic(void *p) {
+  return __builtin_bit_cast(unsigned long, p) + 1; // no-crash
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -288,8 +288,36 @@
   ExplodedNodeSet dstPreStmt;
   getCheckerManager().runCheckersForPreStmt(dstPreStmt, Pred, CastE, *this);
 
-  if (CastE->getCastKind() == CK_LValueToRValue ||
-      CastE->getCastKind() == CK_LValueToRValueBitCast) {
+  if (CastE->getCastKind() == CK_LValueToRValueBitCast) {
+    // Do the same as for 'CK_LValueToRValue' but cast the resulting value to
+    // the appropriate type before binding.
+    for (ExplodedNode *subExprNode : dstPreStmt) {
+      ProgramStateRef state = subExprNode->getState();
+      const LocationContext *LCtx = subExprNode->getLocationContext();
+      Optional<Loc> Location = state->getSVal(Ex, LCtx).getAs<Loc>();
+      if (!Location)
+        return;
+
+      ExplodedNodeSet Tmp;
+      evalLocation(Tmp, CastE, CastE, subExprNode, state, *Location, true);
+      if (Tmp.empty())
+        return;
+
+      // Proceed with the load.
+      StmtNodeBuilder Bldr(Tmp, Dst, *currBldrCtx);
+      for (ExplodedNode *I : Tmp) {
+        state = I->getState();
+
+        SVal V = state->getSVal(*Location, CastE->getType());
+        SVal CastV = getSValBuilder().evalCast(V, CastE->getType(), QualType{});
+        Bldr.generateNode(CastE, I, state->BindExpr(CastE, LCtx, CastV),
+                          nullptr, ProgramPoint::PostLoadKind);
+      }
+    }
+    return;
+  }
+
+  if (CastE->getCastKind() == CK_LValueToRValue) {
     for (ExplodedNodeSet::iterator I = dstPreStmt.begin(), E = dstPreStmt.end();
          I!=E; ++I) {
       ExplodedNode *subExprNode = *I;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D136603.470147.patch
Type: text/x-patch
Size: 3329 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221024/5dbbed97/attachment.bin>