Remove BasicBlock Values#1841
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1841 +/- ##
==========================================
- Coverage 64.46% 64.45% -0.01%
==========================================
Files 252 252
Lines 25216 25209 -7
Branches 4767 4767
==========================================
- Hits 16255 16248 -7
Misses 8961 8961
🚀 New features to boost your workflow:
|
| } | ||
| else if (SVFUtil::isa<BasicBlock>(llvmValue)) | ||
| { | ||
| pag->addBasicBlockValNode(iter->second, llvmModuleSet()->getSVFType(llvmValue->getType())); |
There was a problem hiding this comment.
I would still keep this branch but remove "pag->addBasicBlockValNode(iter->second, llvmModuleSet()->getSVFType(llvmValue->getType()));" and put a comment.
It is better to move this branch just below the branch "if (const Function* func = SVFUtil::dyn_cast(llvmValue))", so it is clearer that we don;t handle basicblock.
There was a problem hiding this comment.
Yep okay. I'm planning later to open a PR removing ASMPCValNode. Should I do the same for its branch?
There was a problem hiding this comment.
It is good to put it under the same pr.
There was a problem hiding this comment.
Pushed its removal.
| } | ||
| else | ||
| { | ||
| pag->addDummyValNode(iter->second, icfgNode); |
There was a problem hiding this comment.
Why did you add a dummy for any unhandled node? Also good to make a comment here.
There was a problem hiding this comment.
Would this cause a new dummy node to be created for any LLVM value not on PAG?
There was a problem hiding this comment.
Yes. I'm experimenting a bit but I'm not really sure what we should do. Some observations:
From what I gather from the log, before BB nodes and ASM etc. nodes were added, I believe it was adding a standard Val node.
SVF expects the number of PAG nodes to be same or greater than the number of symbols. To ignore these symbols in the first place, well they actually exist in the IR and are referred to. For the ASM nodes, I found a case where getCalledOperand() returns an inline ASM node so it needs a corresponding Val node, but we were never properly analysing any ASM node...
So I'm conflicted. On the one hand these nodes are referred to in the IR, on the other hand our model in SVF has been ignoring them.
There was a problem hiding this comment.
I think it is best to ignore these LLVM values and do not create nodes for these values if we don't analyse them. Which ones we may analyse later?
There was a problem hiding this comment.
Dummy nodes contains a bit too many types of llvm values and if we keep these llvm values in the form of dummy nodes, it will be a bit hard to maintain later
There was a problem hiding this comment.
How should we deal with e.g this
void SVFIRBuilder::handleIndCall(CallBase* cs)
{
const CallICFGNode* cbn = llvmModuleSet()->getCallICFGNode(cs);
NodeID indFunPtrId = llvmModuleSet()->getValueNode(cs->getCalledOperand());
const_cast<CallICFGNode*>(cbn)->setIndFunPtr(pag->getGNode(indFunPtrId));
pag->addIndirectCallsites(cbn,indFunPtrId);
}
cs->getCalledOperand() returns an InlineAsm Value which is fed into getGNode.
The node has forever existed I believe, first as a generic Val node then as an InlineAsmVal node.
There was a problem hiding this comment.
I see, then we may have to keep AsmPCValNode? Let us do a removal of other unused values first and make this as an issue or a separate pull request.
There was a problem hiding this comment.
Mm okay, I will try early next week.
There was a problem hiding this comment.
Thanks, I will merge it soon. It would be good to make another pull request for AsmPCValNode later.
2 participants
They do not appear to be used anywhere.