steakhal added a comment.
I like this. Looks really solid.
I only had style nitpicks.
Have you run measurements?
After that we could land it, I think.
FYI: I'll also schedule a test run with this patch. You should expect the
results in a week - not because it takes that long, but because I need to find
time for evaluating it xD.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:57-61
+ RegionRawOffsetV2(const SubRegion *base, SVal offset)
+ : baseRegion(base), byteOffset(offset) {
+ if (baseRegion)
+ assert(llvm::isa<NonLoc>(byteOffset));
+ }
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:121-135
+ if (auto ConcreteTh = Threshold.getAs<nonloc::ConcreteInt>()) {
+ std::tie(Value, Threshold) = getSimplifiedOffsets(Value, *ConcreteTh, SVB);
+ }
+ if (auto ConcreteTh = Threshold.getAs<nonloc::ConcreteInt>()) {
+ QualType T = Value.getType(SVB.getContext());
+ if (T->isUnsignedIntegerType() && ConcreteTh->getValue().isNegative()) {
+ // In this case we reduced the bound check to a comparison of the form
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:136-137
+ }
+ SVal belowThreshold =
+ SVB.evalBinOpNN(State, BO_LT, Value, Threshold, SVB.getConditionType());
+
----------------
steakhal wrote:
>
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:136-140
+ SVal belowThreshold =
+ SVB.evalBinOpNN(State, BO_LT, Value, Threshold, SVB.getConditionType());
+
+ if (std::optional<NonLoc> belowThresholdNL = belowThreshold.getAs<NonLoc>())
+ return State->assume(*belowThresholdNL);
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:167-168
- NonLoc rawOffsetVal = rawOffset.getByteOffset();
-
- // CHECK LOWER BOUND: Is byteOffset < extent begin?
- // If so, we are doing a load/store
- // before the first valid offset in the memory region.
+ // At this point we know that rawOffset is not default-constructed so we may
+ // call this:
+ NonLoc ByteOffset = rawOffset.getByteOffset();
----------------
I don't think we need an explanation here.
BTW This "optional-like" `RegionRawOffsetV2` makes me angry. It should be an
`std::optional<RegionRawOffsetV2>` in the first place.
You don't need to take actions, I'm just ranting...
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173
+ const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace();
+ if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) {
+ // a pointer to UnknownSpaceRegionKind may point to the middle of
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:174-175
+ if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) {
+ // a pointer to UnknownSpaceRegionKind may point to the middle of
+ // an allocated region
----------------
Good point.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:183
if (state_precedesLowerBound && !state_withinLowerBound) {
+ // We know that the index definitely precedes the lower bound
reportOOB(checkerContext, state_precedesLowerBound, OOB_Precedes);
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:195
+ getDynamicExtent(state, rawOffset.getRegion(), svalBuilder);
+ if (std::optional<NonLoc> KnownSize = Size.getAs<NonLoc>()) {
+ ProgramStateRef state_withinUpperBound, state_exceedsUpperBound;
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:196-198
+ ProgramStateRef state_withinUpperBound, state_exceedsUpperBound;
+ std::tie(state_withinUpperBound, state_exceedsUpperBound) =
+ compareValueToThreshold(state, ByteOffset, *KnownSize, svalBuilder);
----------------
I think as we don't plan to overwrite/assign to these states, we could just use
structured bindings.
I think that should be preferred over `std::tie()`-ing think. That is only not
widespread because we just recently moved to C++17.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:212-215
}
- assert(state_withinUpperBound);
- state = state_withinUpperBound;
+ if (state_withinUpperBound)
+ state = state_withinUpperBound;
----------------
You just left the guarded block in the previous line.
By moving this statement there you could spare the `if`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:310-311
offset = getValue(offset, svalBuilder);
if (!offset.isUnknownOrUndef())
return RegionRawOffsetV2(subReg, offset);
}
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148355/new/
https://reviews.llvm.org/D148355
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
