Implement inlining of temporary destructors.

[alexmc]

Woo :D nice work!

test/Analysis/temporaries.cpp

106–107

Remove fixme

[klimek]

Added more tests, and a FIXME for the parameter passing test.
Addressed review comments.

[klimek]

Do not inline temporary destructors for temporaries that bind to parameters.

[jordan_rose]

Do we really need to put the "binds parameter" information in the CFG? I was hoping we could look in the parent map to see how the temporary is used (like for 'new' right now).

include/clang/Analysis/CFG.h

285 ↗

(On Diff #12305)

Why not just use "self"? Or &T? Why "T"? Why does T require a dynamic allocation?

lib/StaticAnalyzer/Core/ExprEngine.cpp

688–693

This is not how LCVs work. An LCV is a snapshot of a region at a particular "time" (state of the Store); the region may not exist in the current store. The "best" answer in the current scheme is to use createTemporaryRegionIfNeeded, but the right answer may end up being to disregard the rvalue-ness of temporaries (as we talked about some on cfe-dev).

lib/StaticAnalyzer/Core/ProgramState.cpp

508–509

This is not correct; see above.

[klimek]

Dumping of intermediate state developed with Jordan...

lib/Analysis/LiveVariables.cpp

for (CFGBlock::const_reverse_iterator it = block->rbegin(),
        ei = block->rend(); it != ei; ++it) {
     const CFGElement &elem = *it;
 
     if (Optional<CFGAutomaticObjDtor> Dtor =
             elem.getAs<CFGAutomaticObjDtor>()) {
       val.liveDecls = DSetFact.add(val.liveDecls, Dtor->getVarDecl());
       continue;
     }
+    if (Optional<CFGTemporaryDtor> Dtor =
+            elem.getAs<CFGTemporaryDtor>()) {
+      // Temporary objects need to survive until the destructor is called.
+      val.liveStmts = SSetFact.add(val.liveStmts,
+                                   Dtor->getBindTemporaryExpr()->getSubExpr());
+      continue;
+    }
 
     if (!elem.getAs<CFGStmt>())
       continue;
     
     const Stmt *S = elem.castAs<CFGStmt>().getStmt();
     TF.Visit(const_cast<Stmt*>(S));
     stmtsToLiveness[S] = val;
   }

lib/StaticAnalyzer/Core/CallEvent.cpp

CallEventManager::getCaller(const StackFrameContext *CalleeCtx,
   }
 
   // Fall back to the CFG. The only thing we haven't handled yet is
   // destructors, though this could change in the future.
   const CFGBlock *B = CalleeCtx->getCallSiteBlock();
   CFGElement E = (*B)[CalleeCtx->getIndex()];
   assert(E.getAs<CFGImplicitDtor>() &&
          "All other CFG elements should have exprs");
-  assert(!E.getAs<CFGTemporaryDtor>() && "We don't handle temporaries yet");
 
   SValBuilder &SVB = State->getStateManager().getSValBuilder();
   const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CalleeCtx->getDecl());
   Loc ThisPtr = SVB.getCXXThis(Dtor, CalleeCtx);
   SVal ThisVal = State->getSVal(ThisPtr);
 
   const Stmt *Trigger;
   if (Optional<CFGAutomaticObjDtor> AutoDtor = E.getAs<CFGAutomaticObjDtor>())

lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
   ExplodedNodeSet CleanDtorState;
   StmtNodeBuilder StmtBldr(Pred, CleanDtorState, *currBldrCtx);
   ProgramStateRef State = Pred->getState();
   assert(State->contains<InitializedTemporariesSet>(
       std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame())));
   State = State->remove<InitializedTemporariesSet>(
       std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame()));
   StmtBldr.generateNode(D.getBindTemporaryExpr(), Pred, State);
-
-  QualType varType = D.getBindTemporaryExpr()->getSubExpr()->getType();
   assert(CleanDtorState.size() == 1);
   ExplodedNode *CleanPred = *CleanDtorState.begin();
