[PATCH] Change llvm::sys::Mutex implementation to use STL-provided mutexes.

[Zachary Turner]

So unfortunately this looks tricker than I had hoped to change the shutdown
order.  My plan was to add a function called
llvm_register_shutdown_handler() that was basically a replacement for
atexit(), which would allow us to run the shutdown handlers in
llvm_shutdown().  Unfortunately there are numerous calls scattered
throughout the codebase which call exit(), which kills this approach unless
we create a new function called llvm_exit() which first calls
llvm_shutdown() and then calls exit, and then replace all calls to exit()
with llvm_exit().  But then that leaves open the possibility that someone
else doesn't know about llvm_exit(), and adds more calls to exit() in the
future.

I'm not really sure what a good solution is.  Assuming the patch that's
currently uploaded passes the tests on linux and Mac, I'm inclined to
special-case this and just use a critical section for this one specific
mutex on Windows-only, and leave everything else using the STL
implementation, adding a comment to the class to indicate that it cannot be
used on Windows during an atexit handler.

At a higher level, I think ti would be great if the system were architected
such that atexit() handlers were never used.  They're error-prone (as we
see here), and seem to me like a bandaid for poorly designed shutdown /
cleanup strategy.  Having only submitted a handful of trivial patches
before and only having looked at llvm code for the first time a few days
ago though, I'm probably not the right person to do that.  Maybe in the
future :P


One more question: When I run the tests from inside Visual Studio, I get a
bunch of errors about not being able to find a bunch of linux utilities
like grep / bash / etc.  Is this normal?  Is it possible to run the test
suite without cygwin?



On Fri, May 30, 2014 at 8:31 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> Thank you for digging so deeply into this!
>
> On Thu, May 29, 2014 at 8:23 PM, Zachary Turner <zturner at google.com>
> wrote:
> > So, the issue is that std::mutex and std::recursive_mutex simply cannot
> be
> > used during an atexit() handler.  It still seems like a good drop-in
> > replacement for the hand-rolled mutex in all other cases.
> >
> > This is not documented anywhere, but if you dig into the code you'll find
> > that std::mutex and std::recursive_mutex are implemented using ConcRT
> > (microsoft concurrency runtime), and it relies on a call to
> > RegisterWaitForSingleObject().  That function in turn uses QueueUserAPC,
> and
> > if you check the documentation you'll see that it says "When the thread
> is
> > in the process of being terminated, calling QueueUserAPC to the thread's
> APC
> > queue will fail with ERROR_GEN_FAILURE (31)".
> >
> > So, TL;DR - can't use mutexes from atexit handler.  This seems like a
> > reasonable restriction, as atexit handlers are probably pretty rare
> anyway.
>
> I agree that this is a reasonable restriction.
>
> > I will try to find a way to change the shutdown order to not rely on the
> > atexit handler.
>
> Sounds like a good approach.
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140530/00d905b5/attachment.html>