This is an archive of the discontinued LLVM Phabricator instance.

Use a BumpPtrAllocator for Loop objects
ClosedPublic

Authored by sanjoy on Sep 22 2017, 3:53 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy created this revision.Sep 22 2017, 3:53 PM
sanjoy planned changes to this revision.Sep 22 2017, 4:03 PM
sanjoy added inline comments.
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).

sanjoy updated this revision to Diff 116445.Sep 22 2017, 5:18 PM
  • Update: Use Deallocate
Harbormaster completed remote builds in B10559: Diff 116445.

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.

chandlerc edited edge metadata.Sep 25 2017, 9:00 PM

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).

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.

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).

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.

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.

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).

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.

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.

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.

sanjoy updated this revision to Diff 116891.Sep 27 2017, 3:08 PM

With this update, we no longer have the UB in the legacy PM

sanjoy updated this revision to Diff 116907.Sep 27 2017, 4:54 PM

Remove the last remaining use of isInvalid() (I had missed this one but it
showed up when building in release mode).

chandlerc accepted this revision.Sep 27 2017, 5:58 PM

LGTM!

include/llvm/Analysis/LoopInfo.h
170 ↗(On Diff #116907)

The comment says IsValid, but the code uses IsInvalid...

This revision is now accepted and ready to land.Sep 27 2017, 5:58 PM
This revision was automatically updated to reflect the committed changes.