-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,15 +23,10 @@ | |
#include "llvm/Support/Debug.h" | ||
#include "llvm/Support/TimeProfiler.h" | ||
#include <cstdint> | ||
#include <memory> | ||
|
||
namespace clang::lifetimes { | ||
namespace internal { | ||
namespace { | ||
template <typename Tag> | ||
inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ID<Tag> ID) { | ||
return OS << ID.Value; | ||
} | ||
} // namespace | ||
|
||
/// Represents the storage location being borrowed, e.g., a specific stack | ||
/// variable. | ||
|
@@ -832,6 +827,91 @@ class LoanPropagationAnalysis | |
} | ||
}; | ||
|
||
// ========================================================================= // | ||
// Expired Loans Analysis | ||
// ========================================================================= // | ||
|
||
/// The dataflow lattice for tracking the set of expired loans. | ||
struct ExpiredLattice { | ||
LoanSet Expired; | ||
|
||
ExpiredLattice() : Expired(nullptr) {}; | ||
explicit ExpiredLattice(LoanSet S) : Expired(S) {} | ||
|
||
bool operator==(const ExpiredLattice &Other) const { | ||
return Expired == Other.Expired; | ||
} | ||
bool operator!=(const ExpiredLattice &Other) const { | ||
return !(*this == Other); | ||
} | ||
|
||
void dump(llvm::raw_ostream &OS) const { | ||
OS << "ExpiredLattice State:\n"; | ||
if (Expired.isEmpty()) | ||
OS << " <empty>\n"; | ||
for (const LoanID &LID : Expired) | ||
OS << " Loan " << LID << " is expired\n"; | ||
} | ||
}; | ||
|
||
/// The analysis that tracks which loans have expired. | ||
class ExpiredLoansAnalysis | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. We do expect this to be significantly cheaper than the other analyses and have a tight bound (proportional to both the #loans and the max nesting). Deferring to a later PR to add some asserts along with strict bounds makes sense to me. |
||
: public DataflowAnalysis<ExpiredLoansAnalysis, ExpiredLattice, | ||
Direction::Forward> { | ||
|
||
LoanSet::Factory &Factory; | ||
|
||
public: | ||
ExpiredLoansAnalysis(const CFG &C, AnalysisDeclContext &AC, FactManager &F, | ||
LifetimeFactory &Factory) | ||
: DataflowAnalysis(C, AC, F), Factory(Factory.LoanSetFactory) {} | ||
|
||
using Base::transfer; | ||
|
||
StringRef getAnalysisName() const { return "ExpiredLoans"; } | ||
|
||
Lattice getInitialState() { return Lattice(Factory.getEmptySet()); } | ||
|
||
/// Merges two lattices by taking the union of the expired loan sets. | ||
Lattice join(Lattice L1, Lattice L2) const { | ||
return Lattice(utils::join(L1.Expired, L2.Expired, Factory)); | ||
} | ||
|
||
Lattice transfer(Lattice In, const ExpireFact &F) { | ||
return Lattice(Factory.add(In.Expired, F.getLoanID())); | ||
} | ||
|
||
// Removes the loan from the set of expired loans. | ||
// | ||
// When a loan is re-issued (e.g., in a loop), it is no longer considered | ||
// expired. A loan can be in the expired set at the point of issue due to | ||
// the dataflow state from a previous loop iteration being propagated along | ||
// a backedge in the CFG. | ||
// | ||
// Note: This has a subtle false-negative though where a loan from previous | ||
// iteration is not overwritten by a reissue. This needs careful tracking | ||
// of loans "across iterations" which can be considered for future | ||
// enhancements. | ||
// | ||
// void foo(int safe) { | ||
// int* p = &safe; | ||
// int* q = &safe; | ||
// while (condition()) { | ||
// int x = 1; | ||
// p = &x; // A loan to 'x' is issued to 'p' in every iteration. | ||
// if (condition()) { | ||
// q = p; | ||
// } | ||
// (void)*p; // OK — 'p' points to 'x' from new iteration. | ||
// (void)*q; // UaF - 'q' still points to 'x' from previous iteration | ||
// // which is now destroyed. | ||
// } | ||
// } | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
}; | ||
|
||
// ========================================================================= // | ||
// TODO: | ||
// - Modify loan expiry analysis to answer `bool isExpired(Loan L, Point P)` | ||
|
@@ -873,6 +953,10 @@ void LifetimeSafetyAnalysis::run() { | |
LoanPropagation = | ||
std::make_unique<LoanPropagationAnalysis>(Cfg, AC, *FactMgr, *Factory); | ||
LoanPropagation->run(); | ||
|
||
ExpiredLoans = | ||
std::make_unique<ExpiredLoansAnalysis>(Cfg, AC, *FactMgr, *Factory); | ||
ExpiredLoans->run(); | ||
} | ||
|
||
LoanSet LifetimeSafetyAnalysis::getLoansAtPoint(OriginID OID, | ||
|
@@ -881,6 +965,11 @@ LoanSet LifetimeSafetyAnalysis::getLoansAtPoint(OriginID OID, | |
return LoanPropagation->getLoans(OID, PP); | ||
} | ||
|
||
LoanSet LifetimeSafetyAnalysis::getExpiredLoansAtPoint(ProgramPoint PP) const { | ||
assert(ExpiredLoans && "ExpiredLoansAnalysis has not been run."); | ||
return ExpiredLoans->getState(PP).Expired; | ||
} | ||
|
||
std::optional<OriginID> | ||
LifetimeSafetyAnalysis::getOriginIDForDecl(const ValueDecl *D) const { | ||
assert(FactMgr && "FactManager not initialized"); | ||
|
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.