steakhal added a comment.
Please mark comments "Done" where applicable.
================
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:
> 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.
================
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:
> 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
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
