iAI icon indicating copy to clipboard operation
iAI copied to clipboard

Indices in uninit variables analysis

Open StamesJames opened this issue 9 months ago • 3 comments

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

StamesJames avatar Apr 29 '24 12:04 StamesJames

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 avatar May 01 '24 15:05 fabianbs96

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 avatar May 06 '24 07:05 StamesJames

@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

StamesJames avatar Sep 08 '24 11:09 StamesJames