<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Awesome!<div><br><div><div>On Mar 22, 2013, at 2:15 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">Author: jrose<br>Date: Fri Mar 22 16:15:28 2013<br>New Revision: 177764<br><br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=177764&view=rev">http://llvm.org/viewvc/llvm-project?rev=177764&view=rev</a><br>Log:<br>[analyzer] Use a forward BFS instead of a reverse BFS to find shortest paths.<br><br>For a given bug equivalence class, we'd like to emit the report with the<br>shortest path. So far to do this we've been trimming the ExplodedGraph to<br>only contain relevant nodes, then doing a reverse BFS (starting at all the<br>error nodes) to find the shortest paths from the root. However, this is<br>fairly expensive when we are suppressing many bug reports in the same<br>equivalence class.<br><br>r177468-9 tried to solve this problem by breaking cycles during graph<br>trimming, then updating the BFS priorities after each suppressed report<br>instead of recomputing the whole thing. However, breaking cycles is not<br>a cheap operation because an analysis graph minus cycles is still a DAG,<br>not a tree.<br><br>This fix changes the algorithm to do a single forward BFS (starting from the<br>root) and to use that to choose the report with the shortest path by looking<br>at the error nodes with the lowest BFS priorities. This was Anna's idea, and<br>has the added advantage of requiring no update step: we can just pick the<br>error node with the next lowest priority to produce the next bug report.<br><br><<a href="rdar://problem/13474689">rdar://problem/13474689</a>><br><br>Modified:<br> cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp<br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=177764&r1=177763&r2=177764&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=177764&r1=177763&r2=177764&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Fri Mar 22 16:15:28 2013<br>@@ -1878,220 +1878,167 @@ namespace {<br>/// node maps.<br>class ReportGraph {<br>public:<br>- OwningPtr<ExplodedGraph> Graph;<br> InterExplodedGraphMap BackMap;<br>- ExplodedNode *ErrorNode;<br>+ OwningPtr<ExplodedGraph> Graph;<br>+ const ExplodedNode *ErrorNode;<br> size_t Index;<br>};<br><br>/// A wrapper around a trimmed graph and its node maps.<br>class TrimmedGraph {<br>- InterExplodedGraphMap ForwardMap;<br> InterExplodedGraphMap InverseMap;<br><br> typedef llvm::DenseMap<const ExplodedNode *, unsigned> PriorityMapTy;<br> PriorityMapTy PriorityMap;<br><br>+ typedef std::pair<const ExplodedNode *, size_t> NodeIndexPair;<br>+ SmallVector<NodeIndexPair, 32> ReportNodes;<br>+<br> OwningPtr<ExplodedGraph> G;<br>- const ExplodedNode *Root;<br>+<br>+ /// A helper class for sorting ExplodedNodes by priority.<br>+ template <bool Descending><br>+ class PriorityCompare {<br>+ const PriorityMapTy &PriorityMap;<br>+<br>+ public:<br>+ PriorityCompare(const PriorityMapTy &M) : PriorityMap(M) {}<br>+<br>+ bool operator()(const ExplodedNode *LHS, const ExplodedNode *RHS) const {<br>+ PriorityMapTy::const_iterator LI = PriorityMap.find(LHS);<br>+ PriorityMapTy::const_iterator RI = PriorityMap.find(RHS);<br>+ PriorityMapTy::const_iterator E = PriorityMap.end();<br>+<br>+ if (LI == E)<br>+ return Descending;<br>+ if (RI == E)<br>+ return !Descending;<br>+<br>+ return Descending ? LI->second > RI->second<br>+ : LI->second < RI->second;<br>+ }<br>+<br>+ bool operator()(const NodeIndexPair &LHS, const NodeIndexPair &RHS) const {<br>+ return (*this)(LHS.first, RHS.first);<br>+ }<br>+ };<br>+<br>public:<br> TrimmedGraph(const ExplodedGraph *OriginalGraph,<br> ArrayRef<const ExplodedNode *> Nodes);<br><br>- void createBestReportGraph(ArrayRef<const ExplodedNode *> Nodes,<br>- ReportGraph &GraphWrapper) const;<br>-<br>- void removeErrorNode(const ExplodedNode *Node);<br>+ bool popNextReportGraph(ReportGraph &GraphWrapper);<br>};<br>-<br>}<br><br>TrimmedGraph::TrimmedGraph(const ExplodedGraph *OriginalGraph,<br> ArrayRef<const ExplodedNode *> Nodes) {<br> // The trimmed graph is created in the body of the constructor to ensure<br> // that the DenseMaps have been initialized already.<br>- G.reset(OriginalGraph->trim(Nodes, /*BreakCycles=*/true,<br>+ InterExplodedGraphMap ForwardMap;<br>+ G.reset(OriginalGraph->trim(Nodes, /*BreakCycles=*/false,<br> &ForwardMap, &InverseMap));<br><br> // Find the (first) error node in the trimmed graph. We just need to consult<br> // the node map which maps from nodes in the original graph to nodes<br> // in the new graph.<br>- std::queue<std::pair<const ExplodedNode *, unsigned> > WS;<br>- typedef llvm::SmallDenseMap<const ExplodedNode *, size_t, 32> IndexMapTy;<br>- IndexMapTy IndexMap(llvm::NextPowerOf2(Nodes.size() + 1));<br>+ llvm::SmallPtrSet<const ExplodedNode *, 32> RemainingNodes;<br><br> for (unsigned i = 0, count = Nodes.size(); i < count; ++i) {<br>- const ExplodedNode *OriginalNode = Nodes[i];<br>- if (const ExplodedNode *N = ForwardMap.lookup(OriginalNode)) {<br>- WS.push(std::make_pair(N, 0));<br>- IndexMap[OriginalNode] = i;<br>+ if (const ExplodedNode *NewNode = ForwardMap.lookup(Nodes[i])) {<br>+ ReportNodes.push_back(std::make_pair(NewNode, i));<br>+ RemainingNodes.insert(NewNode);<br> }<br> }<br><br>- assert(!WS.empty() && "No error node found in the trimmed graph.");<br>+ assert(!RemainingNodes.empty() && "No error node found in the trimmed graph");<br>+<br>+ // Perform a forward BFS to find all the shortest paths.<br>+ std::queue<const ExplodedNode *> WS;<br>+<br>+ assert(G->num_roots() == 1);<br>+ WS.push(*G->roots_begin());<br>+ unsigned Priority = 0;<br><br>- // Perform a reverse BFS to find all the shortest paths.<br>- Root = 0;<br> while (!WS.empty()) {<br>- const ExplodedNode *Node;<br>- unsigned Priority;<br>- llvm::tie(Node, Priority) = WS.front();<br>+ const ExplodedNode *Node = WS.front();<br> WS.pop();<br><br> PriorityMapTy::iterator PriorityEntry;<br> bool IsNew;<br> llvm::tie(PriorityEntry, IsNew) =<br> PriorityMap.insert(std::make_pair(Node, Priority));<br>+ ++Priority;<br><br> if (!IsNew) {<br> assert(PriorityEntry->second <= Priority);<br> continue;<br> }<br><br>- if (Node->pred_empty()) {<br>- assert(!Root && "more than one root");<br>- Root = Node;<br>- }<br>+ if (RemainingNodes.erase(Node))<br>+ if (RemainingNodes.empty())<br>+ break;<br><br>- for (ExplodedNode::const_pred_iterator I = Node->pred_begin(),<br>- E = Node->pred_end();<br>+ for (ExplodedNode::const_pred_iterator I = Node->succ_begin(),<br>+ E = Node->succ_end();<br> I != E; ++I)<br>- WS.push(std::make_pair(*I, Priority + 1));<br>+ WS.push(*I);<br> }<br>- <br>- assert(Root);<br>-}<br>-<br>-void TrimmedGraph::createBestReportGraph(ArrayRef<const ExplodedNode *> Nodes,<br>- ReportGraph &GraphWrapper) const {<br>- assert(!GraphWrapper.Graph && "ReportGraph is already in use");<br>- assert(GraphWrapper.BackMap.empty() && "ReportGraph is already in use");<br><br>- // Find the (first) error node in the trimmed graph. We just need to consult<br>- // the node map which maps from nodes in the original graph to nodes<br>- // in the new graph.<br>- std::queue<std::pair<const ExplodedNode *, unsigned> > WS;<br>- typedef llvm::SmallDenseMap<const ExplodedNode *, size_t, 32> IndexMapTy;<br>- IndexMapTy IndexMap(llvm::NextPowerOf2(Nodes.size() + 1));<br>+ // Sort the error paths from longest to shortest.<br>+ std::sort(ReportNodes.begin(), ReportNodes.end(),<br>+ PriorityCompare<true>(PriorityMap));<br>+}<br><br>- for (unsigned i = 0, count = Nodes.size(); i < count; ++i) {<br>- const ExplodedNode *OriginalNode = Nodes[i];<br>- if (const ExplodedNode *N = ForwardMap.lookup(OriginalNode)) {<br>- WS.push(std::make_pair(N, 0));<br>- IndexMap[OriginalNode] = i;<br>- }<br>- }<br>+bool TrimmedGraph::popNextReportGraph(ReportGraph &GraphWrapper) {<br>+ if (ReportNodes.empty())<br>+ return false;<br><br>- assert(!WS.empty() && "No error node found in the trimmed graph.");<br>+ const ExplodedNode *OrigN;<br>+ llvm::tie(OrigN, GraphWrapper.Index) = ReportNodes.pop_back_val();<br>+ assert(PriorityMap.find(OrigN) != PriorityMap.end() &&<br>+ "error node not accessible from root");<br><br> // Create a new graph with a single path. This is the graph<br> // that will be returned to the caller.<br> ExplodedGraph *GNew = new ExplodedGraph();<br> GraphWrapper.Graph.reset(GNew);<br>+ GraphWrapper.BackMap.clear();<br><br>- // Now walk from the root down the BFS path, always taking the successor<br>- // with the lowest number.<br>- ExplodedNode *Last = 0;<br>- for ( const ExplodedNode *N = Root ;;) {<br>- // Lookup the number associated with the current node.<br>- PriorityMapTy::const_iterator I = PriorityMap.find(N);<br>- assert(I != PriorityMap.end());<br>-<br>+ // Now walk from the error node up the BFS path, always taking the<br>+ // predeccessor with the lowest number.<br>+ ExplodedNode *Succ = 0;<br>+ while (true) {<br> // Create the equivalent node in the new graph with the same state<br> // and location.<br>- ExplodedNode *NewN = GNew->getNode(N->getLocation(), N->getState());<br>+ ExplodedNode *NewN = GNew->getNode(OrigN->getLocation(), OrigN->getState());<br><br> // Store the mapping to the original node.<br>- InterExplodedGraphMap::const_iterator IMitr = InverseMap.find(N);<br>+ InterExplodedGraphMap::const_iterator IMitr = InverseMap.find(OrigN);<br> assert(IMitr != InverseMap.end() && "No mapping to original node.");<br> GraphWrapper.BackMap[NewN] = IMitr->second;<br><br> // Link up the new node with the previous node.<br>- if (Last)<br>- NewN->addPredecessor(Last, *GNew);<br>+ if (Succ)<br>+ Succ->addPredecessor(NewN, *GNew);<br>+ else<br>+ GraphWrapper.ErrorNode = NewN;<br><br>- Last = NewN;<br>+ Succ = NewN;<br><br> // Are we at the final node?<br>- IndexMapTy::iterator IMI = IndexMap.find(IMitr->second);<br>- if (IMI != IndexMap.end()) {<br>- GraphWrapper.ErrorNode = NewN;<br>- GraphWrapper.Index = IMI->second;<br>+ if (OrigN->pred_empty()) {<br>+ GNew->addRoot(NewN);<br> break;<br> }<br><br>- // Find the next successor node. We choose the node that is marked<br>+ // Find the next predeccessor node. We choose the node that is marked<br> // with the lowest BFS number.<br>- unsigned MinVal = -1U;<br>- for (ExplodedNode::const_succ_iterator SI = N->succ_begin(),<br>- SE = N->succ_end();<br>- SI != SE; ++SI) {<br>- I = PriorityMap.find(*SI);<br>-<br>- if (I == PriorityMap.end())<br>- continue;<br>-<br>- if (I->second < MinVal) {<br>- N = *SI;<br>- MinVal = I->second;<br>- }<br>- }<br>-<br>- assert(MinVal != -1U);<br>+ OrigN = *std::min_element(OrigN->pred_begin(), OrigN->pred_end(),<br>+ PriorityCompare<false>(PriorityMap));<br> }<br>-}<br>-<br>-void TrimmedGraph::removeErrorNode(const ExplodedNode *ErrorNode) {<br>- ErrorNode = ForwardMap[ErrorNode];<br>- assert(ErrorNode && "not an error node");<br>-<br>- PriorityMapTy::iterator PriorityEntry = PriorityMap.find(ErrorNode);<br>- assert(PriorityEntry != PriorityMap.end() && "error node already removed");<br>- PriorityMap.erase(PriorityEntry);<br><br>- std::queue<const ExplodedNode *> WS;<br>- for (ExplodedNode::const_pred_iterator PI = ErrorNode->pred_begin(),<br>- PE = ErrorNode->pred_end();<br>- PI != PE; ++PI) {<br>- assert(PriorityMap.find(*PI) != PriorityMap.end() && "predecessor removed");<br>- WS.push(*PI);<br>- }<br>-<br>- // Update all nodes possibly affected by this change.<br>- while (!WS.empty()) {<br>- const ExplodedNode *N = WS.front();<br>- WS.pop();<br>-<br>- PriorityEntry = PriorityMap.find(N);<br>-<br>- // Did we process this node already and find it unreachable?<br>- if (PriorityEntry == PriorityMap.end())<br>- continue;<br>-<br>- unsigned MinPriority = -1U;<br>- for (ExplodedNode::const_succ_iterator SI = N->succ_begin(),<br>- SE = N->succ_end();<br>- SI != SE; ++SI) {<br>- PriorityMapTy::iterator SuccEntry = PriorityMap.find(*SI);<br>- if (SuccEntry == PriorityMap.end())<br>- continue;<br>- MinPriority = std::min(SuccEntry->second, MinPriority);<br>- }<br>-<br>- if (MinPriority == -1U)<br>- PriorityMap.erase(N);<br>- else if (PriorityMap[N] == MinPriority + 1)<br>- continue;<br>- else<br>- PriorityMap[N] = MinPriority + 1;<br>-<br>- for (ExplodedNode::const_pred_iterator PI = N->pred_begin(),<br>- PE = N->pred_end();<br>- PI != PE; ++PI) {<br>- assert(PriorityMap.find(*PI) != PriorityMap.end() && "premature removal");<br>- WS.push(*PI);<br>- }<br>- }<br>+ return true;<br>}<br><br><br>@@ -2197,6 +2144,7 @@ bool GRBugReporter::generatePathDiagnost<br> assert(!bugReports.empty());<br><br> bool HasValid = false;<br>+ bool HasInvalid = false;<br> SmallVector<const ExplodedNode *, 32> errorNodes;<br> for (ArrayRef<BugReport*>::iterator I = bugReports.begin(),<br> E = bugReports.end(); I != E; ++I) {<br>@@ -2204,6 +2152,8 @@ bool GRBugReporter::generatePathDiagnost<br> HasValid = true;<br> errorNodes.push_back((*I)->getErrorNode());<br> } else {<br>+ // Keep the errorNodes list in sync with the bugReports list.<br>+ HasInvalid = true;<br> errorNodes.push_back(0);<br> }<br> }<br>@@ -2217,22 +2167,15 @@ bool GRBugReporter::generatePathDiagnost<br> PathGenerationScheme ActiveScheme = PC.getGenerationScheme();<br><br> TrimmedGraph TrimG(&getGraph(), errorNodes);<br>+ ReportGraph ErrorGraph;<br><br>- for (size_t Remaining = bugReports.size(); Remaining > 0; --Remaining) {<br>- // Construct a new graph that contains only a single path from the error<br>- // node to a root.<br>- ReportGraph ErrorGraph;<br>- TrimG.createBestReportGraph(errorNodes, ErrorGraph);<br>-<br>+ while (TrimG.popNextReportGraph(ErrorGraph)) {<br> // Find the BugReport with the original location.<br> assert(ErrorGraph.Index < bugReports.size());<br> BugReport *R = bugReports[ErrorGraph.Index];<br> assert(R && "No original report found for sliced graph.");<br> assert(R->isValid() && "Report selected by trimmed graph marked invalid.");<br><br>- // Don't try to reuse this report if it ends up being suppressed.<br>- errorNodes[ErrorGraph.Index] = 0;<br>-<br> // Start building the path diagnostic...<br> PathDiagnosticBuilder PDB(*this, R, ErrorGraph.BackMap, &PC);<br> const ExplodedNode *N = ErrorGraph.ErrorNode;<br>@@ -2297,10 +2240,8 @@ bool GRBugReporter::generatePathDiagnost<br> finalReportConfigToken = R->getConfigurationChangeToken();<br> } while (finalReportConfigToken != origReportConfigToken);<br><br>- if (!R->isValid()) {<br>- TrimG.removeErrorNode(R->getErrorNode());<br>+ if (!R->isValid())<br> continue;<br>- }<br><br> // Finally, prune the diagnostic path of uninteresting stuff.<br> if (!PD.path.empty()) {<br>@@ -2322,6 +2263,8 @@ bool GRBugReporter::generatePathDiagnost<br> }<br><br> // We suppressed all the reports in this equivalence class.<br>+ assert(!HasInvalid && "Inconsistent suppression");<br>+ (void)HasInvalid;<br> return false;<br>}<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">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div></blockquote></div><br></div></body></html>