Thanks Ted. The old code is a mystery for me.<br><br><div class="gmail_quote">On Sat, Feb 21, 2009 at 5:10 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Author: kremenek<br>
Date: Fri Feb 20 15:10:26 2009<br>
New Revision: 65160<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=65160&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=65160&view=rev</a><br>
Log:<br>
Greatly simplify the logic in ExplodedGraphImpl::TrimGraph. Now we just do a<br>
vanilla reverse-BFS followed by a forward-DFS instead of resulting to strange<br>
histrionics (whose purpose I can no longer remember) in the reverse-BFS stage.<br>
This fixes an assertion failure in BugReporter due to edge cases where no root<br>
was being hit in the reverse-BFS phase.<br>
<br>
Modified:<br>
cfe/trunk/lib/Analysis/ExplodedGraph.cpp<br>
<br>
Modified: cfe/trunk/lib/Analysis/ExplodedGraph.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ExplodedGraph.cpp?rev=65160&r1=65159&r2=65160&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ExplodedGraph.cpp?rev=65160&r1=65159&r2=65160&view=diff</a><br>
<br>
==============================================================================<br>
--- cfe/trunk/lib/Analysis/ExplodedGraph.cpp (original)<br>
+++ cfe/trunk/lib/Analysis/ExplodedGraph.cpp Fri Feb 20 15:10:26 2009<br>
@@ -127,142 +127,80 @@<br>
llvm::DenseMap<const void*, const void*> *InverseMap)<br>
const {<br>
<br>
- typedef llvm::DenseMap<const ExplodedNodeImpl*,<br>
- const ExplodedNodeImpl*> Pass1Ty;<br>
+ typedef llvm::DenseSet<const ExplodedNodeImpl*> Pass1Ty;<br>
Pass1Ty Pass1;<br>
<br>
- typedef llvm::DenseMap<const ExplodedNodeImpl*,<br>
- ExplodedNodeImpl*> Pass2Ty;<br>
-<br>
+ typedef llvm::DenseMap<const ExplodedNodeImpl*, ExplodedNodeImpl*> Pass2Ty;<br>
Pass2Ty& Pass2 = M->M;<br>
<br>
+ std::list<const ExplodedNodeImpl*> WL1;<br>
llvm::SmallVector<const ExplodedNodeImpl*, 10> WL2;<br>
<br>
- { // ===- Pass 1 (reverse BFS) -===<br>
-<br>
- // Enqueue the source nodes to the first worklist.<br>
+ // ===- Pass 1 (reverse BFS) -===<br>
+ for (const ExplodedNodeImpl* const* I = BeginSources; I != EndSources; ++I) {<br>
+ assert(*I);<br>
+ WL1.push_back(*I);<br>
+ }<br>
<br>
- std::list<std::pair<const ExplodedNodeImpl*,<br>
- const ExplodedNodeImpl*> > WL1, WL1_Loops;<br>
-<br>
- for (const ExplodedNodeImpl* const* I = BeginSources; I != EndSources; ++I){<br>
- assert(*I);<br>
- WL1.push_back(std::make_pair(*I, *I));<br>
- }<br>
+ // Process the first worklist until it is empty. Because it is a std::list<br>
+ // it acts like a FIFO queue.<br>
+ while (!WL1.empty()) {<br>
+ const ExplodedNodeImpl *N = WL1.back();<br>
+ WL1.pop_back();<br>
<br>
- // Process the worklist.<br>
+ // Have we already visited this node? If so, continue to the next one.<br>
+ if (Pass1.count(N))<br>
+ continue;<br>
<br>
- while (!WL1.empty() || !WL1_Loops.empty()) {<br>
- // Only dequeue from the "loops" worklist if WL1 has no items.<br>
- // Thus we prioritize for paths that don't span loop boundaries.<br>
- const ExplodedNodeImpl *N = 0, *Src = 0;<br>
-<br>
- if (WL1.empty()) {<br>
- assert(!WL1_Loops.empty());<br>
- N = WL1_Loops.back().first;<br>
- assert(N);<br>
- Src = WL1_Loops.back().second;<br>
- WL1_Loops.pop_back();<br>
- }<br>
- else {<br>
- N = WL1.back().first;<br>
- assert(N);<br>
- Src = WL1.back().second;<br>
- WL1.pop_back();<br>
- }<br>
-<br>
- if (Pass1.find(N) != Pass1.end())<br>
- continue;<br>
-<br>
- bool PredHasSameSource = false;<br>
- bool VisitPreds = true;<br>
-<br>
- for (ExplodedNodeImpl** I=N->Preds.begin(), **E=N->Preds.end();<br>
- I!=E; ++I) {<br>
-<br>
- Pass1Ty::iterator pi = Pass1.find(*I);<br>
-<br>
- if (pi == Pass1.end())<br>
- continue;<br>
-<br>
- VisitPreds = false;<br>
-<br>
- if (pi->second == Src) {<br>
- PredHasSameSource = true;<br>
- break;<br>
- }<br>
- }<br>
-<br>
- if (VisitPreds || !PredHasSameSource) {<br>
-<br>
- Pass1[N] = Src;<br>
-<br>
- if (N->Preds.empty()) {<br>
- WL2.push_back(N);<br>
- continue;<br>
- }<br>
- }<br>
- else<br>
- Pass1[N] = NULL;<br>
-<br>
- if (VisitPreds)<br>
- for (ExplodedNodeImpl** I=N->Preds.begin(), **E=N->Preds.end();<br>
- I!=E; ++I) {<br>
-<br>
- ProgramPoint P = Src->getLocation();<br>
-<br>
- if (const BlockEdge *BE = dyn_cast<BlockEdge>(&P))<br>
- if (Stmt* T = BE->getSrc()->getTerminator())<br>
- switch (T->getStmtClass()) {<br>
- default: break;<br>
- case Stmt::ForStmtClass:<br>
- case Stmt::WhileStmtClass:<br>
- case Stmt::DoStmtClass:<br>
- assert(*I);<br>
- WL1_Loops.push_front(std::make_pair(*I, Src));<br>
- continue;<br>
-<br>
- }<br>
-<br>
- assert(*I);<br>
- WL1.push_front(std::make_pair(*I, Src));<br>
- }<br>
+ // Otherwise, mark this node as visited.<br>
+ Pass1.insert(N);<br>
+<br>
+ // If this is a root enqueue it to the second worklist.<br>
+ if (N->Preds.empty()) {<br>
+ WL2.push_back(N);<br>
+ continue;<br>
}<br>
+<br>
+ // Visit our predecessors and enqueue them.<br>
+ for (ExplodedNodeImpl** I=N->Preds.begin(), **E=N->Preds.end(); I!=E; ++I)<br>
+ WL1.push_front(*I);<br>
}<br>
<br>
+ // We didn't hit a root? Return with a null pointer for the new graph.<br>
if (WL2.empty())<br>
return 0;<br>
<br>
+ // Create an empty graph.<br>
ExplodedGraphImpl* G = MakeEmptyGraph();<br>
<br>
- // ===- Pass 2 (forward DFS to construct the new graph) -===<br>
-<br>
+ // ===- Pass 2 (forward DFS to construct the new graph) -===<br>
while (!WL2.empty()) {<br>
-<br>
const ExplodedNodeImpl* N = WL2.back();<br>
WL2.pop_back();<br>
<br>
// Skip this node if we have already processed it.<br>
-<br>
if (Pass2.find(N) != Pass2.end())<br>
continue;<br>
<br>
- // Create the corresponding node in the new graph.<br>
-<br>
+ // Create the corresponding node in the new graph and record the mapping<br>
+ // from the old node to the new node.<br>
ExplodedNodeImpl* NewN = G->getNodeImpl(N->getLocation(), N->State, NULL);<br>
Pass2[N] = NewN;<br>
+<br>
+ // Also record the reverse mapping from the new node to the old node.<br>
if (InverseMap) (*InverseMap)[NewN] = N;<br>
<br>
+ // If this node is a root, designate it as such in the graph.<br>
if (N->Preds.empty())<br>
G->addRoot(NewN);<br>
<br>
// In the case that some of the intended predecessors of NewN have already<br>
// been created, we should hook them up as predecessors.<br>
-<br>
+<br>
+ // Walk through the predecessors of 'N' and hook up their corresponding<br>
+ // nodes in the new graph (if any) to the freshly created node.<br>
for (ExplodedNodeImpl **I=N->Preds.begin(), **E=N->Preds.end(); I!=E; ++I) {<br>
-<br>
Pass2Ty::iterator PI = Pass2.find(*I);<br>
-<br>
if (PI == Pass2.end())<br>
continue;<br>
<br>
@@ -273,26 +211,19 @@<br>
// been created, we should hook them up as successors. Otherwise, enqueue<br>
// the new nodes from the original graph that should have nodes created<br>
// in the new graph.<br>
-<br>
for (ExplodedNodeImpl **I=N->Succs.begin(), **E=N->Succs.end(); I!=E; ++I) {<br>
-<br>
- Pass2Ty::iterator PI = Pass2.find(*I);<br>
-<br>
+ Pass2Ty::iterator PI = Pass2.find(*I);<br>
if (PI != Pass2.end()) {<br>
PI->second->addPredecessor(NewN);<br>
continue;<br>
}<br>
<br>
// Enqueue nodes to the worklist that were marked during pass 1.<br>
-<br>
- Pass1Ty::iterator pi = Pass1.find(*I);<br>
-<br>
- if (pi == Pass1.end() || pi->second == NULL)<br>
- continue;<br>
-<br>
- WL2.push_back(*I);<br>
+ if (Pass1.count(*I))<br>
+ WL2.push_back(*I);<br>
}<br>
<br>
+ // Finally, explictly mark all nodes without any successors as sinks.<br>
if (N->isSink())<br>
NewN->markAsSink();<br>
}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br>