Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix incorrect implementation of getting CallGraphSCCRevTopoOrder #1640

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions svf/include/Graphs/SCC.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,30 @@ class SCCDetection


// Return a handle to the stack of nodes in topological
// order. This will be used to seed the initial solution
// order. This will be used to seed the initial solution
// and improve efficiency.
inline GNodeStack &topoNodeStack()
{
return _T;
}

const inline GNODESCCInfoMap &GNodeSCCInfo() const
/// Return a handle to the stack of nodes in reverse topological
/// order. This will be used to seed the initial solution
/// and improve efficiency.
inline GNodeStack revTopoNodeStack()
{
GNodeStack revTopoOrder;
GNodeStack &topoOrder = topoNodeStack();
while(!topoOrder.empty())
{
NodeID callgraphNodeID = topoOrder.top();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callgraphNodeID => nodeID

topoOrder.pop();
revTopoOrder.push(callgraphNodeID);
}
return revTopoOrder;
}

const inline GNODESCCInfoMap &GNodeSCCInfo() const
{
return _NodeSCCAuxInfo;
}
Expand Down
24 changes: 8 additions & 16 deletions svf/include/WPA/WPAFSSolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,31 +69,23 @@ class WPAFSSolver : public WPASolver<GraphType>
/// SCC detection
this->getSCCDetector()->find();

assert(nodeStack.empty() && "node stack is not empty, some nodes are not popped properly.");

/// Both rep and sub nodes need to be processed later.
/// Collect sub nodes from SCCDetector.
NodeStack revTopoStack;
NodeStack& topoStack = this->getSCCDetector()->topoNodeStack();
while (!topoStack.empty())
NodeStack revTopoStack = this->getSCCDetector()->revTopoNodeStack();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why making changes on SCCDetect ? It was correct, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems correct.

while (!revTopoStack.empty())
{
NodeID nodeId = topoStack.top();
topoStack.pop();
NodeID nodeId = revTopoStack.top();
revTopoStack.pop();
const NodeBS& subNodes = this->getSCCDetector()->subNodes(nodeId);
for (NodeBS::iterator it = subNodes.begin(), eit = subNodes.end(); it != eit; ++it)
{
revTopoStack.push(*it);
/// restore the topological order.
nodeStack.push(*it);
}
}

assert(nodeStack.empty() && "node stack is not empty, some nodes are not popped properly.");

/// restore the topological order.
while (!revTopoStack.empty())
{
NodeID nodeId = revTopoStack.top();
revTopoStack.pop();
nodeStack.push(nodeId);
}

return nodeStack;
}
};
Expand Down
10 changes: 5 additions & 5 deletions svf/lib/MSSA/MemRegion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ void MRGenerator::collectModRefForCall()
{
PTACallGraphNode* subCallGraphNode = callGraph->getCallGraphNode(*it);
/// Get mod-ref of all callsites calling callGraphNode
std::cout << subCallGraphNode->getFunction()->getName() << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this debugging line.

modRefAnalysis(subCallGraphNode,worklist);
}
}
Expand Down Expand Up @@ -461,12 +462,11 @@ bool MRGenerator::addModSideEffectOfCallSite(const CallICFGNode* cs, const NodeB
*/
void MRGenerator::getCallGraphSCCRevTopoOrder(WorkList& worklist)
{

NodeStack& topoOrder = callGraphSCC->topoNodeStack();
while(!topoOrder.empty())
NodeStack revTopoNodeStack = callGraphSCC->revTopoNodeStack();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@canliture I think we could just revise this method by using one line: worlist = callGraphSCC->revTopoNodeStack();

could you validate the correctness again?

while(!revTopoNodeStack.empty())
{
NodeID callgraphNodeID = topoOrder.top();
topoOrder.pop();
NodeID callgraphNodeID = revTopoNodeStack.top();
revTopoNodeStack.pop();
worklist.push(callgraphNodeID);
}
}
Expand Down
22 changes: 9 additions & 13 deletions svf/lib/WPA/Andersen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -709,24 +709,20 @@ inline void Andersen::collapseFields()
*/
void Andersen::mergeSccCycle()
{
NodeStack revTopoOrder;
NodeStack & topoOrder = getSCCDetector()->topoNodeStack();
while (!topoOrder.empty())
NodeStack revTopoOrder = getSCCDetector()->revTopoNodeStack();
/// referenced topoOrder is used for restoring the topological order for later solving.
NodeStack &topoOrder = getSCCDetector()->topoNodeStack();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

topoOrder has been there when you retrieve it and should not be changed later at line 725.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line: NodeStack &topoOrder = getSCCDetector()->topoNodeStack()

while (!revTopoOrder.empty())
{
NodeID repNodeId = topoOrder.top();
topoOrder.pop();
revTopoOrder.push(repNodeId);
NodeID repNodeId = revTopoOrder.top();
revTopoOrder.pop();

const NodeBS& subNodes = getSCCDetector()->subNodes(repNodeId);
// merge sub nodes to rep node
mergeSccNodes(repNodeId, subNodes);
}

// restore the topological order for later solving.
while (!revTopoOrder.empty())
{
NodeID nodeId = revTopoOrder.top();
revTopoOrder.pop();
topoOrder.push(nodeId);
/// restore the topological order for later solving.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no restore is needed as you never popped any element in topoOrder

topoOrder.push(repNodeId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line topoOrder.push(repNodeId);

}
}

Expand Down
Loading