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

[ormris]

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.

[NoQ]

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]

That could work. Is the implementation similar to D45491?

[NoQ]

Hopefully.

[ormris]

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

[NoQ]

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.

[ormris]

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

[ormris]

Use AST matchers to select references for preservation

[MTC]

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]

• Reformat with clang-format-diff.py
• Rename test
• Modify test to use clang_analyzer_eval

[ormris]

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]

LGTM, @NoQ May have further feedback. Thanks!

[ormris]

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

[george.karpenkov]

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]

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.

[george.karpenkov]

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]

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]

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]

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]

lib/StaticAnalyzer/Core/LoopWidening.cpp

89 ↗

(On Diff #150567)

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

[NoQ]

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]

lib/StaticAnalyzer/Core/LoopWidening.cpp

89 ↗

(On Diff #150567)

Looks like that worked. Thanks!

[ormris]

• Use match function iterators rather than a callback class
• Update test

[george.karpenkov]

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

lib/StaticAnalyzer/Core/LoopWidening.cpp

21 ↗

(On Diff #151043)

Seems unused now?

[ormris]

Remove unneeded header

[ormris]

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

[NoQ]

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.

[ormris]

Hmm... I'll take a look.

[ormris]

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

https://reviews.llvm.org/rC334554

[george.karpenkov]

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.

[ormris]

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

[ormris]

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]

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!

lib/StaticAnalyzer/Core/ExprEngine.cpp

if (varType->isReferenceType()) {
     if (!ValueRegion) {
       // FIXME: This should not happen. The language guarantees a presence
       // of a valid initializer here, so the reference shall not be undefined.
       // It seems that we're calling destructors over variables that
       // were not initialized yet.
       return;
     }
     Region = ValueRegion->getBaseRegion();
+    if (!isa<TypedValueRegion>(Region))
+      // Loop widening will sometimes invalidate typed regions.
+      return;
+ 
     varType = cast<TypedValueRegion>(Region)->getValueType();
   }
 
   // FIXME: We need to run the same destructor on every element of the array.
   // This workaround will just run the first destructor (which will still
   // invalidate the entire array).
   EvalCallOptions CallOpts;
   Region = makeZeroElementRegion(state, loc::MemRegionVal(Region), varType,

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

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+
+struct A {
+  ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() { // expected-no-diagnostics
+  const A &x = B();
+  for(int i = 0; i < 10; ++i) {}
+}