[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

72

The code should fit within 80 columns of text.

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

1 ↗

(On Diff #150377)

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.

8 ↗

(On Diff #150377)

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

1 ↗

(On Diff #150377)

Agreed. Fixed.

8 ↗

(On Diff #150377)

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

74

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

74

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

[george.karpenkov]

lib/StaticAnalyzer/Core/LoopWidening.cpp

74

@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

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

74

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

14

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

74

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

74

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

74

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

[NoQ]

lib/StaticAnalyzer/Core/LoopWidening.cpp

74

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

74

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

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/LoopWidening.cpp

 //===--- LoopWidening.cpp - Widen loops -------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// This file contains functions which are used to widen loops. A loop may be
 /// widened to approximate the exit state(s), without analyzing every
 /// iteration. The widening is done by invalidating anything which might be
 /// modified by the body of the loop.
 ///
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
 
 using namespace clang;
 using namespace ento;
+using namespace clang::ast_matchers;
+
+const auto MatchRef = "matchref";
 
 /// Return the loops condition Stmt or NULL if LoopStmt is not a loop
 static const Expr *getLoopCondition(const Stmt *LoopStmt) {
   switch (LoopStmt->getStmtClass()) {
   default:
     return nullptr;
   case Stmt::ForStmtClass:
     return cast<ForStmt>(LoopStmt)->getCond();
   case Stmt::WhileStmtClass:
     return cast<WhileStmt>(LoopStmt)->getCond();
   case Stmt::DoStmtClass:
     return cast<DoStmt>(LoopStmt)->getCond();
   }
 }
 
 namespace clang {
 namespace ento {
 
 ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
                                     const LocationContext *LCtx,
                                     unsigned BlockCount, const Stmt *LoopStmt) {
 
   assert(isa<ForStmt>(LoopStmt) || isa<WhileStmt>(LoopStmt) ||
          isa<DoStmt>(LoopStmt));
 
   // Invalidate values in the current state.
   // TODO Make this more conservative by only invalidating values that might
   //      be modified by the body of the loop.
   // TODO Nested loops are currently widened as a result of the invalidation
   //      being so inprecise. When the invalidation is improved, the handling
   //      of nested loops will also need to be improved.
+  ASTContext &ASTCtx = LCtx->getAnalysisDeclContext()->getASTContext();
   const StackFrameContext *STC = LCtx->getCurrentStackFrame();
   MemRegionManager &MRMgr = PrevState->getStateManager().getRegionManager();
   const MemRegion *Regions[] = {MRMgr.getStackLocalsRegion(STC),
                                 MRMgr.getStackArgumentsRegion(STC),
                                 MRMgr.getGlobalsRegion()};
   RegionAndSymbolInvalidationTraits ITraits;
   for (auto *Region : Regions) {
     ITraits.setTrait(Region,
                      RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
   }
 
+  // References should not be invalidated.
+  auto Matches = match(findAll(stmt(hasDescendant(varDecl(hasType(referenceType())).bind(MatchRef)))),
+                       *LCtx->getDecl()->getBody(), ASTCtx);
+  for (BoundNodes Match : Matches) {
+    const VarDecl *VD = Match.getNodeAs<VarDecl>(MatchRef);
+    assert(VD);
+    const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
+    ITraits.setTrait(VarMem,
+                     RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+  }
+
+
   // 'this' pointer is not an lvalue, we should not invalidate it. If the loop
   // is located in a method, constructor or destructor, the value of 'this'
   // pointer shoule remain unchanged.
   if (const CXXMethodDecl *CXXMD = dyn_cast<CXXMethodDecl>(STC->getDecl())) {
     const CXXThisRegion *ThisR = MRMgr.getCXXThisRegion(
         CXXMD->getThisType(STC->getAnalysisDeclContext()->getASTContext()),
         STC);
     ITraits.setTrait(ThisR,

test/Analysis/loop-widening-preserve-reference-type.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+
+void clang_analyzer_eval(int);
+
+struct A {
+  ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() {
+  const A &x = B();
+  for (int i = 0; i < 10; ++i) { }
+  clang_analyzer_eval(&x != 0); // expected-warning{{TRUE}}
+}                               // expected-warning@-1{{reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to true}}