Fix issue of global representation #1484
Conversation
| @@ -97,6 +97,7 @@ class SVFIRBuilder: public llvm::InstVisitor<SVFIRBuilder> | |||
| /// GetObject - Return the object node (stack/global/heap/function) according to a LLVM Value | |||
| inline NodeID getObjectNode(const Value* V) | |||
| { | |||
| V = LLVMUtil::getGlobalRep(V); | |||
There was a problem hiding this comment.
Global rep should always be a global. The getObject node can return a null global obj. Why adding this line?
There was a problem hiding this comment.
The reason why I add V = LLVMUtil::getGlobalRep(V); is that val is converted to global representation in collectObj to perform insert on symInfo->objSymMap
SVF/svf-llvm/lib/SymbolTableBuilder.cpp
Lines 318 to 330 in 0eae653
There was a problem hiding this comment.
Let me give you a case
There was a problem hiding this comment.
Stack is like this (pictures below may be more vivid), objSymMap is updated in above collectObj
There was a problem hiding this comment.
But I'm not sure whether this fix is accurate or not
| @@ -97,6 +97,7 @@ class SVFIRBuilder: public llvm::InstVisitor<SVFIRBuilder> | |||
| /// GetObject - Return the object node (stack/global/heap/function) according to a LLVM Value | |||
| inline NodeID getObjectNode(const Value* V) | |||
| { | |||
| V = LLVMUtil::getGlobalRep(V); | |||
| SVFValue* svfVal = LLVMModuleSet::getLLVMModuleSet()->getSVFValue(V); | |||
There was a problem hiding this comment.
I think it is good to put your V = LLVMUtil::getGlobalRep(V); into method getSVFValue
|
Follow your advise, I find we can just modify So I've pushed a new commit |
|
Since it influences all usage about global value, does it will cause new problem? |
It looks to be a clean fix for global. Would you test more and larger bcs before I merge this? |
|
I agree that more testing is needed and I will report here if I find something new. Does SVF have some unit test cases and integration test cases that can perform the testing? |
Your patch has already passed SVF tests in our CI. Would be good for you to test some large bcs and report back so I can merge your pull request |
|
ok |
#1483
This patch is not widely tested, and we need to think more about how to fix the root causes