fabianbs96
commented
2 months ago 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
newoperator 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
StamesJames
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