[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked an inline comment as done.
donat.nagy added a comment.

@steakhal  Thanks for the review!

I answered some of your questions; and I'll handle the rest soon.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:150
+      SVB.evalBinOp(State, Comparison, Val, LimitVal, SVB.getConditionType());
+  if (auto DURes = ResultVal.getAs<DefinedOrUnknownSVal>()) {
+    auto [StTrue, StFalse] = State->assume(DURes.value());
----------------
steakhal wrote:
> How about swapping these branches on the `ResultVal.isUndef()` predicate?
The argument of `ProgramState::assume()` must be a `DefinedOrUnknownSVal`, so I need to cast the return value of `evalBinOp` (which may be Undefined) to this type.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:157-158
+    }
+    // The code may be valid, so let's assume that it's valid:
+    State = StTrue;
+    if (StFalse) {
----------------
steakhal wrote:
> I wonder if we should add a message here that we introduced a new constraint: "X assumed to be non-negative".
> To add such a message, you should do something similar to what is done in the `StdLibraryFunctionsChecker`.
> Take the `C.getPredecessor()` and chain all your NoteTags by calling the `Pred = C.addTransition(NewState, Pred, Tag)` like this. This way. This is how one "sequence of ExplodedNodes" can be made.
This is already done by `BitwiseShiftValidator::createNoteTag()`. It's a bit complex because the checker merges all its assumptions into a single note tag (because I didn't want to emit two or three notes directly after each other.


================
Comment at: clang/test/Analysis/analyzer-config.c:36
 // CHECK-NEXT: cfg-temporary-dtors = true
+// CHECK-NEXT: consider-single-element-arrays-as-flexible-array-members = true
+// CHECK-NEXT: core.BitwiseShift:Pedantic = false
----------------
steakhal wrote:
> I'm pretty sure I got rid of this one :D
> Consider rebasing on top of `llvm/main`.
Oops, this might be an error introduced during a rebase... Thanks for catching it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312