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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
