[analyzer] Fix assertion failure in CXXInstanceCall::getCXXThisVal

[steakhal]

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 SymExprs, as stated:

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

3. We can't use ElementRegions 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

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)

Changes

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.

I've considered using SVB.evalCast(ThisVal, Base->getType(), QualType()), but it doesn't work as 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 SymExprs, as stated:

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

3. We can't use ElementRegions 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 returned Unknown xD IMO this is still better than having a crash.

Fixes #69922

---

Full diff: https://github.com/llvm/llvm-project/pull/70837.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/CallEvent.cpp (+2-3)
• (modified) clang/test/Analysis/builtin_bitcast.cpp (+21)

diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index ad5bb66c4fff3c8..20bc64dc2631cec 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,10 +715,9 @@ void CXXInstanceCall::getExtraInvalidatedValues(
 SVal CXXInstanceCall::getCXXThisVal() const {
   const Expr *Base = getCXXThisExpr();
   // FIXME: This doesn't handle an overloaded ->* operator.
-  if (!Base)
+  SVal ThisVal = Base ? getSVal(Base) : UnknownVal();
+  if (isa<NonLoc>(ThisVal))
     return UnknownVal();
-
-  SVal ThisVal = getSVal(Base);
   assert(ThisVal.isUnknownOrUndef() || isa<Loc>(ThisVal));
   return ThisVal;
 }
diff --git a/clang/test/Analysis/builtin_bitcast.cpp b/clang/test/Analysis/builtin_bitcast.cpp
index 396e7caa45f6acd..13475694d287a22 100644
--- a/clang/test/Analysis/builtin_bitcast.cpp
+++ b/clang/test/Analysis/builtin_bitcast.cpp
@@ -30,3 +30,24 @@ void test(int i) {
   clang_analyzer_dump(g4);
   // expected-warning@-1 {{&i [as 64 bit integer]}}
 }
+
+struct A {
+  int n;
+  void set(int x) {
+    n = x;
+  }
+};
+using ptr_size = decltype(sizeof(void *));
+void gh_69922(ptr_size p) {
+  // expected-warning-re@+1 {{(reg_${{[0-9]+}}<ptr_size 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@-1 {{(reg_${{[0-9]+}}<ptr_size p>) & 1U}}
+}

[Xazax-hun]

Hmm, I wonder if we should leave a FIXME comment, but it looks good to me.

[steakhal]

Hmm, I wonder if we should leave a FIXME comment, but it looks good to me.

I was thinking where to put the FIXME, and as I explored that should be within the CastVisitor. After that, I argued, that then I should still have the (ineffective) SVB.evalCast() to actually exercise that path.
It should be a fancy way of returning Unknown now, but arguably for the better reason xD

@Xazax-hun WDYT?

[Xazax-hun]

WDYT?

I like this! I hope we do not add too much redundant work, but at least we make it clear what is the plan to fix this in the future.

[steakhal]

WDYT?

I like this! I hope we do not add too much redundant work, but at least we make it clear what is the plan to fix this in the future.

Please approve the PR again, so that I could merge this after I give some time for others to look at this.

[NagyDonat]

Basically LGTM, thanks for fixing this crash.

I don't give a formal approval because I didn't independently verify the correctness of the details, but I like the direction of the change.