This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ClosedPublic

Authored by ormris on May 17 2018, 5:17 PM.

Details

Summary

Loop widening can invalidate an object reference. If the analyzer attempts to visit the destructor to a non-existent object it will crash. This patch ensures that type information is available before attempting to visit the object.

Diff Detail

Event Timeline

ormris created this revision.May 17 2018, 5:17 PM
NoQ added a comment.May 17 2018, 5:23 PM

Mmm, i think loop widening simply shouldn't invalidate references (though it should invalidate objects bound to them). Simply because you can't really reassign a reference.

Could we mark them as "preserve contents", like in D45491?

ormris added a comment.EditedMay 18 2018, 9:22 AM

That could work. Is the implementation similar to D45491?

NoQ added a comment.May 18 2018, 11:33 AM

Hopefully.

NoQ retitled this revision from Ensure that we only visit a destructor for a reference if type information is available. to [analyzer] Ensure that we only visit a destructor for a reference if type information is available..May 18 2018, 11:37 AM

This is my first static analyzer fix, so it may take me some time to figure this out.

NoQ added a comment.May 18 2018, 6:49 PM

Hmm, well, i guess it's not going to be a one-liner. You might have to iterate through all live variables in the scope, see if they are references, and add the trait. I think currently there's no way around scanning the current function body (i.e. LCtx->getDecl(), where LCtx is Pred->getLocationContext()) an AST visitor or an AST matcher. Once that's done, you can take Pred->getState()->getLValue(VD, LCtx) for every const VarDecl * VD you find and set the invalidation trait on that. It might be necessary to restrict your search to active variables (in the sense of LCtx->getAnalysis<RelaxedLiveVariables>()->isLive(S, VD)), where S is... dunno, some statement that makes sense here.

Probably global/static references should also not be invalidated. It'd take even more effort to find those.

I still think it's worth it because widening is indeed at fault, not the common destructor handling code. The assertion you step upon (that the cast<> always succeeds) is a valuable assertion that helped us find that bug, we shouldn't suppress it.

Loop widening in its current form is an experiment that was never announced to work, and, well, yeah, it has this sort of bugs. There is work started by @szepet in D36690 to turn widening into a whitelist-like thing.

MTC added a subscriber: MTC.May 21 2018, 4:23 AM

Hmm... Thanks for these tips. I'll see what I can find.

george.karpenkov resigned from this revision.May 30 2018, 4:29 PM
george.karpenkov added a subscriber: george.karpenkov.
ormris updated this revision to Diff 150377.Jun 7 2018, 11:55 AM

Use AST matchers to select references for preservation

MTC added a comment.Jun 8 2018, 7:31 AM

I didn't test the code, but the code seems correct. Thanks!

