[LLVMdev] PassRegistry thread safety and ManagedStatic interaction

[Zachary Turner]

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.




On Mon, Jun 2, 2014 at 9:53 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Jun 2, 2014 at 9:39 AM, Zachary Turner <zturner at google.com> wrote:
> > 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.
>
> 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)
>
> >
> > 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/6a2b325d/attachment.html>