[LLVMdev] PassRegistry locking scheme

[David Blaikie]

So, long story short I came across some problematic locking:

PassRegistry uses a ManagedStatic<sys::SmartRWMutex<true>> to handle
safe registration, and, importantly, deregistration.

The problem is that ManagedStatic's dtor isn't exactly safe: if the
object is destroyed, then you try to access it again, ManagedStatic
simply constructs a new object and returns it.

This is relied upon to ensure safe/non-racy deregistration of
passes... but what happens during destruction? The mutex is destroyed
and replaced with another, completely different mutex... that doesn't
seem like a safe locking scheme.

Does any of this make sense? Am I missing something about how this is safe?


Long story:

This is how I came across this bug:

* While removing manual memory management in PassRegistry I noticed a
rather arcane piece of Pimpl usage
* The pimpl used a void*, so I set about moving the declaration of the
pimpl type into the outer type as is the usual case
* Then I wondered why it was a pimpl at all - so I tried to remove it
* (aside: to handle safe locking around destruction I added an
Optional<SmartReadLock> as the first member, initialized that in the
dtor, and let it hold the lock through the destruction of the other
members... )
* But then I realized that PassRegistry::removeRegistrationListener
relied on testing that the pimpl was null to safely no-op after the
PassRegistry had been destroyed (undefined order of destruction & all
that jazz)
* so I added a boolean to track destruction state - but this did not
fix the issue
* then, while talking it out, I realized what was really happening:
the PassRegistry had been destroyed and the ManagedStatic around it,
instead of returning the destroyed object, had simply built a new one
in its place with a still null pimpl (the pimpl was lazy, clearly part
of this cunning scheme) - and thus the removeRegistrationListener call
was a no-op, except to construct and leave around an extra, empty,
PassRegistry in the post-destroyed ManagedStatic...
* which got me thinking: what about the lock, which was also a
ManagedStatic... which is how I arrived at the above observation