Skip to content
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

[sanitizer_common] Provide dummy ThreadDescriptorSize on Solaris #109285

Conversation

rorth
Copy link
Collaborator

@rorth rorth commented Sep 19, 2024

Since 2c69a09, the Solaris build is broken like

Undefined			first referenced
 symbol  			    in file
_ZN11__sanitizer20ThreadDescriptorSizeEv projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonLibc.i386.dir/sanitizer_linux_libcdep.cpp.o

The ThreadDescriptorSize reference is from sanitizer_linux_libcdep.cpp (GetTls), l.590. This isn't actually needed on non-glibc targets AFAICS, so this patch provides a dummy to restore the build.

Tested on sparcv9-sun-solaris2.11, amd64-pc-solaris2.11, and x86_64-pc-linux-gnu.

Since 2c69a09, the Solaris build is broken
like
```
Undefined			first referenced
 symbol  			    in file
_ZN11__sanitizer20ThreadDescriptorSizeEv projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonLibc.i386.dir/sanitizer_linux_libcdep.cpp.o
```
The `ThreadDescriptorSize` reference is from `sanitizer_linux_libcdep.cpp`
(`GetTls`), l.590.  This isn't actually needed on non-glibc targets AFAICS,
so this patch provides a dummy to restore the build.

Tested on `sparcv9-sun-solaris2.11`, `amd64-pc-solaris2.11`, and
`x86_64-pc-linux-gnu`.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 19, 2024

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

Author: Rainer Orth (rorth)

Changes

Since 2c69a09, the Solaris build is broken like

Undefined			first referenced
 symbol  			    in file
_ZN11__sanitizer20ThreadDescriptorSizeEv projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonLibc.i386.dir/sanitizer_linux_libcdep.cpp.o

The ThreadDescriptorSize reference is from sanitizer_linux_libcdep.cpp (GetTls), l.590. This isn't actually needed on non-glibc targets AFAICS, so this patch provides a dummy to restore the build.

Tested on sparcv9-sun-solaris2.11, amd64-pc-solaris2.11, and x86_64-pc-linux-gnu.


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

1 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp (+2-1)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
index aa156acd7b657a..4fc99197aae3d5 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -226,7 +226,7 @@ static void GetGLibcVersion(int *major, int *minor, int *patch) {
 // sizeof(struct pthread) from glibc.
 static uptr thread_descriptor_size;
 
-// FIXME: Implementation is very GLIBC specific, but it's used by FREEBSD.
+// FIXME: Implementation is very GLIBC specific, but it's used by FreeBSD.
 static uptr ThreadDescriptorSizeFallback() {
 #    if defined(__x86_64__) || defined(__i386__) || defined(__arm__) || \
         SANITIZER_RISCV64
@@ -364,6 +364,7 @@ static uptr TlsPreTcbSize() {
 #    endif
 #  else   // (SANITIZER_FREEBSD || SANITIZER_GLIBC) && !SANITIZER_GO
 void InitTlsSize() {}
+uptr ThreadDescriptorSize() { return 0; }
 #  endif  // (SANITIZER_FREEBSD || SANITIZER_GLIBC) && !SANITIZER_GO
 
 #  if (SANITIZER_FREEBSD || SANITIZER_LINUX || SANITIZER_SOLARIS) && \

@rorth
Copy link
Collaborator Author

rorth commented Sep 19, 2024

This (and the previous) Solaris build breakage are a consequence of thecompiler-rt source organisation, I belive: quite a number of source files with linux in their name are either not Linux-specific at all or contain just a few lines of non-Linux content, which is both very confusing. It's extremely difficult to find one's way through the resulting maze of #ifdefs, not breaking non-Linux targets while working on the Linux code.

@rorth
Copy link
Collaborator Author

rorth commented Sep 20, 2024

The Solaris builds have been broken for 3 days now, and I got no reaction whatsoever.

Given the trivial nature of the patch and the testing done, I'll commit the patch now to unbreak the builds.

@rorth rorth merged commit f322f4a into llvm:main Sep 20, 2024
10 checks passed
@vitalybuka
Copy link
Collaborator

The Solaris builds have been broken for 3 days now, and I got no reaction whatsoever.

Given the trivial nature of the patch and the testing done, I'll commit the patch now to unbreak the builds.

Thanks for fixing!
Sorry, I didn't notice the email on time.
Don't hesitate to revert or fix such trivial matters without a review.

@vitalybuka
Copy link
Collaborator

Oh, I was confused how Solaris references to the function are possible, I looked through the code before landing 2c69a09.

But the reason is that some references to the function are guarded with if (SANITIZER_GLIBC) instead of #ifdef SANITIZER_GLIBC.

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.

3 participants