llvm dev - Jun 2014 - [LLVMdev] PassRegistry thread safety and ManagedStatic interaction

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>

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

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>