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

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

steakhal added inline comments.


================
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();
----------------
donat.nagy wrote:
> 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...
Please :D


================
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
----------------
donat.nagy wrote:
> steakhal wrote:
> > 
> You're completely right, I just blindly copied this test from the needlessly overcomplicated `computeExtentBegin()`.
Hold on. This would only skip the lower bounds check if it's an `UnknownSpaceRegion`.
Shouldn't we early return instead?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:212-215
     }
 
-    assert(state_withinUpperBound);
-    state = state_withinUpperBound;
+    if (state_withinUpperBound)
+      state = state_withinUpperBound;
----------------
donat.nagy wrote:
> steakhal wrote:
> > You just left the guarded block in the previous line.
> > By moving this statement there you could spare the `if`.
> Nice catch :)
On second though no. The outer if guards `state_exceedsUpperBound`.
So this check seems necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148355