Removing the static destructor from ManagedStatic.cpp by controlling the allocation and de-allocaation of the mutex.

[beanz]

Summary

This is part of the ongoing work to remove static constructors and destructors.

[chandlerc]

lib/Support/ManagedStatic.cpp

30 ↗

(On Diff #14021)

Uh... How is this safe? How does this bootstrap when it is called while acquiring a lock (and thus concurrently without any other synchronization)?

[beanz]

lib/Support/ManagedStatic.cpp

30 ↗

(On Diff #14021)

It isn't, but since MSVC doesn't implement thread-safe static local initialization I figured this wasn't any worse off.

Do you have an alternate suggestion? I know in other places in LLVM we avoid the static destructor by just leaking the pointer (which I don't like). I could also static init the pointer, and still control deleting it, but I'm not sure that gets us much.

Thoughts?

[chandlerc]

lib/Support/ManagedStatic.cpp

30 ↗

(On Diff #14021)

I don't know how we've not been bitten by this before. It certainly was always unsafe on Windows, but historically was safe on Linux and Mac. =/

I think the correct fix here is to just platform specific code here. Rewrite this as an initialization function, and then use pthreads if we have it or C++11 if not (so MinGW's defective C++11 <mutex> and <thread> implementation can be coped with) to do a 'run_once' or 'once_init' or however it is spelled in a threadsafe way.

I'm happy to "leak" the single mutex provided we initialize it in a thread safe way.

[rnk]

lib/Support/ManagedStatic.cpp

30 ↗

(On Diff #14021)

The problematic platform is MinGW with native win32 threads. This platform has neither pthreads nor C++11 <mutex>. The only way to implement initialization here is to use the native win32 one-time initialization:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686934(v=vs.85).aspx

I think I have the right bits to test this configuration now. If you send a patch I can fill in the blanks and send it back.

[beanz]

This revision of the patch handles the thread safe initialization concerns that were raised by Chandler and Reid. The approach to handling the thread safe initialization is the implementation of a new LLVM_CALL_ONCE macro which has implementations using C++11 call_once, pthread_once, and sys::CompareAndSwap.

The LLVM_CALL_ONCE macro could alternatively be implemented as a function taking two pointer parameters, one to the function to execute, and the other to the flag.

[rnk]

configure

9628 ↗

(On Diff #14532)

Did you modify autoconf/configure.ac to produce this, or write it out yourself based on the pthread_mutex_init code above? We should keep our changes in the .ac file and regenerate this with the appropriate autoconf. Eric can do that if you don't have the right bits.

include/llvm/Support/Threading.h

18 ↗

(On Diff #14532)

We can only include llvm/Config/config.h from .cpp files, not .h files.

24 ↗

(On Diff #14532)

We take a function pointer as a template parameter instead of using macros. We'll get different static data members / static locals between different instantiations that way, which is what the macros appear to be solving.

47 ↗

(On Diff #14532)

I don't think we want to commit this double checked locking. I think we can simplify this to:

if Windows:
  use InitOnceExecuteOnce in lib/Support/Windows/Threading.inc
else:
  use a C++11 thread-safe static local in lib/Support/Unix/Threading.inc

lib/Support/ManagedStatic.cpp

99–100 ↗

(On Diff #14532)

This is a semantic change, but it can only break in ways that we probably don't support, ie racily shutting down LLVM while someone is trying to access a ManagedStatic. Seems fine. :)

[beanz]

I'm working on patch revisions, but I do have some comments inline below.

configure

9628 ↗

(On Diff #14532)

I'll have Eric review these patches. My autoconf foo is severely lacking.

include/llvm/Support/Threading.h

18 ↗

(On Diff #14532)

Really? We do include it in headers like Unix.h and WindowsSupport.h. I'm not really sure how to implement this without access to the HAVE_PTHREAD_ONCE define.

24 ↗

(On Diff #14532)

This would absolutely be a cleaner way to do this.

47 ↗

(On Diff #14532)

I'm not sure this is the right breakdown. I believe MSVC has std::call_once, so I was hoping to do a breakdown based on the availability of <mutex>, and pthread_once.

[beanz]

This update has the following changes:

• Based on discussions about the avaialability of <mutex> I've removed the pthread implementation and all the autoconf/cmake associated changes.
• Moved away from macros to a template implementation
• All platforms other than MingW32 use std::call_once, MingW32 uses the hand-rolled solution.

[rnk]

Thanks, I think this is almost done.

include/llvm/Support/Threading.h

51–69 ↗

(On Diff #14823)

mingw64 should have thread safe statics, so I think this can just be:

struct InitOnceWrapper {
  InitOnceWrapper() { UserFn(); }
};
static InitOnceWrapper InitOnceVar;

I don't actually understand memory barriers, so I'd rather rely on standard primitives.

18 ↗

(On Diff #14532)

I should've said .h files in llvm/include/, since those are exported to users, while config.h is not installed. The .h files in llvm/lib/ like Unix.h can use config.h.

47 ↗

(On Diff #14532)

That won't cover the mingw64 with no pthread emulation case, though. I think we can use thread-safe statics in that case, though.

[beanz]

include/llvm/Support/Threading.h

51–69 ↗

(On Diff #14823)

Doesn't MingW64 have <mutex>? I think we only need to fall back to the custom code on MingW32.

[beanz]

Updated to have all non-MingW platforms use std::call_once, and MingW platforms use thread safe static initialization.

[rnk]

Thanks! lgtm

[chandlerc]

include/llvm/Support/Threading.h

41 ↗

(On Diff #14831)

You really need some documentation for how to use this correctly...

42 ↗

(On Diff #14831)

I would remove the extraneous blank lines here.

Closed by commit rL219640 (authored by cbieneman).

llvm/trunk/include/llvm/Support/Threading.h

namespace llvm {
   ///
   /// \param UserFn - The callback to execute.
   /// \param UserData - An argument to pass to the callback function.
   /// \param RequestedStackSize - If non-zero, a requested size (in bytes) for
   /// the thread stack.
   void llvm_execute_on_thread(void (*UserFn)(void*), void *UserData,
                               unsigned RequestedStackSize = 0);
 
+/// \brief Execute the function specified as a template parameter once.
+///
+/// Calls \p UserFn once ever. The call uniqueness is based on the address of
+/// the function passed in via the template arguement. This means no matter how
+/// many times you call llvm_call_once<foo>() in the same or different
+/// locations, foo will only be called once.
+///
+/// Typical usage:
+/// \code
+///   void foo() {...};
+///   ...
+///   llvm_call_once<foo>();
+/// \endcode
+///
+/// \param UserFn Function to call once.
 template <void (*UserFn)(void)> void llvm_call_once() {
-
 #if !defined(__MINGW__)
   static std::once_flag flag;
   std::call_once(flag, UserFn);
-
 #else
   struct InitOnceWrapper {
     InitOnceWrapper() { UserFn(); }
   };
   static InitOnceWrapper InitOnceVar;
 #endif
 }
 }
 
 #endif