-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LifetimeSafety] Add loan expiry analysis #148712
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
880cf3b
to
ed73e3b
Compare
c217d0d
to
702d255
Compare
ed73e3b
to
a50b00e
Compare
7e098b0
to
793d58e
Compare
a50b00e
to
a9f1e8d
Compare
793d58e
to
2bff132
Compare
6ce3173
to
9265536
Compare
2253ff0
to
c073ef3
Compare
9265536
to
9da8f3a
Compare
c073ef3
to
51afc3e
Compare
9da8f3a
to
04d6193
Compare
51afc3e
to
7b26a72
Compare
04d6193
to
2073937
Compare
5f09913
to
aca68c2
Compare
b109b6a
to
7502ee5
Compare
7365fc4
to
a7d03b1
Compare
a7d03b1
to
d5b093e
Compare
d5b093e
to
70b63ed
Compare
16fccbd
to
3e0b53f
Compare
9a3a69a
to
8c5c8f1
Compare
3e0b53f
to
e6fc855
Compare
8c5c8f1
to
c29040a
Compare
Added unit tests for loan expiry! |
@@ -832,6 +833,65 @@ class LoanPropagationAnalysis | |||
} | |||
}; | |||
|
|||
// ========================================================================= // | |||
// Expired Loans Analysis | |||
// ========================================================================= // |
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.
nit: add dash.
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.
Can you please add a suggestion? I don't quite get the location where you are referring.
} | ||
|
||
Lattice transfer(Lattice In, const IssueFact &F) { | ||
return Lattice(Factory.remove(In.Expired, F.getLoanID())); |
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.
Might be worth explaining why this is necessary. Specifically, now its possible that the loan would appear in In.Expired
at all to be worth removing. I assume this arises because of backedges.
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.
Done. There is a subtle false negative due to this as well. Mentioned that.
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 comments are addressed it looks good to me.
c29040a
to
6ad27da
Compare
e6fc855
to
ad8d8a8
Compare
6ad27da
to
7ec322f
Compare
7ec322f
to
65f5402
Compare
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.
LG too!
65f5402
to
7a78e40
Compare
This PR adds the `ExpiredLoansAnalysis` class 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.
This PR adds the
ExpiredLoansAnalysis
class 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.