[PATCH] PrettyStackTrace should use ManagedStatic for its ThreadLocal

[Filip Pizlo]

On Sep 12, 2013, at 1:50 PM, Reid Kleckner <rnk at google.com> wrote:

> I don't think this is a good way to go.  Does LLVM support reinitializing and calling llvm_shutdown() multiple times?  If so, this won't work.

Interesting.  It appears that this could almost work, since llvm_shutdown() clears all ManagedStatics, making them return to their pristine state.

I don't think your proposed solution is a good way to go, either (see below).  What do you think of the following: ~PrettyStackTraceEntry does nothing if its thread-local's wrapping ManagedStatic is uninitialized.  This can only happen if ~PrettyStackTraceEntry is called after a shutdown.  This rescues you if you use this idiom of destroying ManagedStatic's before destroying PrettyStackTraceEntry.  Of course, it will mean that you will see things missing from your pretty stack trace if you do something like:

PrettyStackTraceEntry X(...)
{
    llvm_shutdown_obj Y;
    // (1)
}
{
    llvm_shutdown_obj Y;
    // (2)
}

In (1) you'll see X in the stack and in (2) you won't.  I'm not aware of anyone using this more complex idiom so I suspect that this fix should be fine.

See attached patch.

> 
> How about adding a variant of ManagedStatic that registers the destructor with atexit(), just like C++ dynamic initializers do, instead of the LLVM shutdown machinery?

That wouldn't fix the problem that I'm trying to fix.  A library that wishes to be used in a larger system shouldn't register atexit() or anything like it.  That's why I like that ManagedStatic's are only destroyed via an explicit call to llvm_shutdown().

Registering atexit() means that you end up with the following situation:

Program A links against library B, which then links against library C.  Lets say C = llvm.

Library B provides a create/destroy API for its own objects.  It does not require programs that use it to call destroy, if the object is used as a global.  This is common.  The code base I work with (JavaScriptCore, part of WebKit, see webkit.org) has provided just such an API for a long time and we intend to continue supporting it.

Library B decides to spawn LLVM compilations concurrently.

Now you get the following exit-time race:

1) B tells LLVM to compile something on a thread.
2) LLVM starts working.
3) A calls exit(0).

exit() then calls atexit() handlers and you start destroying ManagedStatic's while LLVM is working in a thread.

One of the things that my patches are trying to ensure is that if the client of LLVM (in this case library B) isn't itself shut down explicitly (something that is common and that we allow with our APIs) then you don't get exit-time races, because then the client won't call llvm_shutdown().  In fact, we're going further in JavaScriptCore: we never call llvm_shutdown(), though we do destroy the Context/ExecutionEngine/etc if our notion of context (we call it "VM") is destroyed or if we haven't used LLVM for a while.  Of course, we won't destroy the Context/ExecutionEngine/etc if our client calls exit() while we're using them on a thread - and we'd like this to Just Work.

I know that you could argue that clients of LLVM should always call llvm_shutdown().  This will probably work for a lot of clients where the relationship between the outer application and LLVM isn't transitive through multiple intervening libraries.  We certainly can't change our API to require our client to destroy the global VM, and it would be silly if that blocked us from ever using LLVM to compile any code due to exit-time crashes.

> 
> Then we could convert llvm::outs() to use it, thereby potential initialization races on Windows.

Interesting.  Can you elaborate what the problem is there?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130912/73a1a4d8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pretty-stack-trace-managed-static.diff
Type: application/octet-stream
Size: 3136 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130912/73a1a4d8/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130912/73a1a4d8/attachment-0001.html>