[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

[Balázs Benics via Phabricator via cfe-commits]

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