[analyzer][solver] Fix assertion on (NonLoc, Op, Loc) expressions
Previously, the `SValBuilder` could not encounter expressions of the
following kind:
NonLoc OP Loc
Loc OP NonLoc
Where the `Op` is other than `BO_Add`.
As of now, due to the smarter simplification and the fixedpoint
iteration, it turns out we can.
It can happen if the `Loc` was perfectly constrained to a concrete
value (`nonloc::ConcreteInt`), thus the simplifier can do
constant-folding in these cases as well.
Unfortunately, this could cause assertion failures, since we assumed
that the operator must be `BO_Add`, causing a crash.
---
In the patch, I decided to preserve the original behavior (aka. swap the
operands (if the operator is commutative), but if the `RHS` was a
`loc::ConcreteInt` call `evalBinOpNN()`.
I think this interpretation of the arithmetic expression is closer to
reality.
I also tried naively introducing a separate handler for
`loc::ConcreteInt` RHS, before doing handling the more generic `Loc` RHS
case. However, it broke the `zoo1backwards()` test in the `nullptr.cpp`
file. This highlighted for me the importance to preserve the original
behavior for the `BO_Add` at least.
PS: Sorry for introducing yet another branch into this `evalBinOpXX`
madness. I've got a couple of ideas about refactoring these.
We'll see if I can get to it.
The test file demonstrates the issue and makes sure nothing similar
happens. The `no-crash` annotated lines show, where we crashed before
applying this patch.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D115149
GitOrigin-RevId: a6816b957d28ab7855f2af1277c72a6d65b600b4
diff --git a/lib/StaticAnalyzer/Core/SValBuilder.cpp b/lib/StaticAnalyzer/Core/SValBuilder.cpp
index 71bfc86..8edcef3 100644
--- a/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -459,13 +459,23 @@
return evalBinOpLN(state, op, *LV, rhs.castAs<NonLoc>(), type);
}
- if (Optional<Loc> RV = rhs.getAs<Loc>()) {
- // Support pointer arithmetic where the addend is on the left
- // and the pointer on the right.
- assert(op == BO_Add);
+ 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;
+ };
- // Commute the operands.
- return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
+ if (IsCommutative(op)) {
+ // Swap operands.
+ return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
+ }
+
+ // 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);
+ }
}
return evalBinOpNN(state, op, lhs.castAs<NonLoc>(), rhs.castAs<NonLoc>(),
diff --git a/test/Analysis/symbol-simplification-nonloc-loc.cpp b/test/Analysis/symbol-simplification-nonloc-loc.cpp
new file mode 100644
index 0000000..cd7d700
--- /dev/null
+++ b/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core %s \
+// RUN: -triple x86_64-pc-linux-gnu -verify
+
+#define BINOP(OP) [](auto x, auto y) { return x OP y; }
+
+template <typename BinOp>
+void nonloc_OP_loc(int *p, BinOp op) {
+ long p_as_integer = (long)p;
+ if (op(12, p_as_integer) != 11)
+ return;
+
+ // Perfectly constrain 'p', thus 'p_as_integer', and trigger a simplification
+ // of the previously recorded constraint.
+ if (p) {
+ // no-crash
+ }
+ if (p == (int *)0x404) {
+ // no-crash
+ }
+}
+
+// Same as before, but the operands are swapped.
+template <typename BinOp>
+void loc_OP_nonloc(int *p, BinOp op) {
+ long p_as_integer = (long)p;
+ if (op(p_as_integer, 12) != 11)
+ return;
+
+ if (p) {
+ // no-crash
+ }
+ if (p == (int *)0x404) {
+ // no-crash
+ }
+}
+
+void instantiate_tests_for_nonloc_OP_loc(int *p) {
+ // Multiplicative and additive operators:
+ nonloc_OP_loc(p, BINOP(*));
+ nonloc_OP_loc(p, BINOP(/)); // no-crash
+ nonloc_OP_loc(p, BINOP(%)); // no-crash
+ nonloc_OP_loc(p, BINOP(+));
+ nonloc_OP_loc(p, BINOP(-)); // no-crash
+
+ // Bitwise operators:
+ // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
+ // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
+ nonloc_OP_loc(p, BINOP(<<)); // no-crash
+ nonloc_OP_loc(p, BINOP(>>)); // no-crash
+ nonloc_OP_loc(p, BINOP(&));
+ nonloc_OP_loc(p, BINOP(^));
+ nonloc_OP_loc(p, BINOP(|));
+}
+
+void instantiate_tests_for_loc_OP_nonloc(int *p) {
+ // Multiplicative and additive operators:
+ loc_OP_nonloc(p, BINOP(*));
+ loc_OP_nonloc(p, BINOP(/));
+ loc_OP_nonloc(p, BINOP(%));
+ loc_OP_nonloc(p, BINOP(+));
+ loc_OP_nonloc(p, BINOP(-));
+
+ // Bitwise operators:
+ loc_OP_nonloc(p, BINOP(<<));
+ loc_OP_nonloc(p, BINOP(>>));
+ loc_OP_nonloc(p, BINOP(&));
+ loc_OP_nonloc(p, BINOP(^));
+ loc_OP_nonloc(p, BINOP(|));
+}
+
+// from: nullptr.cpp
+void zoo1backwards() {
+ char **p = nullptr;
+ // expected-warning@+1 {{Dereference of null pointer [core.NullDereference]}}
+ *(0 + p) = nullptr; // warn
+ **(0 + p) = 'a'; // no-warning: this should be unreachable
+}
