Don't register and de-register all PassRegistrationListeners automatically

[zturner]

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.

[rnk]

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

[dblaikie]

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

include/llvm/IR/LegacyPassNameParser.h

47

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

You can omit this ctor entirely.

345

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]

include/llvm/IR/LegacyPassNameParser.h

47

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 by commit rL210724 (authored by @zturner).

[dblaikie]

include/llvm/IR/LegacyPassNameParser.h

47

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

include/llvm/IR/LegacyPassNameParser.h

 //===----------------------------------------------------------------------===//
 // PassNameParser class - Make use of the pass registration mechanism to
 // automatically add a command line argument to opt for each pass.
 //
 class PassNameParser : public PassRegistrationListener,
                        public cl::parser<const PassInfo*> {
   cl::Option *Opt;
 public:
-  PassNameParser() : Opt(nullptr) {}
+  PassNameParser();
   virtual ~PassNameParser();
 
   void initialize(cl::Option &O) {
     Opt = &O;
     cl::parser<const PassInfo*>::initialize(O);
 
     // Add all of the passes to the map that got initialized before 'this' did.
     enumeratePasses();
   }

include/llvm/PassSupport.h

void llvm::initialize##passName##Pass(PassRegistry &Registry) { \
     CALL_ONCE_INITIALIZATION(initialize##passName##PassOnce) \
   }
 
 //===---------------------------------------------------------------------------
 /// PassRegistrationListener class - This class is meant to be derived from by
 /// clients that are interested in which passes get registered and unregistered
 /// at runtime (which can be because of the RegisterPass constructors being run
 /// as the program starts up, or may be because a shared object just got
-/// loaded).  Deriving from the PassRegistrationListener class automatically
-/// registers your object to receive callbacks indicating when passes are loaded
-/// and removed.
+/// loaded).
 ///
 struct PassRegistrationListener {
 
-  /// PassRegistrationListener ctor - Add the current object to the list of
-  /// PassRegistrationListeners...
-  PassRegistrationListener();
-
-  /// dtor - Remove object from list of listeners...
-  ///
-  virtual ~PassRegistrationListener();
+  PassRegistrationListener() {}
+  virtual ~PassRegistrationListener() {}
 
   /// Callback functions - These functions are invoked whenever a pass is loaded
   /// or removed from the current executable.
   ///
   virtual void passRegistered(const PassInfo *) {}
 
   /// enumeratePasses - Iterate over the registered passes, calling the
   /// passEnumerate callback on each PassInfo object.

lib/IR/Pass.cpp

RegisterAGBase::RegisterAGBase(const char *Name, const void *InterfaceID,
   PassRegistry::getPassRegistry()->registerAnalysisGroup(InterfaceID, PassID,
                                                          *this, isDefault);
 }
 
 //===----------------------------------------------------------------------===//
 // PassRegistrationListener implementation
 //
 
-// PassRegistrationListener ctor - Add the current object to the list of
-// PassRegistrationListeners...
-PassRegistrationListener::PassRegistrationListener() {
-  PassRegistry::getPassRegistry()->addRegistrationListener(this);
-}
-
-// dtor - Remove object from list of listeners...
-PassRegistrationListener::~PassRegistrationListener() {
-  PassRegistry::getPassRegistry()->removeRegistrationListener(this);
-}
-
 // enumeratePasses - Iterate over the registered passes, calling the
 // passEnumerate callback on each PassInfo object.
 //
 void PassRegistrationListener::enumeratePasses() {
   PassRegistry::getPassRegistry()->enumerateWith(this);
 }
 
-PassNameParser::~PassNameParser() {}
+PassNameParser::PassNameParser()
+    : Opt(nullptr) {
+  PassRegistry::getPassRegistry()->addRegistrationListener(this);
+}
+
+PassNameParser::~PassNameParser() {
+  // This only gets called during static destruction, in which case the
+  // PassRegistry will have already been destroyed by llvm_shutdown().  So
+  // attempting to remove the registration listener is an error.
+}
 
 //===----------------------------------------------------------------------===//
 //   AnalysisUsage Class Implementation
 //
 
 namespace {
   struct GetCFGOnlyPasses : public PassRegistrationListener {
     typedef AnalysisUsage::VectorType VectorType;