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