Page MenuHomePhabricator

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

Authored by beanz on Sep 23 2014, 3:35 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 14021.Sep 23 2014, 3:35 PM
beanz retitled this revision from to Removing the static destructor from ManagedStatic.cpp by controlling the allocation and de-allocaation of the mutex..
beanz updated this object.
beanz edited the test plan for this revision. (Show Details)
beanz added a reviewer: chandlerc.
beanz added a subscriber: Unknown Object (MLST).
chandlerc added inline comments.Sep 23 2014, 3:46 PM
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 added inline comments.Sep 23 2014, 3:55 PM
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 added inline comments.Sep 23 2014, 4:07 PM
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 added a subscriber: rnk.Sep 23 2014, 5:15 PM
rnk added inline comments.
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 updated this revision to Diff 14532.Oct 7 2014, 3:05 PM

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 added inline comments.Oct 7 2014, 4:08 PM
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 added a comment.Oct 8 2014, 4:52 PM

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 updated this revision to Diff 14823.Oct 13 2014, 2:23 PM

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 added a comment.Oct 13 2014, 2:57 PM

Thanks, I think this is almost done.

include/llvm/Support/Threading.h
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.

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.

beanz added inline comments.Oct 13 2014, 3:04 PM
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 this revision to Diff 14831.Oct 13 2014, 3:35 PM

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

rnk accepted this revision.Oct 13 2014, 3:38 PM
rnk added a reviewer: rnk.

Thanks! lgtm

This revision is now accepted and ready to land.Oct 13 2014, 3:38 PM
chandlerc added inline comments.Oct 13 2014, 3:47 PM
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.

Diffusion closed this revision.Oct 13 2014, 4:14 PM
Diffusion updated this revision to Diff 14841.

Closed by commit rL219640 (authored by cbieneman).