[LLVMdev] PassRegistry thread safety and ManagedStatic interaction

[Zachary Turner]

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


On Mon, Jun 2, 2014 at 9:59 AM, Zachary Turner <zturner at google.com> wrote:

> 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/85922b10/attachment.html>