Page MenuHomePhabricator

Don't register and de-register all PassRegistrationListeners automatically
ClosedPublic

Authored by zturner on Jun 11 2014, 4:01 PM.

Details

Summary

PassRegistrationListener is intended for use as a generic listener.
In some cases, PassRegistrationListener-derived classes were being
created, and automatically registered and de-registered in static
constructors and destructors. Since ManagedStatics are destroyed
prior to program shutdown, this leads to errors where an attempt is
made to access a ManagedStatic that has already been destroyed.
This patch addresses this by removing automatic de-registration
on destruction entirely.

In other cases, registering / de-registering a PassRegistrationListener
is not even necessarily desired, as they can be used for enumeration
without being registered. So this change additionally moves the
automatic registration out of the base class and into those concrete
implementations which desire this behavior.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 10337.Jun 11 2014, 4:01 PM
zturner retitled this revision from to Don't register and de-register all PassRegistrationListeners automatically.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: dblaikie, rnk.
zturner set the repository for this revision to rL LLVM.
zturner added a subscriber: Unknown Object (MLST).
rnk accepted this revision.Jun 11 2014, 4:06 PM
rnk edited edge metadata.

lgtm, ship it :) This is way simpler than the alternatives we discussed.

This revision is now accepted and ready to land.Jun 11 2014, 4:06 PM
dblaikie accepted this revision.Jun 11 2014, 4:59 PM
dblaikie edited edge metadata.

I'm with Reid - ship it. (comments describing some optional simplifications, if you like)

include/llvm/IR/LegacyPassNameParser.h
47 ↗(On Diff #10337)

should we just remove this dtor entirely? (I don't see any reason it needs to be virtual, and it doesn't do anything)

include/llvm/PassSupport.h
344 ↗(On Diff #10337)

You can omit this ctor entirely.

345 ↗(On Diff #10337)

You could make this dtor non-virtual and protected, if you like (not necessary by any means) since these things aren't polymorphically owned/destroyed, by the looks of it.

zturner added inline comments.Jun 11 2014, 5:22 PM
include/llvm/IR/LegacyPassNameParser.h
47 ↗(On Diff #10337)

I kind of would rather leave it, if only because it's a nice place to drop a comment that explicitly explains the reasoning behind the asymmetry of not calling removeRegistrationListener, just to make sure we don't get back into this bind later in the future.

zturner closed this revision.Jun 11 2014, 5:24 PM
zturner updated this revision to Diff 10340.

Closed by commit rL210724 (authored by @zturner).

dblaikie added inline comments.Jun 11 2014, 5:29 PM
include/llvm/IR/LegacyPassNameParser.h
47 ↗(On Diff #10337)

Will we be able to secure that bug from recurring silently in the future?

I'm not sure what assert/check we can add to make sure that ManagedStatics aren't (re)created during global dtors...

If they didn't allow re-initialization, then we could add assertions that check that after destruction they are never accessed again (I've done this for other lazy singletons I've dealt with, but they didn't have resurrection). Without that, and no (that I know of) to detect that code is currently running in a global dtor, I can't think of a way, unfortunately...