[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> (1) UBOR is only triggered when the constant folding performed by the Clang Static Analyzer engine determines that the value of a binary operator expression is undefined

Yes I wholeheartedly agree, these checks are pre-condition checks, they should have never been implemented in `checkPostStmt`. Instead they should stop the engine from evaluating the statement entirely if they think undefined behavior would occur, just like the division by zero checker. Just for that reason I'm happy to remove the existing bitshift check in favor of this check, as well as remove the `ExprEngine`/`SValBuilder` code that produces `UndefinedVal`s in these cases. So, architecturally, this is the step in the right direction and I love it!

> (4) UBOR exhibits buggy behavior in code that involves cast expressions

Hmm, what makes your check more resilient to our overall type-incorrectness?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98
+void BitwiseShiftValidator::run() {
+  if (isValidShift()) {
+    Ctx.addTransition(State, createNoteTag());
+  }
+}
----------------
Because `addTransition()` is a very confusing API that's very easy to misuse in subtle ways (causing unexpected state splits), I feel anxious when I see functions like
```
  bool isValidShift();
  bool isOvershift();
  bool isOperandNegative(OperandSide Side);
  bool isLeftShiftOverflow();
```
that look like boolean getters but in fact call `addTransition()` under the hood. If we could at least call them `checkOvershift()` etc., that'd be much better. Ideally I wish there was some safeguard against producing redundant transitions, like make them return an `ExplodedNode` and chain them, or something like that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302
+                      pluralSuffix(MaximalAllowedShift));
+    R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(),
+                                                Ctx.getLocationContext()});
+    Ctx.emitReport(std::move(R));
----------------
Can we just append this to the warning? The `addNote()` is useful for notes that need to be placed in code outside of the execution path, but if it's always next to the warning, it probably doesn't make sense to display it separately.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:352-353
+        std::make_unique<PathSensitiveBugReport>(BT, ShortMsg, Msg, ErrNode);
+    bugreporter::trackExpressionValue(ErrNode, Op->getLHS(), *R);
+    bugreporter::trackExpressionValue(ErrNode, Op->getRHS(), *R);
+    return R;
----------------
Props!! Even if they don't do anything, ultimately it's their responsibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156312/new/

https://reviews.llvm.org/D156312