[PATCH] PrettyStackTrace should use ManagedStatic for its ThreadLocal

[Reid Kleckner]

+
+  bool isInitialized() const {
+    return Ptr != 0;
+  }
 };

ManagedStaticBase already has isConstructed() which implements this check,
so I'd use that.  Also, it's better for code size to put non-dependent code
in the template base class.

Otherwise LGTM.  :)


On Thu, Sep 12, 2013 at 2:13 PM, Filip Pizlo <fpizlo at apple.com> wrote:

>
> 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/20130913/e96c487e/attachment.html>