[clang] 51d15d1 - [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (#70837)

[via cfe-commits]

Author: Balazs Benics
Date: 2023-11-04T11:11:24+01:00
New Revision: 51d15d13dea4325d1f76353af847d9de0b532e87

URL: https://github.com/llvm/llvm-project/commit/51d15d13dea4325d1f76353af847d9de0b532e87
DIFF: https://github.com/llvm/llvm-project/commit/51d15d13dea4325d1f76353af847d9de0b532e87.diff

LOG: [analyzer] Fix assertion failure in `CXXInstanceCall::getCXXThisVal` (#70837)

Workaround the case when the `this` pointer is actually a `NonLoc`, by
returning `Unknown` instead.
The solution isn't ideal, as `this` should be really a `Loc`, but due to
how casts work, I feel this is our easiest and best option.

As this patch presents, I'm evaluating a cast to transform the `NonLoc`.
However, given that `evalCast()` can't be cast from `NonLoc` to a
pointer type thingy (`Loc`), we end up with `Unknown`.
It is because `EvalCastVisitor::VisitNonLocSymbolVal()` only evaluates
casts that happen from NonLoc to NonLocs.

When I tried to actually implement that case, I figured:
1) Create a `SymbolicRegion` from that `nonloc::SymbolVal`; but
`SymbolRegion` ctor expects a pointer type for the symbol.
2) Okay, just have a `SymbolCast`, getting us the pointer type; but
`SymbolRegion` expects `SymbolData` symbols, not generic `SymExpr`s, as
stated:

> // Because pointer arithmetic is represented by ElementRegion layers,
> // the base symbol here should not contain any arithmetic.

3) We can't use `ElementRegion`s to perform this cast because to have an
`ElementRegion`, you already have to have a `SubRegion` that you want to
cast, but the point is that we don't have that.

At this point, I gave up, and just left a FIXME instead, while still
returning `Unknown` on that path.
IMO this is still better than having a crash.

Fixes #69922

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Core/CallEvent.cpp
    clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
    clang/test/Analysis/builtin_bitcast.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index ad5bb66c4fff3c8..76fb7481f65194b 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,10 +715,14 @@ void CXXInstanceCall::getExtraInvalidatedValues(
 SVal CXXInstanceCall::getCXXThisVal() const {
   const Expr *Base = getCXXThisExpr();
   // FIXME: This doesn't handle an overloaded ->* operator.
-  if (!Base)
-    return UnknownVal();
+  SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
+
+  if (isa<NonLoc>(ThisVal)) {
+    SValBuilder &SVB = getState()->getStateManager().getSValBuilder();
+    QualType OriginalTy = ThisVal.getType(SVB.getContext());
+    return SVB.evalCast(ThisVal, Base->getType(), OriginalTy);
+  }
 
-  SVal ThisVal = getSVal(Base);
   assert(ThisVal.isUnknownOrUndef() || isa<Loc>(ThisVal));
   return ThisVal;
 }

diff  --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index d89d82626f72694..9375f39b2d71dd4 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -980,6 +980,11 @@ class EvalCastVisitor : public SValVisitor<EvalCastVisitor, SVal> {
           return VB.makeNonLoc(SE, T, CastTy);
     }
 
+    // FIXME: We should be able to cast NonLoc -> Loc
+    // (when Loc::isLocType(CastTy) is true)
+    // But it's hard to do as SymbolicRegions can't refer to SymbolCasts holding
+    // generic SymExprs. Check the commit message for the details.
+
     // Symbol to pointer and whatever else.
     return UnknownVal();
   }

diff  --git a/clang/test/Analysis/builtin_bitcast.cpp b/clang/test/Analysis/builtin_bitcast.cpp
index 396e7caa45f6acd..5a0d9e7189b8edd 100644
--- a/clang/test/Analysis/builtin_bitcast.cpp
+++ b/clang/test/Analysis/builtin_bitcast.cpp
@@ -2,6 +2,7 @@
 // RUN:   -analyzer-checker=core,debug.ExprInspection
 
 template <typename T> void clang_analyzer_dump(T);
+using size_t = decltype(sizeof(int));
 
 __attribute__((always_inline)) static inline constexpr unsigned int _castf32_u32(float __A) {
   return __builtin_bit_cast(unsigned int, __A); // no-warning
@@ -30,3 +31,23 @@ void test(int i) {
   clang_analyzer_dump(g4);
   // expected-warning at -1 {{&i [as 64 bit integer]}}
 }
+
+struct A {
+  int n;
+  void set(int x) {
+    n = x;
+  }
+};
+void gh_69922(size_t p) {
+  // expected-warning-re at +1 {{(reg_${{[0-9]+}}<size_t p>) & 1U}}
+  clang_analyzer_dump(__builtin_bit_cast(A*, p & 1));
+
+  __builtin_bit_cast(A*, p & 1)->set(2); // no-crash
+  // However, since the `this` pointer is expected to be a Loc, but we have
+  // NonLoc there, we simply give up and resolve it as `Unknown`.
+  // Then, inside the `set()` member function call we can't evaluate the
+  // store to the member variable `n`.
+
+  clang_analyzer_dump(__builtin_bit_cast(A*, p & 1)->n); // Ideally, this should print "2".
+  // expected-warning-re at -1 {{(reg_${{[0-9]+}}<size_t p>) & 1U}}
+}