[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

Author: Balazs Benics

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

@@ -459,13 +459,23 @@ SVal SValBuilder::evalBinOp(ProgramStateRef state, BinaryOperator::Opcode op,
     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;
+    };
+
+    if (IsCommutative(op)) {
+      // Swap operands.
+      return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
+    }
 
-    // Commute the 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>(),

clang/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
+}