[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/docs/analyzer/checkers.rst:35
+core.BitwiseShift (C, C++)
+"""""""""""""""""""""""""""""""
+
----------------



================
Comment at: clang/docs/analyzer/checkers.rst:44-50
+Moreover, if the pedantic mode is activated by
+``-analyzer-config core.BitwiseShift:Pedantic=true``, then this checker also
+reports situations where the _left_ operand of a shift operator is negative or
+overflow occurs during the right shift of a signed value. (Most compilers
+handle these predictably, but the C standard and the C++ standards before C++20
+say that they're undefined behavior. In the C++20 standard these constructs are
+well-defined, so activating pedantic mode in C++20 has no effect.)
----------------
I believe shifting out the "sign bit" is UB because the representation of the signed integer type was not defined.
Prior C++20 (?), it was allowed to represent signed integer types as `two's-complement` (virtually every platform uses this), `one's complement`, `sign-magnitude`. [[ https://en.wikipedia.org/wiki/Signed_number_representations | Wiki ]].
And it turns out, all these three representations behave differently with negative values.
Check out [[ https://youtu.be/JhUxIVf1qok?t=2089 | JF's talk ]] from 2018 about this.

I'd need to think about the relevance of C++20 in this context, but here is what I think of now:
My idea is that the observed behavior is not influenced by the "standard", rather by the time we released C++20, they realized that no actual platform uses weird signed integer representations.
Given this observation, I'd argue that we shouldn't have this flag at all.


================
Comment at: clang/docs/analyzer/checkers.rst:81
+
+Ensure the shift operands are in proper range before shifting.
+
----------------
We should exactly elaborate on what "proper" means here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:142
+                                              BinaryOperator::Opcode Comparison,
+                                              long long Limit) {
+  SValBuilder &SVB = Ctx.getSValBuilder();
----------------
One does not frequently see the `long long` type.
Do you have a specific reason why `int` or `unsigned` wouldn't be good?


================
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());
----------------
How about swapping these branches on the `ResultVal.isUndef()` predicate?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:152-156
+    if (!StTrue) {
+      // We detected undefined behavior (the caller will report it).
+      State = StFalse;
+      return false;
+    }
----------------
Generally, I don't favor mutating member functions.
It makes it harder to track how we ended up with a given State.



================
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) {
----------------
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.


================
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
----------------
I'm pretty sure I got rid of this one :D
Consider rebasing on top of `llvm/main`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312