[libcxx-commits] [PATCH] D155278: [libc++][mingw] TLS cleanup on windows

Thu Aug 10 12:35:52 PDT 2023

mstorsjo added a comment.

In D155278#4576476 <https://reviews.llvm.org/D155278#4576476>, @kalle-llvm wrote:

> However, in cxa_exception_storage.cpp there is another implementation of __cxa_get_globals() that is ifdef-ed by `HAS_THREAD_LOCAL`.  This doesn't seem to be documented anywhere, but searching for it I found this <https://github.com/android/ndk/issues/1200> which is the exact same bug.  And turning on `HAS_THREAD_LOCAL` fixes it for me too.

Weird. It seems like this codepath has existed in tree since it was added in 1df50ca6a2f60b3b60ca6adc1ab98f33184c3597 in 2011, without any way of it becoming enabled other than manually defining this macro. There seems to have been a recent attempt to enable it in November 2022, in 8271aa5335668a1dc62168a4e90b4554bd3a0ca7 <https://reviews.llvm.org/rG8271aa5335668a1dc62168a4e90b4554bd3a0ca7>, which struck issues and was reverted a couple days later in bc91104e85b7a233a8966998f387b2222f350f6e <https://reviews.llvm.org/rGbc91104e85b7a233a8966998f387b2222f350f6e>. See https://reviews.llvm.org/D138461 for the full story.

> @mstorsjo So `thread_local` seems to work on mingw, are there any gotchas?  If not, perhaps `HAS_THREAD_LOCAL` should be turned on (at least on mingw, but it seems to me this bug should be possible to trigger on other platforms).

Hmm, I've paged out this matter from my mind again, so I'm not sure I have all the concerns in mind right now...

The thread local destructors do run at roughly the right time when they're supposed to, both when threads exit and when a DLL is unloaded. I guess there's a potential concern that there's no ordering between this thread local destructor and other thread local destructors. So I guess that'd mean that other thread local destructors can't use exception handling within their call stack, if they run after this particular dtor, as there's nothing to force their internal ordering - or is there?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155278/new/

https://reviews.llvm.org/D155278