[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

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

donat.nagy added a comment.

In D156312#4537200 <https://reviews.llvm.org/D156312#4537200>, @NoQ wrote:

>> (4) UBOR exhibits buggy behavior in code that involves cast expressions
>
> Hmm, what makes your check more resilient to our overall type-incorrectness?

The code of UBOR (UndefResultChecker.cpp) uses the method `LHS->countl_zero()` (count zeros on the left side of the binary representation) which is very convenient but uses the "cached" bit width that is stored in the APSInt and apparently not updated by the casts. The checker proposed in this commit avoids this pitfall because it calls `getActiveBits()` on the APSInt and subtracts the result from the bit width of the type of the LHS (which is queried from the AST node).



================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98
+void BitwiseShiftValidator::run() {
+  if (isValidShift()) {
+    Ctx.addTransition(State, createNoteTag());
+  }
+}
----------------
NoQ wrote:
> 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.
These functions were originally called `checkXXX()` but I felt that the meaning of their boolean return value was too confusing with that naming scheme.

I'll try to ensure that `emitReport` or `addTransition` are only called on the topmost layer (the `run()` method) and change the return types of internal subroutines to make them return nullable pointers (e.g. `std::unique_ptr<PathSensitiveBugReport>`) instead of `bool`s. This would (1) eliminate the "what does `true` mean at this point?" issues (2) clarify that this checker never performs more than one transition. (If the shift is invaild, we transition to a sink node by emitting a bug; if the shift is vaild, we perform a transition to record the state update.)


================
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));
----------------
NoQ wrote:
> 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.
I didn't append this to the warning because I felt that the warning text is already too long. (By the way, an earlier internal variant of this checker produced two separate notes next to the warning message.)

I tweaked the messages of this checker before initiating this review, but I feel that there's still some room for improvements. (E.g. in this particular case perhaps we could omit some of the details that are not immediately useful and could be manually calculated from other parts of the message...) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312