llvm dev - Jun 2014 - [LLVMdev] 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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140601/db260431/attachment.html>

Hi Zachary,

On 1 June 2014 18:36, Zachary Turner <zturner at google.com>
wrote:
> 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?
The pass infrastructure is designed so that individual passes can be
built as shared libraries and loaded at runtime (e.g. via opt), so the
passes require the use of the static initializer rather than
initializing in main.

Cheers,
Amara

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?

On Sun, Jun 1, 2014 at 10:36 AM, Zachary Turner <zturner at google.com>
wrote:
> +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?
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

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

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


On Mon, Jun 2, 2014 at 9:14 AM, David Blaikie <dblaikie at gmail.com>
wrote:
> 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?
>
> On Sun, Jun 1, 2014 at 10:36 AM, Zachary Turner <zturner at
google.com>
> wrote:
> > +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?
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140602/7c2fedc5/attachment.html>