[sanitizer] Use _thread_db_sizeof_pthread to obtain struct pthread size

[fweimer]

Summary

This symbol has been exported (as an internal GLIBC_PRIVATE symbol) from libc.so.6 starting with glibc 2.34. glibc uses it internally for its libthread_db implementation to enable thread debugging on GDB, so it is unlikely to go away for now.

Fixes #52989.

[MaskRay]

So now you see why I requested https://sourceware.org/pipermail/libc-alpha/2021-December/134456.html :)

Please include more context for future patches: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[fweimer]

Would someone please commit this patch for me? Thanks.

[MaskRay]

In D119007#3298072, @fweimer wrote:

Would someone please commit this patch for me? Thanks.

I'd like a sanitizer maintainer (e.g. @vitalybuka) to have a look.

I personally tested this on x86-64 and aarch64 on glibc 2.33 (the _thread_db_sizeof_pthread code path is not taken).
Have you tested check-asan check-msan check-tsan check-lsan with glibc 2.34?

[fweimer]

In D119007#3298258, @MaskRay wrote:

In D119007#3298072, @fweimer wrote:

Would someone please commit this patch for me? Thanks.

I'd like a sanitizer maintainer (e.g. @vitalybuka) to have a look.

Sorry for jumping the gun here. Phabricator told me that *This revision is now accepted and ready to land.*, so I assumed I was done.

I personally tested this on x86-64 and aarch64 on glibc 2.33 (the _thread_db_sizeof_pthread code path is not taken).
Have you tested check-asan check-msan check-tsan check-lsan with glibc 2.34?

I have checked that the correct path is taken on x86-64 on both glibc 2.28 and glibc 2.34. In the glibc 2.34 case, dlsym is used, and returns the same constant as the hard-coded one. I have since returned the lab machines, but I guess I can try once more and run those tests.

EDIT I see two test failures on glibc 2.34 (from Fedora 35), but they aren't regressions:

Failed Tests (2):
  AddressSanitizer-i386-linux :: TestCases/Linux/odr-vtable.cpp
  AddressSanitizer-i386-linux :: TestCases/strcat-overlap.cpp

[MaskRay]

In D119007#3300424, @fweimer wrote:

In D119007#3298258, @MaskRay wrote:

In D119007#3298072, @fweimer wrote:

Would someone please commit this patch for me? Thanks.

I'd like a sanitizer maintainer (e.g. @vitalybuka) to have a look.

Sorry for jumping the gun here. Phabricator told me that *This revision is now accepted and ready to land.*, so I assumed I was done.

Phabricator has loose permission management.
Anyone accepting the patch will make it green, unless there is a blocking reviewer.

What applies is this rule https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted :

"Every patch should be reviewed by at least one technical expert in the areas of the project affected by the change."

From my observation, @vitalybuka usually wants to keep an eye on portability changes like this... So I think it's a good idea that he can take a look.
Also, we'd need this in release/14.x (14.0.0-rc1 is scheduled tomorrow?), so additional testing is definitely appreciated (as you have done below, thanks!)...

I personally tested this on x86-64 and aarch64 on glibc 2.33 (the _thread_db_sizeof_pthread code path is not taken).
Have you tested check-asan check-msan check-tsan check-lsan with glibc 2.34?

I have checked that the correct path is taken on x86-64 on both glibc 2.28 and glibc 2.34. In the glibc 2.34 case, dlsym is used, and returns the same constant as the hard-coded one. I have since returned the lab machines, but I guess I can try once more and run those tests.

EDIT I see two test failures on glibc 2.34 (from Fedora 35), but they aren't regressions:

Failed Tests (2):
  AddressSanitizer-i386-linux :: TestCases/Linux/odr-vtable.cpp
  AddressSanitizer-i386-linux :: TestCases/strcat-overlap.cpp

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

297

if (psizeof)

[MaskRay]

> if (psizeof)

remove unneeded formatting

[paulkirth]

