- Notifications
You must be signed in to change notification settings - Fork14.5k
[LifetimeSafety] Add loan expiry analysis#148712
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:users/usx95/07-16-lifetime-safety-add-unit-tests
Are you sure you want to change the base?
[LifetimeSafety] Add loan expiry analysis#148712
Conversation
usx95 commentedJul 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stackon Graphite.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
880cf3b toed73e3bComparec217d0d to702d255Compareed73e3b toa50b00eCompare7e098b0 to793d58eComparea50b00e toa9f1e8dCompare793d58e to2bff132Compare6ce3173 to9265536Compare2253ff0 toc073ef3Compare9265536 to9da8f3aComparec073ef3 to51afc3eCompare9da8f3a to04d6193Compare51afc3e to7b26a72Compare04d6193 to2073937Compare5f09913 toaca68c2Compareb109b6a to7502ee5Compare8bbae58 to037a7ecCompareLooks good, but what's the plan for tests? |
Good question :) I think adding tests for each analyses to the currentllvm-lit tests is quite a pain for the eyes at this point. Also I want to test things at specific program points (is loan expired here, does origin contain loan here, etc). I am exploring a couple of options, primarilyhttps://github.com/llvm/llvm-project/blob/main/clang/include/clang/Testing/TestAST.h,https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/FlowSensitive/TestingSupport.h and something likehttps://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp#L874-L900 This will likely take some more time. Happy to hold this off until I add something like this for unit tests! I would still rely on llvm-lit tests for final diagnostic reporting (or maybe that also can be done through unit testing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Once the testing story is figured out this looks good to me.
| }; | ||
| /// The analysis that tracks which loans have expired. | ||
| class ExpiredLoansAnalysis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So we have the expectation that we have a tight bound on this analysis. I wonder if there is a way to somehow add an assert to verify that the reality matches our expectations. Not super important but if it is not too complicated it could be nice.
We can also defer this to a later PR since we want to be able to add strict bounds to the number of iterations in the future and that might be required for us to easily assert on this.
Alright. I will add some unit test infra separately before this PR then! |
037a7ec toa4cba20Compare9475733 to7762a38CompareRefactored the lifetime safety analysis to use a generic dataflow framework with a policy-based design.### Changes- Introduced a generic `DataflowAnalysis` template class that can be specialized for different analyses- Renamed `LifetimeLattice` to `LoanPropagationLattice` to better reflect its purpose- Created a `LoanPropagationAnalysis` class that inherits from the generic framework- Moved transfer functions from the standalone `Transferer` class into the analysis class- Restructured the code to separate the dataflow engine from the specific analysis logic- Updated debug output and test expectations to use the new class names### MotivationIn order to add more analyses, e.g. [loan expiry](#148712) and origin liveness, the previous implementation would have separate, nearly identical dataflow runners for each analysis. This change creates a single, reusable component, which will make it much simpler to add subsequent analyses without repeating boilerplate code.This is quite close to the existing dataflow framework!
4a23369 tob76c916Comparea4cba20 to7365fc4CompareRefactored the lifetime safety analysis to use a generic dataflow framework with a policy-based design.### Changes- Introduced a generic `DataflowAnalysis` template class that can be specialized for different analyses- Renamed `LifetimeLattice` to `LoanPropagationLattice` to better reflect its purpose- Created a `LoanPropagationAnalysis` class that inherits from the generic framework- Moved transfer functions from the standalone `Transferer` class into the analysis class- Restructured the code to separate the dataflow engine from the specific analysis logic- Updated debug output and test expectations to use the new class names### MotivationIn order to add more analyses, e.g. [loan expiry](llvm/llvm-project#148712) and origin liveness, the previous implementation would have separate, nearly identical dataflow runners for each analysis. This change creates a single, reusable component, which will make it much simpler to add subsequent analyses without repeating boilerplate code.This is quite close to the existing dataflow framework!
7365fc4 toa7d03b1Comparea7d03b1 tod5b093eCompared5b093e to70b63edCompare16fccbd to3e0b53fCompare9a3a69a to8c5c8f1Compare3e0b53f toe6fc855Compare8c5c8f1 toc29040aCompareAdded unit tests for loan expiry! |
Uh oh!
There was an error while loading.Please reload this page.
This PR adds the
ExpiredLoansAnalysisclass to track which loans have expired. The analysis uses a dataflow lattice (ExpiredLattice) to maintain the set of expired loans at each program point.This is a very light weight dataflow analysis and is expected to reach fixed point in ~2 iterations.
In principle, this does not need a dataflow analysis but is used for convenience in favour of lean code.