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

Indices in uninit variables analysis #720

Open
StamesJames opened this issue Apr 29, 2024 · 3 comments
Open

Indices in uninit variables analysis #720

StamesJames opened this issue Apr 29, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@StamesJames
Copy link
Contributor

I tried some phasar-cli analysis on rust code. On a simple hello-world program I got multiple unused variable notifications. When I looked at the llvm code those often where there because phasar hasn't tracked the indices that where already initialized and therefore defined an InsertValue operation as undefined as a whole, although just some indices are undefined.

I have started to implement a solution for this that I wanted to share here. I wrote a IndexWrapper class that acts as the dataflow fact and stores what indices inside a llvm::Value are the Facts.
While implementing this I also changed the current implementation of the uninit variables analysis to use the lambdaFlow function instead of local structs so maybe this could also be interesting for PR #616

I'm currently not sure if I handle the GetElementPtrInst right and there is no alias analysis at the moment.

I add my current implementations here:

PhASAR-Uninit-Indexed-Variables.zip

@fabianbs96
Copy link
Member

Hi @StamesJames, thank you for your contribution; it looks quite promising.

However, when reviewing the code, I realized, there are some issues:

  • Memory Leaks: You allocate many objects with the new operator and not assigning an owner. This will likely exhaust your RAM quickly. Maybe consider using RAII types instead.
  • Termination: In your flow functions you are allocating new data-flow facts (IndexWrapper) on the heap. Hence, when encountering a loop in the CFG, your program may constantly generate new facts (that all look the same) and not terminate. A better solution in this case may be using value types or using a deduplicating cache.
  • Stack-use after Scope: in IndexWrapper.cpp:29 you create and store an ArrayRef initialized with a temporary reference to the parameter index. This will lead to a stack-use after scope error
  • some other minor things

Can you maybe create a fork of phasar to put your implementation there? This would make reviews easier

@fabianbs96 fabianbs96 added the enhancement New feature or request label May 1, 2024
@StamesJames
Copy link
Contributor Author

Thanks a lot for the review @fabianbs96. I'm quite new to C++ so I already thought there will be a lot of c++ beginner mistakes. I will make a fork and try to fix the issues.

@StamesJames
Copy link
Contributor Author

@fabianbs96 I have finally worked on it again but I ran into a problem. I tried to compile my analysis as a tool, but I have the problem, that there is no DtoString function for my IndexWrapper. I tried to change the emitTextReport function so it dosn't call DtoString but it is getting called in IDESolver and when I tried to implement it for the Indexwarper I got linking problems. Do you know how to properly implement it so that it can be linked in the rest of phasar?
My current version is in my fork at https://github.com/StamesJames/phasar/tree/indexed-uninit-analysis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants