PassRegistry thread safety and ManagedStatic interaction

+cc original authors of these changes.

Is PassRegistry intended to be thread-safe? The header file explicitly says that PassRegistry is not thread-safe, but there are mutexes and locking used in the code. This is actually creating a problem, because of a subtle bug involving static initialization order and shutdown.

In particular, the RegisterPass<> static template will get invoked during static initialization and call

PassRegistry::getPassRegistry()->registerPass(*this);

Note that PassRegistry, however, is a ManagedStatic. So the call to getPassRegistry() creates the backing object of the ManagedStatic here. Then registerPass gets called, which attempts to lock the mutex. This will initialize the backing object of the SmartRWMutex.

During shutdown, it happens in reverse order. First the SmartRWMutex is destroyed, then the PassRegistry is destroyed. During the PassRegistry’s destructor, it attempts to lock the mutex again. This works in the current code because ManagedStatic “accidentally” allocates another SmartRWMutex. However, the current implementation of ManagedStatic is already buggy for other reasons, which I’ve tried to fix, and am now running into this as a result of my fix (true once-only initialization of ManagedStatics).

I’m curious about the history here. Can we remove the locking from PassRegistry? And what would it take to get RegisterPass<> to not rely on static initialization. Is there any reason why we can’t just initialize these at runtime early in main?

Hi Zachary,

Yeah, I ran into this a few weeks ago trying to tidy up ownership of
the PassRegistry and it made me sad. Chatting to Chandler he seemed to
be of the opinion that the whole thing was a rats nest of bad & not
worth my time (though perhaps it's worth yours, I'm not sure).

Chandler - was this just going to "go away" in the the glorious new
pass manager future?

I actually had an idea about how to fix this in a relatively painless manner. Although given my experience over the past 4 days, it might not be best to call it painless without first trying :slight_smile:

The idea is to make a StaticPassRegistry. RegisterPass<> only touches the StaticPassRegistry, and nothing else touches the StaticPassRegistry. So once you enter main(), StaticPassRegistry can be considered immutable. In main(), the existing PassRegistry initializes itself from the StaticPassRegistry. This should solve all the problems, the only trick is finding every executable that uses the PassRegistry.

it’s times like this I wish we have an LLVMInitialize() function which every executable using LLVM is required to call early in main().

I actually had an idea about how to fix this in a relatively painless
manner. Although given my experience over the past 4 days, it might not be
best to call it painless without first trying :slight_smile:

The idea is to make a StaticPassRegistry. RegisterPass<> only touches the
StaticPassRegistry, and nothing else touches the StaticPassRegistry. So
once you enter main(), StaticPassRegistry can be considered immutable. In
main(), the existing PassRegistry initializes itself from the
StaticPassRegistry. This *should* solve all the problems, the only trick is
finding every executable that uses the PassRegistry.

How does this address the llvm_shutdown/pass derigstration issue?

(a little more of my tale: I started trying to remove PassRegistry's
pimpl, but noticed that during pass derigstration the PassRegistry had
already been destroyed (since it's created by the first global that
tries to register a pass, so it must be destroyed before that global
in turn tries to deregister the pass) and what would happen is that
the global would be reconstructed, its pimpl would be reinitialized to
null and the deregistration function would stop there... subtle and
annoying)

The mutex could be made an actual global static instead of a ManagedStatic. This guarantees it would be constructed before main, and live until the end of main. So even in PassRegistry’s destructor, which would get call during llvm_shutdown(), it would always have the same mutex. Ideally I’d like to just delete the mutex, as it doesn’t seem like anyone is using it in a multi-threaded fashion, but I could be wrong about that, so I’ll wait for more information.

Eventually the goal would be to actually enforce once-only inside of ManagedStatic (I found this bug actually by changing ManagedStatic to sue std::call_once), so we could prevent problems like this from ever happening in the future.

I accidentally a word. I meant to say the goal would be to enforce once-only initialization inside of ManagedStatic.

I accidentally a word. I meant to say the goal would be to enforce
once-only *initialization* inside of ManagedStatic.

The mutex could be made an actual global static instead of a
ManagedStatic. This guarantees it would be constructed before main, and
live until the end of main.

Except being live before main is insufficient. Passes are registered
from other global initializers. There's no ordering guarantee between
global initializers, so it's possible that pass registrations may
occur before the global mutex is initialized... (at least I think
that's an issue?)

So even in PassRegistry's destructor, which
would get call during llvm_shutdown(), it would always have the same mutex.
Ideally I'd like to just delete the mutex, as it doesn't seem like anyone is
using it in a multi-threaded fashion, but I could be wrong about that, so
I'll wait for more information.

Eventually the goal would be to actually enforce once-only inside of
ManagedStatic (I found this bug actually by changing ManagedStatic to sue
std::call_once), so we could prevent problems like this from ever happening
in the future.

Agreed. I've seen other nasty bugs with insufficiently constrained
singletons that allow reconstruction in some cases... I'd love to have
ManagedStatic be more restrictive about how it can be used.

One other wrinkle is that, at least in theory, LLVM can be used after
llvm_shutdown (llvm_shutdown just brings everything back to the
startup state... so then it can all run again)... so that constraint
can't be added to ManagedStatic. They're meant to reinitialize on
first use after llvm_shutdown :confused:

Shouldn't be an issue. Static initialization is single threaded, and
StaticPassRegistry is immutable once you enter main, so StaticPassRegistry
won't even need the mutex. That's the thing that this solves. It
separates out the code that needs the mutex from the code that doesn't.

Pass registrations occur before the mutex is initialized, but they go into
StaticPassRegistry. As soon as you enter main, you go through the
StaticPassRegistry and add each item to the PassRegistry. The mutex is
guaranteed to be initialized at this time.