Skip to content

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jul 3, 2024

Previously, we'd use HAS_THREAD_LOCAL which was never defined. Hence, we'd basically never use the code path where we use thread_local.

Fixes #78207

…or exception storage

Previously, we'd use HAS_THREAD_LOCAL which was never defined. Hence,
we'd basically never use the code path where we use thread_local.

Fixes llvm#78207
@ldionne ldionne requested a review from a team as a code owner July 3, 2024 15:33
@llvmbot llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Jul 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 3, 2024

@llvm/pr-subscribers-libcxxabi

Author: Louis Dionne (ldionne)

Changes

Previously, we'd use HAS_THREAD_LOCAL which was never defined. Hence, we'd basically never use the code path where we use thread_local.

Fixes #78207


Full diff: https://github.com/llvm/llvm-project/pull/97591.diff

1 Files Affected:

  • (modified) libcxxabi/src/cxa_exception_storage.cpp (+1-1)
diff --git a/libcxxabi/src/cxa_exception_storage.cpp b/libcxxabi/src/cxa_exception_storage.cpp
index 2479f550e09ef..c842da195accb 100644
--- a/libcxxabi/src/cxa_exception_storage.cpp
+++ b/libcxxabi/src/cxa_exception_storage.cpp
@@ -24,7 +24,7 @@ extern "C" {
 } // extern "C"
 } // namespace __cxxabiv1
 
-#elif defined(HAS_THREAD_LOCAL)
+#elif __has_feature(cxx_thread_local)
 
 namespace __cxxabiv1 {
 namespace {

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @mstorsjo may want to take a look #79542 (comment)

@mstorsjo
Copy link
Member

mstorsjo commented Jul 5, 2024

LGTM. @mstorsjo may want to take a look #79542 (comment)

Thanks; I tested this patch in some of my test environments, and it seems to not break anything, so I think it's fine.

As mentioned in #79542 (comment), this could have an effect on emutls environments; I tried to run a set of tests with emutls enabled as well. With emutls enabled (as in that linked PR), a bunch of my tests hang, both with and without this PR. So that doesn't indicate any reason to hold back this patch either.

So, in short, +1 for this patch from me, and AFAIK this will fix the same issue as https://reviews.llvm.org/D155278 tried to fix.

@ldionne ldionne merged commit acaa4c8 into llvm:main Jul 8, 2024
@ldionne ldionne deleted the review/libcxxabi-remove-HAS_THREAD_LOCAL branch July 8, 2024 20:20
@nico
Copy link
Contributor

nico commented Aug 2, 2024

In case anyone else wonders why their binaries that statically link libc++abi no longer dlclose() after this change, https://crbug.com/351867703 has some details on that.

(No action needed for libc++abi, just mentioning it in case someone else runs into this.)

dybv-sc pushed a commit to dybv-sc/llvm-project that referenced this pull request Mar 20, 2025
…or exception storage (llvm#97591)

Previously, we'd use HAS_THREAD_LOCAL which was never defined. Hence,
we'd basically never use the code path where we use thread_local.

Fixes llvm#78207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libcxxabi] HAS_THREAD_LOCAL is not defined by cmake
5 participants