lib/StaticAnalyzer/Core/LoopWidening.cpp
88 ↗(On Diff #150377)

The code should fit within 80 columns of text.

test/Analysis/loop-widening-invalid-type.cpp
2

I think it's better to add more expressive tests. Like:

struct A {
  int x;
  A(int x) : x(x) {}
};

void invalid_type_region_access() {
  const A &a = A(10);
  for(int i = 0; i < 10; ++i) {}
  clang_analyzer_eval(a.x ==10); // expected-warning{{TRUE}}
}

I think should use more related names instead of loop-widening-invalid-type.cpp, like loop-widening-reference-type.

9

I don't know what the purpose of the test is, is the comment no-crash better?

ormris updated this revision to Diff 150567.Jun 8 2018, 2:02 PM
  • Reformat with clang-format-diff.py
  • Rename test
  • Modify test to use clang_analyzer_eval
ormris marked 3 inline comments as done.Jun 8 2018, 2:04 PM

Thanks for the comments so far.

test/Analysis/loop-widening-invalid-type.cpp
2

Agreed. Fixed.

9

I've changed the test to (hopefully) look for a valid address for "x".

MTC added a comment.Jun 8 2018, 11:55 PM

LGTM, @NoQ May have further feedback. Thanks!

ormris marked 2 inline comments as done.Jun 11 2018, 9:29 AM

Thanks for the review @MTC! I'll wait for @NoQ's feedback.

lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

IMO using the iterator directly (e.g. like it was done in https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213) leads to a much cleaner code and saves you from having to define a callback class.

ormris added inline comments.Jun 11 2018, 10:14 AM
lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

Hmm... I think that's a better approach. Let me see if I can get that working.

lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

@ormris Yeah I'm really not sure why all examples use the callback API by default.

NoQ added a comment.Jun 11 2018, 4:00 PM

Thanks, this looks very reasonable!

I agree that the syntax pointed out by @george.karpenkov is much cleaner.

include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
30 ↗(On Diff #150567)

You can obtain the ASTContext either from PrevState->getStateManager() or from LCtx->getAnalysisDeclContext(), there's no need to pass it separately.

lib/StaticAnalyzer/Core/LoopWidening.cpp
46–48 ↗(On Diff #150567)

We usually just write X(Y y): y(y) {} and don't bother about name collisions.

89 ↗(On Diff #150567)

Also, please match only the local AST, i.e. the body of the function under analysis, which can be obtained as LCtx->getDecl()->getBody(). There's no need to scan the whole translation unit.

test/Analysis/loop-widening-preserve-reference-type.cpp
13 ↗(On Diff #150567)

The expression is trivially true, i don't think it's exactly the thing we want to be testing.

I'm not sure how to come up with a better test here. One good thing to test, which i'd prefer, would be &x != 0 - which should be true because stack objects can't have address 0 and the analyzer is supposed to know that, but the symbolic pointer that would have overwritten x during over-aggressive widening could as well be null.

Other alternatives are to add some sort of clang_analyzer_getMemorySpace() and check that the variable is still on the stack (which tests more, but is also more work) or use clang_analyzer_dump()/clang_analyzer_explain() to verify the exact value (but that'd test too much as it'd also test the dump format, which is redundant).

NoQ added inline comments.Jun 11 2018, 4:02 PM
lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

Actually not sure, would widening screw the previous stack frames as well? We should test that, i guess. And even then, it's better to match a few stack frames in the current stack trace than to match the whole translation unit.

ormris added inline comments.Jun 11 2018, 6:29 PM
lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

I can't seem to get the new syntax to work. The following assert(0) is never triggered.

auto Matches = match(varDecl(hasType(referenceType())).bind(MatchRef),
                     *LCtx->getDecl()->getBody(), ASTCtx);
for (BoundNodes Match : Matches) {
  assert(0 && "anything");
  const VarDecl *VD = Match.getNodeAs<VarDecl>(MatchRef);
  const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
  ITraits.setTrait(VarMem,
                   RegionAndSymbolInvalidationTraits::TK_PreserveContents);
}
ormris added inline comments.Jun 12 2018, 12:41 PM
lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

It appears that "decl()" produces no matches...

NoQ added inline comments.Jun 12 2018, 12:45 PM
lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

Mmm, i think when you're matching using match rather than matchAST, you need to write a match for the exact statement rather than any sub-statement. I.e., those are like "match" vs. "find". I.e., try wraping your matcher into stmt(hasDescendant(...)), where stmt() would match the whole function body (most likely a CompoundStmt for the curly braces around the function body, but there are some other cases).

ormris added inline comments.Jun 12 2018, 1:00 PM
lib/StaticAnalyzer/Core/LoopWidening.cpp
89 ↗(On Diff #150567)

Looks like that worked. Thanks!

ormris updated this revision to Diff 151043.Jun 12 2018, 2:58 PM
  • Use match function iterators rather than a callback class
  • Update test
ormris marked 4 inline comments as done.Jun 12 2018, 2:59 PM
george.karpenkov accepted this revision.Jun 12 2018, 3:03 PM

Looks good to me, apart from the nit on an unused header.

lib/StaticAnalyzer/Core/LoopWidening.cpp
21 ↗(On Diff #151043)

Seems unused now?

This revision is now accepted and ready to land.Jun 12 2018, 3:03 PM
ormris updated this revision to Diff 151046.Jun 12 2018, 3:12 PM

Remove unneeded header

ormris marked an inline comment as done.Jun 12 2018, 3:14 PM

Sounds good. I'll go ahead and commit this. Thanks for the review!

NoQ accepted this revision.Jun 12 2018, 3:14 PM

I'm still curious whether this also works:

void foo() {
  const A &x = B();
  bar();
}

void bar() {
  for (int i = 0; i < 10; ++i) {}
}

Though we can land this patch and deal with this later.

Hmm... I'll take a look.

ormris closed this revision.Jun 13 2018, 9:59 AM

This change has been committed, so I'm closing this review.

https://reviews.llvm.org/rC334554

This change has been committed, so I'm closing this review.

@ormris The process which should be followed here is to add a line (exactly) "Differential Revision: https://reviews.llvm.org/D47044" to the bottom of your commit message, so that the phabricator can cross-reference the review and the commit.
Alternatively, you could use the "arc" tool which would do that for you.

Thanks @george.karpenkov . I was wondering why it didn't close automatically.

In D47044#1130339, @NoQ wrote:

I'm still curious whether this also works:

void foo() {
  const A &x = B();
  bar();
}

void bar() {
  for (int i = 0; i < 10; ++i) {}
}

Though we can land this patch and deal with this later.

@NoQ The above example doesn't crash.

NoQ added a comment.Jun 22 2018, 12:55 PM

Aha, yeah, i see. It only invalidates the current stack frame, and additionally it's impossible to bring the reference into the current stack frame by reference, because, well, it's already a reference and you can't mutate a reference.

Ok then!