[PATCH] D28220: provide Win32 native threading

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: include/__threading_support:44
+#define WIN32_LEAN_AND_MEAN
+#define VC_EXTRA_LEAN
+#include <Windows.h>
----------------
kimgr wrote:
> EricWF wrote:
> > compnerd wrote:
> > > EricWF wrote:
> > > > Do these definitions have any affect when `<Windows.h>` has already been included?
> > > > Also are these definitions required before including the header, or merely beneficial? If they are required this will make the `<Windows.h>` header a pain to use with modules.
> > > > 
> > > > 
> > > No, they dont effect it once it has been included.  They are beneficial since they reduce the amount of stuff that gets included (including things which, at least when I last checked, can cause clang to choke).
> > And can users re-include `<Windows.h>` afterwards in the same TU and get all of the symbols?
> I don't think so.
> 
> We've recently switched to defining these two symbols in our build system, and I think that's basically the only way to make this work in a project composed of headers from various authors. I think you're right that libc++ should not define them.
I don't think libc++ should not include windows.h in a public header. I'd rather write our own __threading_support_win.h that re-prototypes everything we need from windows.h, and then we can have some test in libc++ that validates that there are no mismatches when including windows.h before libc++ <mutex>. If we do this, we should sink as much win32 API usage as possible out of headers to reduce our duplication.

It's worth pointing out VS 2015's thread implementation also hides its win32 API usage.


================
Comment at: include/__threading_support:527
+static inline _LIBCPP_ALWAYS_INLINE unsigned int WINAPI
+__libcpp_thread_trampoline(void *__data)
+{
----------------
halyavin wrote:
> Trampolines will never be inlined. Should we put them in support *.cpp instead? The downside is new public symbols which can't be changed without breaking backward compatibility. The upside is that we will have only one copy of each trampoline. What do you think?
Considering that libc++ already has a __thread_proxy trampoline, let's just give it the right CC and get rid of this trampoline.


================
Comment at: include/__threading_support:593
+{
+  // TODO(compnerd) provide a wrapper for CC adjustment
+  *__key = FlsAlloc(reinterpret_cast<void(WINAPI*)(void*)>(__at_exit));
----------------
Yeah, we need to fix that. We should find a way to make `__thread_specific_ptr::__at_thread_exit(void*)` have the right convention out of the box, rather than thunking. Something like:
  #define __LIBCPP_TLS_CALLBACK_CC WINAPI
  .. // else
  #define __LIBCPP_TLS_CALLBACK_CC
  ... // <thread>
      static void __LIBCPP_TLS_CALLBACK_CC __at_thread_exit(void*);



Repository:
  rL LLVM

https://reviews.llvm.org/D28220