<div dir="ltr">I accidentally a word. I meant to say the goal would be to enforce once-only *initialization* inside of ManagedStatic.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 2, 2014 at 9:59 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div>
<br></div><div>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.<br>
<div><br></div><div><br></div></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 2, 2014 at 9:53 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Mon, Jun 2, 2014 at 9:39 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> I actually had an idea about how to fix this in a relatively painless<br>
> manner. Although given my experience over the past 4 days, it might not be<br>
> best to call it painless without first trying :)<br>
><br>
> The idea is to make a StaticPassRegistry. RegisterPass<> only touches the<br>
> StaticPassRegistry, and nothing else touches the StaticPassRegistry. So<br>
> once you enter main(), StaticPassRegistry can be considered immutable. In<br>
> main(), the existing PassRegistry initializes itself from the<br>
> StaticPassRegistry. This *should* solve all the problems, the only trick is<br>
> finding every executable that uses the PassRegistry.<br>
<br>
</div>How does this address the llvm_shutdown/pass derigstration issue?<br>
<br>
(a little more of my tale: I started trying to remove PassRegistry's<br>
pimpl, but noticed that during pass derigstration the PassRegistry had<br>
already been destroyed (since it's created by the first global that<br>
tries to register a pass, so it must be destroyed before that global<br>
in turn tries to deregister the pass) and what would happen is that<br>
the global would be reconstructed, its pimpl would be reinitialized to<br>
null and the deregistration function would stop there... subtle and<br>
annoying)<br>
<div><div><br>
><br>
> it's times like this I wish we have an LLVMInitialize() function which every<br>
> executable using LLVM is required to call early in main().<br>
><br>
><br>
> On Mon, Jun 2, 2014 at 9:14 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> Yeah, I ran into this a few weeks ago trying to tidy up ownership of<br>
>> the PassRegistry and it made me sad. Chatting to Chandler he seemed to<br>
>> be of the opinion that the whole thing was a rats nest of bad & not<br>
>> worth my time (though perhaps it's worth yours, I'm not sure).<br>
>><br>
>> Chandler - was this just going to "go away" in the the glorious new<br>
>> pass manager future?<br>
>><br>
>> On Sun, Jun 1, 2014 at 10:36 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>><br>
>> wrote:<br>
>> > +cc original authors of these changes.<br>
>> ><br>
>> > Is PassRegistry intended to be thread-safe? The header file explicitly<br>
>> > says<br>
>> > that PassRegistry is not thread-safe, but there are mutexes and locking<br>
>> > used<br>
>> > in the code. This is actually creating a problem, because of a subtle<br>
>> > bug<br>
>> > involving static initialization order and shutdown.<br>
>> ><br>
>> > In particular, the RegisterPass<> static template will get invoked<br>
>> > during<br>
>> > static initialization and call<br>
>> ><br>
>> > PassRegistry::getPassRegistry()->registerPass(*this);<br>
>> ><br>
>> > Note that PassRegistry, however, is a ManagedStatic. So the call to<br>
>> > getPassRegistry() creates the backing object of the ManagedStatic here.<br>
>> > Then registerPass gets called, which attempts to lock the mutex. This<br>
>> > will<br>
>> > initialize the backing object of the SmartRWMutex.<br>
>> ><br>
>> > During shutdown, it happens in reverse order. First the SmartRWMutex is<br>
>> > destroyed, then the PassRegistry is destroyed. During the<br>
>> > PassRegistry's<br>
>> > destructor, it attempts to lock the mutex again. This works in the<br>
>> > current<br>
>> > code because ManagedStatic "accidentally" allocates another<br>
>> > SmartRWMutex.<br>
>> > However, the current implementation of ManagedStatic is already buggy<br>
>> > for<br>
>> > other reasons, which I've tried to fix, and am now running into this as<br>
>> > a<br>
>> > result of my fix (true once-only initialization of ManagedStatics).<br>
>> ><br>
>> > I'm curious about the history here. Can we remove the locking from<br>
>> > PassRegistry? And what would it take to get RegisterPass<> to not rely<br>
>> > on<br>
>> > static initialization. Is there any reason why we can't just initialize<br>
>> > these at runtime early in main?<br>
>> ><br>
>> > _______________________________________________<br>
>> > LLVM Developers mailing list<br>
>> > <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>