r196593 - Revert "[analyzer] Refactor conditional expression evaluating code"
Anna Zaks
ganna at apple.com
Fri Dec 6 10:56:30 PST 2013
Author: zaks
Date: Fri Dec 6 12:56:29 2013
New Revision: 196593
URL: http://llvm.org/viewvc/llvm-project?rev=196593&view=rev
Log:
Revert "[analyzer] Refactor conditional expression evaluating code"
This reverts commit r189090.
The original patch introduced regressions (see the added live-variables.* tests). The patch depends on the correctness of live variable analyses, which are not computed correctly. I've opened PR18159 to track the proper resolution to this problem.
The patch was a stepping block to r189746. This is why part of the patch reverts temporary destructor tests that started crashing. The temporary destructors feature is disabled by default.
Added:
cfe/trunk/test/Analysis/live-variables.cpp
cfe/trunk/test/Analysis/live-variables.m
Modified:
cfe/trunk/lib/Analysis/LiveVariables.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
cfe/trunk/test/Analysis/temporaries.cpp
Modified: cfe/trunk/lib/Analysis/LiveVariables.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/LiveVariables.cpp?rev=196593&r1=196592&r2=196593&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/LiveVariables.cpp (original)
+++ cfe/trunk/lib/Analysis/LiveVariables.cpp Fri Dec 6 12:56:29 2013
@@ -211,8 +211,6 @@ class TransferFunctions : public StmtVis
LiveVariables::LivenessValues &val;
LiveVariables::Observer *observer;
const CFGBlock *currentBlock;
-
- void markLogicalExprLeaves(const Expr *E);
public:
TransferFunctions(LiveVariablesImpl &im,
LiveVariables::LivenessValues &Val,
@@ -369,25 +367,9 @@ void TransferFunctions::VisitBinaryOpera
if (observer)
observer->observerKill(DR);
}
- } else if (B->isLogicalOp()) {
- // Leaf expressions in the logical operator tree are live until we reach the
- // outermost logical operator. Static analyzer relies on this behaviour.
- markLogicalExprLeaves(B->getLHS()->IgnoreParens());
- markLogicalExprLeaves(B->getRHS()->IgnoreParens());
}
}
-void TransferFunctions::markLogicalExprLeaves(const Expr *E) {
- const BinaryOperator *B = dyn_cast<BinaryOperator>(E);
- if (!B || !B->isLogicalOp()) {
- val.liveStmts = LV.SSetFact.add(val.liveStmts, E);
- return;
- }
-
- markLogicalExprLeaves(B->getLHS()->IgnoreParens());
- markLogicalExprLeaves(B->getRHS()->IgnoreParens());
-}
-
void TransferFunctions::VisitBlockExpr(BlockExpr *BE) {
AnalysisDeclContext::referenced_decls_iterator I, E;
llvm::tie(I, E) =
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=196593&r1=196592&r2=196593&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Fri Dec 6 12:56:29 2013
@@ -1385,27 +1385,11 @@ void ExprEngine::processBranch(const Stm
return;
}
- SValBuilder &SVB = Pred->getState()->getStateManager().getSValBuilder();
- SVal TrueVal = SVB.makeTruthVal(true);
- SVal FalseVal = SVB.makeTruthVal(false);
if (const Expr *Ex = dyn_cast<Expr>(Condition))
Condition = Ex->IgnoreParens();
- // If the value is already available, we don't need to do anything.
- if (Pred->getState()->getSVal(Condition, LCtx).isUnknownOrUndef()) {
- // Resolve the condition in the presence of nested '||' and '&&'.
- Condition = ResolveCondition(Condition, BldCtx.getBlock());
- }
-
- // Cast truth values to the correct type.
- if (const Expr *Ex = dyn_cast<Expr>(Condition)) {
- TrueVal = SVB.evalCast(TrueVal, Ex->getType(),
- getContext().getLogicalOperationType());
- FalseVal = SVB.evalCast(FalseVal, Ex->getType(),
- getContext().getLogicalOperationType());
- }
-
+ Condition = ResolveCondition(Condition, BldCtx.getBlock());
PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
Condition->getLocStart(),
"Error evaluating branch");
@@ -1448,37 +1432,31 @@ void ExprEngine::processBranch(const Stm
}
}
- ProgramStateRef StTrue, StFalse;
-
// If the condition is still unknown, give up.
if (X.isUnknownOrUndef()) {
-
- StTrue = PrevState->BindExpr(Condition, BldCtx.LC, TrueVal);
- StFalse = PrevState->BindExpr(Condition, BldCtx.LC, FalseVal);
-
- builder.generateNode(StTrue, true, PredI);
- builder.generateNode(StFalse, false, PredI);
+ builder.generateNode(PrevState, true, PredI);
+ builder.generateNode(PrevState, false, PredI);
continue;
}
DefinedSVal V = X.castAs<DefinedSVal>();
+
+ ProgramStateRef StTrue, StFalse;
tie(StTrue, StFalse) = PrevState->assume(V);
// Process the true branch.
if (builder.isFeasible(true)) {
- if (StTrue) {
- StTrue = StTrue->BindExpr(Condition, BldCtx.LC, TrueVal);
+ if (StTrue)
builder.generateNode(StTrue, true, PredI);
- } else
+ else
builder.markInfeasible(true);
}
// Process the false branch.
if (builder.isFeasible(false)) {
- if (StFalse) {
- StFalse = StFalse->BindExpr(Condition, BldCtx.LC, FalseVal);
+ if (StFalse)
builder.generateNode(StFalse, false, PredI);
- } else
+ else
builder.markInfeasible(false);
}
}
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp?rev=196593&r1=196592&r2=196593&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp Fri Dec 6 12:56:29 2013
@@ -505,33 +505,6 @@ void ExprEngine::VisitDeclStmt(const Dec
getCheckerManager().runCheckersForPostStmt(Dst, B.getResults(), DS, *this);
}
-static ProgramStateRef evaluateLogicalExpression(const Expr *E,
- const LocationContext *LC,
- ProgramStateRef State) {
- SVal X = State->getSVal(E, LC);
- if (! X.isUnknown())
- return State;
-
- const BinaryOperator *B = dyn_cast<BinaryOperator>(E->IgnoreParens());
- if (!B || (B->getOpcode() != BO_LAnd && B->getOpcode() != BO_LOr))
- return State;
-
- State = evaluateLogicalExpression(B->getLHS(), LC, State);
- X = State->getSVal(B->getLHS(), LC);
- QualType XType = B->getLHS()->getType();
-
- assert(X.isConstant());
- if (!X.isZeroConstant() == (B->getOpcode() == BO_LAnd)) {
- // LHS not sufficient, we need to check RHS as well
- State = evaluateLogicalExpression(B->getRHS(), LC, State);
- X = State->getSVal(B->getRHS(), LC);
- XType = B->getRHS()->getType();
- }
-
- SValBuilder &SVB = State->getStateManager().getSValBuilder();
- return State->BindExpr(E, LC, SVB.evalCast(X, B->getType(), XType));
-}
-
void ExprEngine::VisitLogicalExpr(const BinaryOperator* B, ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
assert(B->getOpcode() == BO_LAnd ||
@@ -540,25 +513,64 @@ void ExprEngine::VisitLogicalExpr(const
StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
ProgramStateRef state = Pred->getState();
- state = evaluateLogicalExpression(B, Pred->getLocationContext(), state);
- SVal X = state->getSVal(B, Pred->getLocationContext());
+ ExplodedNode *N = Pred;
+ while (!N->getLocation().getAs<BlockEntrance>()) {
+ ProgramPoint P = N->getLocation();
+ assert(P.getAs<PreStmt>()|| P.getAs<PreStmtPurgeDeadSymbols>());
+ (void) P;
+ assert(N->pred_size() == 1);
+ N = *N->pred_begin();
+ }
+ assert(N->pred_size() == 1);
+ N = *N->pred_begin();
+ BlockEdge BE = N->getLocation().castAs<BlockEdge>();
+ SVal X;
+
+ // Determine the value of the expression by introspecting how we
+ // got this location in the CFG. This requires looking at the previous
+ // block we were in and what kind of control-flow transfer was involved.
+ const CFGBlock *SrcBlock = BE.getSrc();
+ // The only terminator (if there is one) that makes sense is a logical op.
+ CFGTerminator T = SrcBlock->getTerminator();
+ if (const BinaryOperator *Term = cast_or_null<BinaryOperator>(T.getStmt())) {
+ (void) Term;
+ assert(Term->isLogicalOp());
+ assert(SrcBlock->succ_size() == 2);
+ // Did we take the true or false branch?
+ unsigned constant = (*SrcBlock->succ_begin() == BE.getDst()) ? 1 : 0;
+ X = svalBuilder.makeIntVal(constant, B->getType());
+ }
+ else {
+ // If there is no terminator, by construction the last statement
+ // in SrcBlock is the value of the enclosing expression.
+ // However, we still need to constrain that value to be 0 or 1.
+ assert(!SrcBlock->empty());
+ CFGStmt Elem = SrcBlock->rbegin()->castAs<CFGStmt>();
+ const Expr *RHS = cast<Expr>(Elem.getStmt());
+ SVal RHSVal = N->getState()->getSVal(RHS, Pred->getLocationContext());
- if (!X.isUndef()) {
- DefinedOrUnknownSVal DefinedRHS = X.castAs<DefinedOrUnknownSVal>();
- ProgramStateRef StTrue, StFalse;
- llvm::tie(StTrue, StFalse) = state->assume(DefinedRHS);
- if (StTrue) {
- if (!StFalse) {
- // The value is known to be true.
- X = getSValBuilder().makeIntVal(1, B->getType());
- } // else The truth value of X is unknown, just leave it as it is.
+ if (RHSVal.isUndef()) {
+ X = RHSVal;
} else {
- // The value is known to be false.
- assert(StFalse && "Infeasible path!");
- X = getSValBuilder().makeIntVal(0, B->getType());
+ DefinedOrUnknownSVal DefinedRHS = RHSVal.castAs<DefinedOrUnknownSVal>();
+ ProgramStateRef StTrue, StFalse;
+ llvm::tie(StTrue, StFalse) = N->getState()->assume(DefinedRHS);
+ if (StTrue) {
+ if (StFalse) {
+ // We can't constrain the value to 0 or 1.
+ // The best we can do is a cast.
+ X = getSValBuilder().evalCast(RHSVal, B->getType(), RHS->getType());
+ } else {
+ // The value is known to be true.
+ X = getSValBuilder().makeIntVal(1, B->getType());
+ }
+ } else {
+ // The value is known to be false.
+ assert(StFalse && "Infeasible path!");
+ X = getSValBuilder().makeIntVal(0, B->getType());
+ }
}
}
-
Bldr.generateNode(B, Pred, state->BindExpr(B, Pred->getLocationContext(), X));
}
Added: cfe/trunk/test/Analysis/live-variables.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/live-variables.cpp?rev=196593&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/live-variables.cpp (added)
+++ cfe/trunk/test/Analysis/live-variables.cpp Fri Dec 6 12:56:29 2013
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+class B {
+public:
+ bool m;
+ ~B() {} // The destructor ensures that the binary logical operator below is wrapped in the ExprWithCleanups.
+};
+B foo();
+int getBool();
+int *getPtr();
+int test() {
+ int r = 0;
+ for (int x = 0; x< 10; x++) {
+ int *p = getPtr();
+ // Liveness info is not computed correctly due to the following expression.
+ // This happens due to CFG being special cased for short circuit operators.
+ // PR18159
+ if (p != 0 && getBool() && foo().m && getBool()) {
+ r = *p; // no warning
+ }
+ }
+ return r;
+}
Added: cfe/trunk/test/Analysis/live-variables.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/live-variables.m?rev=196593&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/live-variables.m (added)
+++ cfe/trunk/test/Analysis/live-variables.m Fri Dec 6 12:56:29 2013
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -fobjc-arc -verify %s
+// expected-no-diagnostics
+ at interface NSObject
+ at end
+ at interface NSString : NSObject
+- (id)lastPathComponent;
+ at end
+int getBool();
+int *getPtr();
+int foo() {
+ int r = 0;
+ NSString *filename = @"filename";
+ for (int x = 0; x< 10; x++) {
+ int *p = getPtr();
+ // Liveness info is not computed correctly due to the following expression.
+ // This happens due to CFG being special cased for short circuit operators.
+ // Note, due to ObjC method call, the outermost logical operator is wrapped in ExprWithCleanups.
+ // PR18159
+ if ((p != 0) && (getBool()) && ([filename lastPathComponent]) && (getBool())) {
+ r = *p; // no-warning
+ }
+ }
+ return r;
+}
\ No newline at end of file
Modified: cfe/trunk/test/Analysis/temporaries.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/temporaries.cpp?rev=196593&r1=196592&r2=196593&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/temporaries.cpp (original)
+++ cfe/trunk/test/Analysis/temporaries.cpp Fri Dec 6 12:56:29 2013
@@ -1,6 +1,6 @@
// 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 -verify -w -analyzer-config cfg-temporary-dtors=true %s -DTEMPORARY_DTORS
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s
extern bool clang_analyzer_eval(bool);
@@ -111,17 +111,20 @@ namespace compound_literals {
}
namespace destructors {
- void testPR16664Crash() {
+ void testPR16664andPR18159Crash() {
struct Dtor {
~Dtor();
};
extern bool coin();
extern bool check(const Dtor &);
- // Don't crash here.
+#ifndef TEMPORARY_DTORS
+ // FIXME: Don't crash here when tmp dtros are enabled.
+ // PR16664 and PR18159
if (coin() && (coin() || coin() || check(Dtor()))) {
Dtor();
}
+#endif
}
#ifdef TEMPORARY_DTORS
@@ -147,9 +150,6 @@ namespace destructors {
extern bool check(const NoReturnDtor &);
void testConsistencyIf(int i) {
- if (i == 5 && (i == 4 || i == 5 || check(NoReturnDtor())))
- clang_analyzer_eval(true); // expected-warning{{TRUE}}
-
if (i != 5)
return;
if (i == 5 && (i == 4 || check(NoReturnDtor()) || i == 5)) {
@@ -170,11 +170,18 @@ namespace destructors {
clang_analyzer_eval(true); // no warning, unreachable code
}
+
+/*
+ // PR16664 and PR18159
+ FIXME: Don't crash here.
void testConsistencyNested(int i) {
extern bool compute(bool);
-
+
+ if (i == 5 && (i == 4 || i == 5 || check(NoReturnDtor())))
+ clang_analyzer_eval(true); // expected TRUE
+
if (i == 5 && (i == 4 || i == 5 || check(NoReturnDtor())))
- clang_analyzer_eval(true); // expected-warning{{TRUE}}
+ clang_analyzer_eval(true); // expected TRUE
if (i != 5)
return;
@@ -183,7 +190,7 @@ namespace destructors {
(i == 4 || compute(true) ||
compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) ||
i != 4) {
- clang_analyzer_eval(true); // expected-warning{{TRUE}}
+ clang_analyzer_eval(true); // expected TRUE
}
if (compute(i == 5 &&
@@ -192,7 +199,8 @@ namespace destructors {
i != 4) {
clang_analyzer_eval(true); // no warning, unreachable code
}
- }
+ }*/
+
#endif // TEMPORARY_DTORS
}
More information about the cfe-commits
mailing list