We're seeing a breakage in Fuchsia's Clang canary builder on Linux that seem related to this change.

The error output is:

156/182] Checking TSan Go runtime...
FAILED: compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck /b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck 
cd /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/tsan/go && env "CC=/b/s/w/ir/x/w/staging/llvm_build/./bin/clang " EXTRA_CFLAGS= IN_TMPDIR=1 SILENT=1 /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/tsan/rtl/../go/buildgo.sh
ld.lld: error: undefined symbol: dlsym
>>> referenced by gotsan.cpp
>>>               /tmp/gotsan.Irz2GAAcvm/race_linux_amd64.syso:(__sanitizer::ThreadDescriptorSize())
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

The builder can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8822761889490955409/overview

If the problem is difficult to address, would you mind reverting until it can be fixed?

[MaskRay]

In D119007#3306179, @paulkirth wrote:

We're seeing a breakage in Fuchsia's Clang canary builder on Linux that seem related to this change.

The error output is:

156/182] Checking TSan Go runtime...
FAILED: compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck /b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck 
cd /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/tsan/go && env "CC=/b/s/w/ir/x/w/staging/llvm_build/./bin/clang " EXTRA_CFLAGS= IN_TMPDIR=1 SILENT=1 /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/tsan/rtl/../go/buildgo.sh
ld.lld: error: undefined symbol: dlsym
>>> referenced by gotsan.cpp
>>>               /tmp/gotsan.Irz2GAAcvm/race_linux_amd64.syso:(__sanitizer::ThreadDescriptorSize())
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

The builder can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8822761889490955409/overview

If the problem is difficult to address, would you mind reverting until it can be fixed?

Should be fixed by f0cdacd99bd41dbd778099dfe303ba92660826b3.
Looks like check-tsan passes even if I skip the entire thread stuff for the SANITIZER_GO code path...
(I know nearly nothing about the SANITIZER_GO code path)

[paulkirth]

Thanks for the quick fix. Looks like this is no longer an issue.

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

 
 // On glibc x86_64, ThreadDescriptorSize() needs to be precise due to the usage
 // of g_tls_size. On other targets, ThreadDescriptorSize() is only used by lsan
 // to get the pointer to thread-specific data keys in the thread control block.
 #if (SANITIZER_FREEBSD || SANITIZER_LINUX) && !SANITIZER_ANDROID
 // sizeof(struct pthread) from glibc.
 static atomic_uintptr_t thread_descriptor_size;
 
-uptr ThreadDescriptorSize() {
-  uptr val = atomic_load_relaxed(&thread_descriptor_size);
-  if (val)
-    return val;
+static uptr ThreadDescriptorSizeFallback() {
+  uptr val = 0;
 #if defined(__x86_64__) || defined(__i386__) || defined(__arm__)
   int major;
   int minor;
   int patch;
   if (GetLibcVersion(&major, &minor, &patch) && major == 2) {
     /* sizeof(struct pthread) values from various glibc versions.  */
     if (SANITIZER_X32)
       val = 1728; // Assume only one particular version for x32.
#elif SANITIZER_RISCV64
   }
 
 #elif defined(__aarch64__)
   // The sizeof (struct pthread) is the same from GLIBC 2.17 to 2.22.
   val = 1776;
 #elif defined(__powerpc64__)
   val = 1776; // from glibc.ppc64le 2.20-8.fc21
 #endif
+  return val;
+}
+
+uptr ThreadDescriptorSize() {
+  uptr val = atomic_load_relaxed(&thread_descriptor_size);
   if (val)
-    atomic_store_relaxed(&thread_descriptor_size, 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();
   return val;
 }
 
 #if defined(__mips__) || defined(__powerpc64__) || SANITIZER_RISCV64
 // TlsPreTcbSize includes size of struct pthread_descr and size of tcb
 // head structure. It lies before the static tls blocks.
 static uptr TlsPreTcbSize() {
 #if defined(__mips__)