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

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

donat.nagy added inline comments.


================
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:
> steakhal wrote:
> > donat.nagy wrote:
> > > donat.nagy wrote:
> > > > steakhal wrote:
> > > > > 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?
> > > > This behavior is inherited from the code before my commit: the old block `if ( /*... =*/ extentBegin.getAs<NonLoc>() ) { /* ... */ }` is equivalent to `if (llvm::isa<UnknownSpaceRegion>(SR)) { /*...*/ }` and there was no early return connected to //this// NonLocness check. (The old code skipped the upper bound check if the result of `evalBinOpNN()` is unknown, and that's what I changed because I saw no reason to do an early return there.)
> > > > 
> > > > After some research into the memory region model, I think that there is no reason to perform an early return -- in fact, the condition of this  `if` seems to be too narrow because we would like to warn about code like
> > > >   struct foo {
> > > >     int tag;
> > > >     int array[5];
> > > >   };
> > > >   int f(struct foo *p) {
> > > >     return p->arr[-1];
> > > >   }
> > > > despite the fact that it's indexing into a `FieldRegion` inside a `SymbolicRegion` in `UnknownSpaceRegion`. That is, instead of checking the top-level MemorySpace, the correct logic would be checking the kind of the memory region and/or perhaps its immediate super-region.
> > > > 
> > > > As this is a complex topic and completely unrelated to the main goal of this commit; I'd prefer to keep the old (not ideal, but working) logic in this patch, then revisit this question by creating a separate follow-up commit.
> > > Minor nitpick: your suggested change accidentally negated the conditional :) ... and I said that it's "completely right". I'm glad that I noticed this and inserted the "!" before the `isa` check because otherwise it could've been annoying to debug this...
> > Agreed.
> Sorry about that. Happens
No problem :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148355