Hi jordan_rose, krememek,
Alex McCarthy and me have put our heads together and come up with something
that works. This still needs some cleanup, and has broken tests (in the compiler
based "consumed" and "reachable code" analysis, as mentioned in other threads).
We would like to get some feedback on the architecture, and whether the changes
the the CFG make sense or are expected to make problems in corner cases.
The gist of the change is a change to how the CFG is created:
For every temporary, add a new block that runs the dtor, and a decision
block that has the CXXBindTemporaryExpr as terminator. That way, we have one
entry and exit block for each temporary cleanup, and as long as they're chained
in the right order, it'll work.
This is of course suboptimal in multiple ways:
- often we know when one destructor has executed that we must also execute
the dtor of things that were generated previously; this makes the code and
the CFG significantly more complex, though; the biggest downside seems to
be that we potentially generate more states; not sure whether it's worth it
- often we do not actually need a new block (for example, if the temporary
is created unconditionally); in that case, we could just run with a single
block; again this would make the code more complex though, and I'm not sure
it's worth the effort, unless we feel it's an important invariant on the CFG
http://reviews.llvm.org/D3627
Files:
lib/Analysis/CFG.cpp
lib/StaticAnalyzer/Core/CoreEngine.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/temporaries.cpp
Index: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -3486,57 +3486,6 @@
}
CFGBlock *CFGBuilder::VisitBinaryOperatorForTemporaryDtors(BinaryOperator *E) {
- if (E->isLogicalOp()) {
- // Destructors for temporaries in LHS expression should be called after
- // those for RHS expression. Even if this will unnecessarily create a block,
- // this block will be used at least by the full expression.
- autoCreateBlock();
- CFGBlock *ConfluenceBlock = VisitForTemporaryDtors(E->getLHS());
- if (badCFG)
- return NULL;
-
- Succ = ConfluenceBlock;
- Block = NULL;
- CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getRHS());
-
- if (RHSBlock) {
- if (badCFG)
- return NULL;
-
- // If RHS expression did produce destructors we need to connect created
- // blocks to CFG in same manner as for binary operator itself.
- CFGBlock *LHSBlock = createBlock(false);
- LHSBlock->setTerminator(CFGTerminator(E, true));
-
- // For binary operator LHS block is before RHS in list of predecessors
- // of ConfluenceBlock.
- std::reverse(ConfluenceBlock->pred_begin(),
- ConfluenceBlock->pred_end());
-
- // See if this is a known constant.
- TryResult KnownVal = tryEvaluateBool(E->getLHS());
- if (KnownVal.isKnown() && (E->getOpcode() == BO_LOr))
- KnownVal.negate();
-
- // Link LHSBlock with RHSBlock exactly the same way as for binary operator
- // itself.
- if (E->getOpcode() == BO_LOr) {
- addSuccessor(LHSBlock, KnownVal.isTrue() ? NULL : ConfluenceBlock);
- addSuccessor(LHSBlock, KnownVal.isFalse() ? NULL : RHSBlock);
- } else {
- assert (E->getOpcode() == BO_LAnd);
- addSuccessor(LHSBlock, KnownVal.isFalse() ? NULL : RHSBlock);
- addSuccessor(LHSBlock, KnownVal.isTrue() ? NULL : ConfluenceBlock);
- }
-
- Block = LHSBlock;
- return LHSBlock;
- }
-
- Block = ConfluenceBlock;
- return ConfluenceBlock;
- }
-
if (E->isAssignmentOp()) {
// For assignment operator (=) LHS expression is visited
// before RHS expression. For destructors visit them in reverse order.
@@ -3569,76 +3518,29 @@
Succ = B;
Block = createNoReturnBlock();
} else {
- autoCreateBlock();
+ if (B)
+ Succ = B;
+ Block = createBlock();
}
appendTemporaryDtor(Block, E);
+
+ CFGBlock *Decision = createBlock(false);
+ Decision->setTerminator(CFGTerminator(E, true));
+ addSuccessor(Decision, Block);
+ addSuccessor(Decision, Succ);
+ Block = Decision;
B = Block;
}
return B;
}
CFGBlock *CFGBuilder::VisitConditionalOperatorForTemporaryDtors(
AbstractConditionalOperator *E, bool BindToTemporary) {
- // First add destructors for condition expression. Even if this will
- // unnecessarily create a block, this block will be used at least by the full
- // expression.
- autoCreateBlock();
- CFGBlock *ConfluenceBlock = VisitForTemporaryDtors(E->getCond());
- if (badCFG)
- return NULL;
- if (BinaryConditionalOperator *BCO
- = dyn_cast<BinaryConditionalOperator>(E)) {
- ConfluenceBlock = VisitForTemporaryDtors(BCO->getCommon());
- if (badCFG)
- return NULL;
- }
-
- // Try to add block with destructors for LHS expression.
- CFGBlock *LHSBlock = NULL;
- Succ = ConfluenceBlock;
- Block = NULL;
- LHSBlock = VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary);
- if (badCFG)
- return NULL;
-
- // Try to add block with destructors for RHS expression;
- Succ = ConfluenceBlock;
- Block = NULL;
- CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getFalseExpr(),
- BindToTemporary);
- if (badCFG)
- return NULL;
-
- if (!RHSBlock && !LHSBlock) {
- // If neither LHS nor RHS expression had temporaries to destroy don't create
- // more blocks.
- Block = ConfluenceBlock;
- return Block;
- }
-
- Block = createBlock(false);
- Block->setTerminator(CFGTerminator(E, true));
- assert(Block->getTerminator().isTemporaryDtorsBranch());
-
- // See if this is a known constant.
- const TryResult &KnownVal = tryEvaluateBool(E->getCond());
-
- if (LHSBlock) {
- addSuccessor(Block, LHSBlock, !KnownVal.isFalse());
- } else if (KnownVal.isFalse()) {
- addSuccessor(Block, NULL);
- } else {
- addSuccessor(Block, ConfluenceBlock);
- std::reverse(ConfluenceBlock->pred_begin(), ConfluenceBlock->pred_end());
- }
-
- if (!RHSBlock)
- RHSBlock = ConfluenceBlock;
-
- addSuccessor(Block, RHSBlock, !KnownVal.isTrue());
-
- return Block;
+ CFGBlock *CondBlock = VisitForTemporaryDtors(E->getCond());
+ CFGBlock *TrueBlock = VisitForTemporaryDtors(E->getTrueExpr());
+ CFGBlock *FalseBlock = VisitForTemporaryDtors(E->getFalseExpr());
+ return FalseBlock ? FalseBlock : (TrueBlock ? TrueBlock : CondBlock);
}
} // end anonymous namespace
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -346,6 +346,10 @@
default:
llvm_unreachable("Analysis for this terminator not implemented.");
+ case Stmt::CXXBindTemporaryExprClass:
+ HandleBranch(B->getTerminator().getStmt(), Term, B, Pred);
+ return;
+
// Model static initializers.
case Stmt::DeclStmtClass:
HandleStaticInit(cast<DeclStmt>(Term), B, Pred);
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -51,6 +51,10 @@
STATISTIC(NumTimesRetriedWithoutInlining,
"The # of times we re-evaluated a call without inlining");
+REGISTER_TRAIT_WITH_PROGRAMSTATE(
+ InitializedTemporariesSet,
+ llvm::ImmutableSet<const CXXBindTemporaryExpr *>);
+
//===----------------------------------------------------------------------===//
// Engine construction and deletion.
//===----------------------------------------------------------------------===//
@@ -757,6 +761,20 @@
// Handled due to fully linearised CFG.
break;
+ case Stmt::CXXBindTemporaryExprClass: {
+ Bldr.takeNodes(Pred);
+ const CXXBindTemporaryExpr *BTE = cast<CXXBindTemporaryExpr>(S);
+ StmtNodeBuilder StmtBldr(Pred, Dst, *currBldrCtx);
+ ProgramStateRef State = Pred->getState();
+ assert(!getAnalysisManager().options.includeTemporaryDtorsInCFG() ||
+ !State->contains<InitializedTemporariesSet>(BTE));
+ State = State->add<InitializedTemporariesSet>(BTE);
+ StmtBldr.generateNode(BTE, Pred, State);
+
+ Bldr.addNodes(Dst);
+ break;
+ }
+
// Cases not handled yet; but will handle some day.
case Stmt::DesignatedInitExprClass:
case Stmt::ExtVectorElementExprClass:
@@ -794,7 +812,6 @@
case Stmt::SizeOfPackExprClass:
case Stmt::StringLiteralClass:
case Stmt::ObjCStringLiteralClass:
- case Stmt::CXXBindTemporaryExprClass:
case Stmt::CXXPseudoDestructorExprClass:
case Stmt::SubstNonTypeTemplateParmExprClass:
case Stmt::CXXNullPtrLiteralExprClass: {
@@ -1431,6 +1448,21 @@
return;
}
+ if (const CXXBindTemporaryExpr *BTE =
+ dyn_cast<CXXBindTemporaryExpr>(Condition)) {
+ BranchNodeBuilder TempDtorBuilder(Pred, Dst, BldCtx, DstT, DstF);
+ if (Pred->getState()->contains<InitializedTemporariesSet>(BTE)) {
+ TempDtorBuilder.markInfeasible(false);
+ ProgramStateRef State = Pred->getState();
+ State = State->remove<InitializedTemporariesSet>(BTE);
+ TempDtorBuilder.generateNode(State, true, Pred);
+ } else {
+ TempDtorBuilder.markInfeasible(true);
+ TempDtorBuilder.generateNode(Pred->getState(), false, Pred);
+ }
+ return;
+ }
+
if (const Expr *Ex = dyn_cast<Expr>(Condition))
Condition = Ex->IgnoreParens();
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -193,8 +193,7 @@
(i == 4 || i == 4 ||
compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) ||
i != 4) {
- // FIXME: This shouldn't cause a warning.
- clang_analyzer_eval(true); // expected-warning{{TRUE}}
+ clang_analyzer_eval(true); // no warning, unreachable code
}
}
@@ -211,8 +210,7 @@
void testConsistencyNestedComplex(bool value) {
if (value) {
if (!value || !value || check(NoReturnDtor())) {
- // FIXME: This shouldn't cause a warning.
- clang_analyzer_eval(true); // expected-warning{{TRUE}}
+ clang_analyzer_eval(true); // no warning, unreachable code
}
}
}
@@ -225,6 +223,55 @@
}
}
}
+ // PR16664 and PR18159
+ void testConsistencyNestedComplexMidBranch(bool value) {
+ if (value) {
+ if (!value || !value || check(NoReturnDtor()) || value) {
+ clang_analyzer_eval(true); // no warning, unreachable code
+ }
+ }
+ }
+
+ // PR16664 and PR18159
+ void testConsistencyNestedComplexNestedBranch(bool value) {
+ if (value) {
+ if (!value || (!value || check(NoReturnDtor()) || value)) {
+ clang_analyzer_eval(true); // no warning, unreachable code
+ }
+ }
+ }
+
+ // PR16664 and PR18159
+ void testConsistencyNestedVariableModification(bool value) {
+ bool other = true;
+ if (value) {
+ if (!other || !value || (other = false) || check(NoReturnDtor())
+ || !other) {
+ clang_analyzer_eval(true); // no warning, unreachable code
+ }
+ }
+ }
+
+ void testTernary(bool value) {
+ if (value) {
+ bool b = value && value ? check(NoReturnDtor()) : true;
+ clang_analyzer_eval(true); // no warning, unreachable code
+ }
+ if (value) {
+ bool b = !value && !value ? true : check(NoReturnDtor());
+ clang_analyzer_eval(true); // no warning, unreachable code
+ }
+ }
+
+ void testLoop() {
+ for (int i = 0; i < 10; ++i) {
+ if (i < 3 && (i >= 2 || check(NoReturnDtor()))) {
+ // FIXME: Figure out why we don't run into this for i == 2.
+ // If we add if (i == 2 && (...)) we run into this case, though.
+ clang_analyzer_eval(true);
+ }
+ }
+ }
void testBinaryOperatorShortcut(bool value) {
if (value) {
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
