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