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

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

@steakhal Thanks for the review, it was really instructive :). I'll probably upload the updated commit on Monday.

What kind of measurements are you suggesting? I analyzed a few open source projects with the patched Clang SA (see results in my previous commit) and a few additional analysis runs are still ongoing. (I didn't measure performance/runtime, because I don't think that this change significantly affects those.)



================
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();
----------------
steakhal wrote:
> 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...
Yes, I was also very annoyed by this "intrustive optional" behavior and I seriously considered refactoring the code to use a real std::optional, but I didn't want to snowball this change into a full-scale rewrite so I ended up with the assert in the constructor and this comment. Perhaps I'll refactor it in a separate NFC...


================
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
----------------
steakhal wrote:
> 
You're completely right, I just blindly copied this test from the needlessly overcomplicated `computeExtentBegin()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:212-215
     }
 
-    assert(state_withinUpperBound);
-    state = state_withinUpperBound;
+    if (state_withinUpperBound)
+      state = state_withinUpperBound;
----------------
steakhal wrote:
> You just left the guarded block in the previous line.
> By moving this statement there you could spare the `if`.
Nice catch :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148355/new/

https://reviews.llvm.org/D148355