Skip to content

[sanitizer] Switch from lazy ThreadDescriptorSize #108923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Sep 17, 2024

ThreadDescriptorSize uses dlsym which may use
malloc in unexpected time.

It's relatively easy to init size with regular
init.

@llvmbot
Copy link
Member

llvmbot commented Sep 17, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Vitaly Buka (vitalybuka)

Changes

ThreadDescriptorSize uses dlsym which may use
malloc in unexpected time.

It's relatively easy to init size with regular
init.

This reverts commit 9f40cadf1bfd2cc5baf3058b13a554c7535f9acf.


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

2 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp (+15-15)
  • (modified) compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cpp (+2)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
index 6b43fea507a401..aa156acd7b657a 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -224,7 +224,7 @@ static void GetGLibcVersion(int *major, int *minor, int *patch) {
 // to get the pointer to thread-specific data keys in the thread control block.
 #  if (SANITIZER_FREEBSD || SANITIZER_GLIBC) && !SANITIZER_GO
 // sizeof(struct pthread) from glibc.
-static atomic_uintptr_t thread_descriptor_size;
+static uptr thread_descriptor_size;
 
 // FIXME: Implementation is very GLIBC specific, but it's used by FREEBSD.
 static uptr ThreadDescriptorSizeFallback() {
@@ -305,20 +305,7 @@ static uptr ThreadDescriptorSizeFallback() {
 #    endif
 }
 
-uptr ThreadDescriptorSize() {
-  uptr val = atomic_load_relaxed(&thread_descriptor_size);
-  if (val)
-    return val;
-  // _thread_db_sizeof_pthread is a GLIBC_PRIVATE symbol that is exported in
-  // glibc 2.34 and later.
-  if (unsigned *psizeof = static_cast<unsigned *>(
-          dlsym(RTLD_DEFAULT, "_thread_db_sizeof_pthread")))
-    val = *psizeof;
-  if (!val)
-    val = ThreadDescriptorSizeFallback();
-  atomic_store_relaxed(&thread_descriptor_size, val);
-  return val;
-}
+uptr ThreadDescriptorSize() { return thread_descriptor_size; }
 
 #    if SANITIZER_GLIBC
 __attribute__((unused)) static size_t g_tls_size;
@@ -330,6 +317,15 @@ void InitTlsSize() {
   GetGLibcVersion(&major, &minor, &patch);
   g_use_dlpi_tls_data = major == 2 && minor >= 25;
 
+  if (major == 2 && minor >= 34) {
+    // _thread_db_sizeof_pthread is a GLIBC_PRIVATE symbol that is exported in
+    // glibc 2.34 and later.
+    if (unsigned *psizeof = static_cast<unsigned *>(
+            dlsym(RTLD_DEFAULT, "_thread_db_sizeof_pthread"))) {
+      thread_descriptor_size = *psizeof;
+    }
+  }
+
 #      if defined(__aarch64__) || defined(__x86_64__) || \
           defined(__powerpc64__) || defined(__loongarch__)
   auto *get_tls_static_info = (void (*)(size_t *, size_t *))dlsym(
@@ -339,7 +335,11 @@ void InitTlsSize() {
   if (get_tls_static_info)
     get_tls_static_info(&g_tls_size, &tls_align);
 #      endif
+
 #    endif  // SANITIZER_GLIBC
+
+  if (!thread_descriptor_size)
+    thread_descriptor_size = ThreadDescriptorSizeFallback();
 }
 
 #    if defined(__mips__) || defined(__powerpc64__) || SANITIZER_RISCV64 || \
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cpp
index 338c4d3bab2b04..b286ab72a5c795 100644
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cpp
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cpp
@@ -202,6 +202,8 @@ TEST(SanitizerLinux, ThreadDescriptorSize) {
   void *result;
   ASSERT_EQ(0, pthread_create(&tid, 0, thread_descriptor_size_test_func, 0));
   ASSERT_EQ(0, pthread_join(tid, &result));
+  EXPECT_EQ(0u, ThreadDescriptorSize());
+  InitTlsSize();
   EXPECT_EQ((uptr)result, ThreadDescriptorSize());
 }
 #  endif

Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.sanitizer-switch-from-lazy-threaddescriptorsize to main September 18, 2024 23:30
@vitalybuka vitalybuka merged commit 999313d into main Sep 18, 2024
7 of 9 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/sanitizer-switch-from-lazy-threaddescriptorsize branch September 18, 2024 23:45
tmsri pushed a commit to tmsri/llvm-project that referenced this pull request Sep 19, 2024
`ThreadDescriptorSize` uses `dlsym` which may use
malloc in unexpected time.

It's relatively easy to init size from the main init.
@mgorny
Copy link
Member

mgorny commented Oct 15, 2024

For the record, the assertion introduced here is flaky, and it fails depending on how other tests are ordered: #112399.

@vitalybuka
Copy link
Collaborator Author

For the record, the assertion introduced here is flaky, and it fails depending on how other tests are ordered: #112399.

Thanks for the report!
Unless you have a patch, I'll fix it today.

@mgorny
Copy link
Member

mgorny commented Oct 15, 2024

I don't. Feel free to fix it any way you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants