And now that we no longer have to explicitly free() the Loop instances, we can
(with more ease) use the destructor of LoopBase to do what LoopBase::clear() was
doing.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Analysis/LoopInfo.cpp | ||
---|---|---|
625 ↗ | (On Diff #116422) | I should also be "freeing" the loop objects here (which for the bump ptr allocator would mean poisoning them). |
There is a problem with this patch -- the legacy pass manager checks if a loop is valid or not by querying isInvalid, which is incorrect; since we call the destructor for loop objects to mark them as "invalid" calling isInvalid for possibly invalid loop objects has UB. We already have this UB in User::operator delete though, and so given that and that the legacy pass manager is going away at some point, perhaps this is okay? Alternatively, I could add a mechanism on the legacy loop pass manager that lets passes denote loop objects to have been deleted (by remembering the pointers in a set) and use that to decide if a Loop object is valid or not (and with that, I can guard isInvalid with #ifndef NDEBUG).
The asserts are using isInvalid correctly, as far as I can tell; they should never have UB in an execution that uses the loop objects correctly.
Sadly, I think this would be problematic. I think it'll trip ASan's use-after-scope or use-after-lifetime checks.... =/ But maybe there is a way around that. Or maybe just implement the solution you suggest. Dunno, but definitely worth checking.
The asserts are using isInvalid correctly, as far as I can tell; they should never have UB in an execution that uses the loop objects correctly.
Cool.
We already do this for User instances -- we read from fields in the User object to be destroyed in its operator delete, after the destructors have run. However, two wrongs don't make a right so I'll first try to see if I can fix this in the old pass manager.
The asserts are using isInvalid correctly, as far as I can tell; they should never have UB in an execution that uses the loop objects correctly.
Cool.
Cool...
However, to be clear, I'm not trying to argue any theorectical purity here. Mostly, I'm worried that the operator delete may be somewhat less problematic *in practice*. If you have carefully verified that this causes no problems in practice with lifetime-markers and sanitizers as they are currently implemented.
Remove the last remaining use of isInvalid() (I had missed this one but it
showed up when building in release mode).
LGTM!
include/llvm/Analysis/LoopInfo.h | ||
---|---|---|
170 ↗ | (On Diff #116907) | The comment says IsValid, but the code uses IsInvalid... |