Skip to content

[analyzer] Improve some comments in ArrayBoundCheckerV2 (NFC) #83545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 4, 2024
Merged

[analyzer] Improve some comments in ArrayBoundCheckerV2 (NFC) #83545

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b15b88d
[analyzer] Fix a typo in a comment in ArrayBoundCheckerV2 (NFC)
NagyDonat Mar 1, 2024
467b9c6
[analyzer] Improve some comments in ArrayBoundCheckerV2 (NFC)
NagyDonat Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,21 +301,27 @@ compareValueToThreshold(ProgramStateRef State, NonLoc Value, NonLoc Threshold,
// calling `evalBinOpNN`:
if (isNegative(SVB, State, Value) && isUnsigned(SVB, Threshold)) {
if (CheckEquality) {
// negative_value == unsigned_value is always false
// negative_value == unsigned_threshold is always false
return {nullptr, State};
}
// negative_value < unsigned_value is always false
// negative_value < unsigned_threshold is always true
return {State, nullptr};
}
if (isUnsigned(SVB, Value) && isNegative(SVB, State, Threshold)) {
// unsigned_value == negative_value and unsigned_value < negative_value are
// both always false
// unsigned_value == negative_threshold and
// unsigned_value < negative_threshold are both always false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit: Wouldn't it be better to write <=?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mentioning them separately because these are separate cases in this function. (It may check either "value < threshold" or "value == threshold" depending on the last argument.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I understand.

return {nullptr, State};
}
// FIXME: these special cases are sufficient for handling real-world
// FIXME: These special cases are sufficient for handling real-world
// comparisons, but in theory there could be contrived situations where
// automatic conversion of a symbolic value (which can be negative and can be
// positive) leads to incorrect results.
// NOTE: We NEED to use the `evalBinOpNN` call in the "common" case, because
// we want to ensure that assumptions coming from this precondition and
// assumptions coming from regular C/C++ operator calls are represented by
// constraints on the same symbolic expression. A solution that would
// evaluate these "mathematical" compariosns through a separate pathway would
// be a step backwards in this sense.

const BinaryOperatorKind OpKind = CheckEquality ? BO_EQ : BO_LT;
auto BelowThreshold =
Expand Down