-  // FIXME: Inlining of temporary destructors is not supported yet anyway, so
-  // we just put a NULL region for now. This will need to be changed later.
-  VisitCXXDestructor(varType, nullptr, D.getBindTemporaryExpr(),
+
+  QualType varType = D.getBindTemporaryExpr()->getSubExpr()->getType();
+
+  const LocationContext *LCtx = CleanPred->getLocationContext();
+  SVal Val = CleanPred->getState()->getSVal(
+      D.getBindTemporaryExpr()->getSubExpr(), LCtx->getCurrentStackFrame());
+  const MemRegion *Region = nullptr;
+  // If the class does not have any members, there will not be a region
+  // for it bound in the environment.
+  if (Optional<nonloc::LazyCompoundVal> LCV =
+          Val.getAs<nonloc::LazyCompoundVal>()) {
+    Region = LCV->getRegion();
+  }
+
+  VisitCXXDestructor(varType, Region, D.getBindTemporaryExpr(),
                      /*IsBase=*/false, CleanPred, Dst);
 }
 
 void ExprEngine::processCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
                                                NodeBuilderContext &BldCtx,
                                                ExplodedNode *Pred,
                                                ExplodedNodeSet &Dst,
                                                const CFGBlock *DstT,

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

case CE_CXXConstructor: {
     if (Ctor.getDecl()->getParent()->hasTrivialDestructor())
       break;
 
     // For other types, only inline constructors if destructor inlining is
     // also enabled.
     if (!Opts.mayInlineCXXMemberFunction(CIMK_Destructors))
       return CIP_DisallowedAlways;
 
-    // FIXME: This is a hack. We don't handle temporary destructors
-    // right now, so we shouldn't inline their constructors.
-    if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete)
-      if (!Target || !isa<DeclRegion>(Target))
-        return CIP_DisallowedOnce;
-
     break;
   }
   case CE_CXXDestructor: {
     if (!Opts.mayInlineCXXMemberFunction(CIMK_Destructors))
       return CIP_DisallowedAlways;
 
     // Inlining destructors requires building the CFG correctly.
     const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
bool ExprEngine::shouldInlineCall(const CallEvent &Call, const Decl *D,
   if (!D)
     return false;
 
   AnalysisManager &AMgr = getAnalysisManager();
   AnalyzerOptions &Opts = AMgr.options;
   AnalysisDeclContextManager &ADCMgr = AMgr.getAnalysisDeclContextManager();
   AnalysisDeclContext *CalleeADC = ADCMgr.getContext(D);
 
-  // Temporary object destructor processing is currently broken, so we never
-  // inline them.
-  // FIXME: Remove this once temp destructors are working.
-  if (isa<CXXDestructorCall>(Call)) {
-    if ((*currBldrCtx->getBlock())[currStmtIdx].getAs<CFGTemporaryDtor>())
-      return false;
-  }
-
   // The auto-synthesized bodies are essential to inline as they are
   // usually small and commonly used. Note: we should do this check early on to
   // ensure we always inline these calls.
   if (CalleeADC->isBodyAutosynthesized())
     return true;
 
   if (!AMgr.shouldInlineCall())
     return false;

lib/StaticAnalyzer/Core/ProgramState.cpp

ProgramStateRef ProgramStateManager::removeGDM(ProgramStateRef state, void *Key) {
   if (NewM == OldM)
     return state;
 
   ProgramState NewState = *state;
   NewState.GDM = NewM;
   return getPersistentState(NewState);
 }
 
 bool ScanReachableSymbols::scan(nonloc::LazyCompoundVal val) {
-  bool wasVisited = !visited.insert(val.getCVData()).second;
-  if (wasVisited)
-    return true;
-
-  StoreManager &StoreMgr = state->getStateManager().getStoreManager();
-  // FIXME: We don't really want to use getBaseRegion() here because pointer
-  // arithmetic doesn't apply, but scanReachableSymbols only accepts base
-  // regions right now.
-  const MemRegion *R = val.getRegion()->getBaseRegion();
-  return StoreMgr.scanReachableSymbols(val.getStore(), R, *this);
+  return scan(val.getRegion());
 }
 
 bool ScanReachableSymbols::scan(nonloc::CompoundVal val) {
   for (nonloc::CompoundVal::iterator I=val.begin(), E=val.end(); I!=E; ++I)
     if (!scan(*I))
       return false;
 
   return true;

lib/StaticAnalyzer/Core/RegionStore.cpp

RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
   // If we get here, the location should be a region.
   const MemRegion *R = L.castAs<loc::MemRegionVal>().getRegion();
 
   // Check if the region is a struct region.
   if (const TypedValueRegion* TR = dyn_cast<TypedValueRegion>(R)) {
     QualType Ty = TR->getValueType();
     if (Ty->isArrayType())
       return bindArray(B, TR, V);
-    if (Ty->isStructureOrClassType())
+    if (Ty->isStructureOrClassType()) {
       return bindStruct(B, TR, V);
+    }
     if (Ty->isVectorType())
       return bindVector(B, TR, V);
     if (Ty->isUnionType())
       return bindAggregate(B, TR, V);
   }
 
   if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R)) {
     // Binding directly to a symbolic region should be treated as binding
RegionBindingsRef RegionStoreManager::bindStruct(RegionBindingsConstRef B,
   const RecordDecl *RD = RT->getDecl();
 
   if (!RD->isCompleteDefinition())
     return B;
 
   // Handle lazy compound values and symbolic values.
   if (Optional<nonloc::LazyCompoundVal> LCV =
         V.getAs<nonloc::LazyCompoundVal>()) {
-    if (Optional<RegionBindingsRef> NewB = tryBindSmallStruct(B, R, RD, *LCV))
+    if (Optional<RegionBindingsRef> NewB = tryBindSmallStruct(B, R, RD, *LCV)) {
       return *NewB;
+    }
     return bindAggregate(B, R, V);
   }
   if (V.getAs<nonloc::SymbolVal>())
     return bindAggregate(B, R, V);
 
   // We may get non-CompoundVal accidentally due to imprecise cast logic or
   // that we are binding symbolic struct value. Kill the field values, and if
   // the value is symbolic go and bind it as a "default" binding.

test/Analysis/temporaries.cpp

 // RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++03 %s
 // RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++11 %s
 // RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s -std=c++11
 
 extern bool clang_analyzer_eval(bool);
-extern bool clang_analyzer_warnIfReached();
+extern void clang_analyzer_warnIfReached();
 
 struct Trivial {
   Trivial(int x) : value(x) {}
   int value;
 };
 
 struct NonTrivial : public Trivial {
   NonTrivial(int x) : Trivial(x) {}
namespace compound_literals {
   };
 
   void test() {
     clang_analyzer_eval(((POD){1, 42}).y == 42); // expected-warning{{TRUE}}
     clang_analyzer_eval(((HasDtor){1, 42}).y == 42); // expected-warning{{TRUE}}
 
 #if __cplusplus >= 201103L
     clang_analyzer_eval(((HasCtor){1, 42}).y == 42); // expected-warning{{TRUE}}
 
-    // FIXME: should be TRUE, but we don't inline the constructors of
-    // temporaries because we can't model their destructors yet.
-    clang_analyzer_eval(((HasCtorDtor){1, 42}).y == 42); // expected-warning{{UNKNOWN}}
+    clang_analyzer_eval(((HasCtorDtor){1, 42}).y == 42); // expected-warning{{TRUE}}
 #endif
   }
 }
 
 namespace destructors {
   struct Dtor {
     ~Dtor();
   };
#ifdef TEMPORARY_DTORS
   void testBinaryOperatorShortcut(bool value) {
     if (value) {
       if (false && false && check(NoReturnDtor()) && true) {
         clang_analyzer_eval(true);
       }
     }
   }
 
+  struct WriteInDestructor {
+    WriteInDestructor(int *p) : p(p) {}
+    ~WriteInDestructor() { *p = 23; } // no warning
+    int *p;
+  };
+  void testDtorInlining() {
+    int x;
+    (WriteInDestructor(&x));
+    clang_analyzer_eval(x == 23); // expected-warning{{TRUE}}
+  }
+
+  struct FlagDestructor {
+    FlagDestructor() : flag(false) {}
+    ~FlagDestructor() {
+      // FIXME: This currently doesn't trigger because when use(FlagDestructor)
+      // is inlined, the memory region of the by-value parameter is copied, but
+      // neither a constructor nor destructor is called.
+      if (flag) clang_analyzer_warnIfReached(); // no warning
+    }
+    bool flag;
+  };
+  void use(FlagDestructor d) {
+    d.flag = true;
+  }
+  void testByValueArgs() {
+    FlagDestructor d;
+    use(d);
+  }
+
   void testIfAtEndOfLoop() {
     int y = 0;
     while (true) {
       if (y > 0) {
         clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
       }
       ++y;
       // Test that the CFG gets hooked up correctly when temporary destructors