This is an archive of the discontinued LLVM Phabricator instance.

[CompilerRT] Sanitizer compilation on musl systems
Needs RevisionPublic

Authored by vchuravy on May 7 2021, 5:23 PM.

Details

Reviewers
MaskRay
Summary

For Julia we cross-compile LLVM to musl and this fixes some compilation issues we run into with LLVM 12.
The changes for sanitizer_platform_limits_posix.h are needed for 32-bit in particular and the change in
sanitizer_platform_limits_posix.cpp is due to two different version of the sysinfo struct being included.

Diff Detail

Event Timeline

vchuravy requested review of this revision.May 7 2021, 5:23 PM
vchuravy created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2021, 5:23 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vchuravy added a subscriber: Restricted Project.May 7 2021, 5:28 PM
vchuravy added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
478

Any better way to test for musl?

MaskRay added inline comments.May 9 2021, 9:09 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
478

Does this fix some #include <dirent.h> tests?

SANITIZER_LINUX && !SANITIZER_GLIBC if this is glibc specific, though I wonder why defined(__x86_64__) is here.

vchuravy added inline comments.May 10 2021, 10:06 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
478

Yeah, on 32bit musl the static checks were failing for dirent

MaskRay requested changes to this revision.Jul 28 2021, 8:39 PM
This revision now requires changes to proceed.Jul 28 2021, 8:39 PM

@ccross is building musl runtimes for Android host tools and we seem to need the following diff. The SANITIZER_LINUX && !SANITIZER_GLIBC idiom may not apply everywhere since it'd include Android as well. @vchuravy @MaskRay any thoughts? We could also consider an explicit CMake variable similar to _LIBCPP_HAS_MUSL_LIBC.

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.
h
index d0db0129d4af..2040bbfd4aab 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -177,7 +177,7 @@ typedef long pid_t;
 typedef int pid_t;
 #endif
 
-#if SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_MAC ||             \
+#if SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_MAC || SANITIZER_MUSL || \
     (SANITIZER_SOLARIS && (defined(_LP64) || _FILE_OFFSET_BITS == 64)) || \
     (SANITIZER_LINUX && (defined(__x86_64__) || defined(__hexagon__)))
 typedef u64 OFF_T;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
index 3153de34e5a3..1f4c4dd95475 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
@@ -116,6 +116,13 @@
 # define SANITIZER_FUCHSIA 0
 #endif
 
+// If Linux and not glibc or android assume musl
+#if SANITIZER_LINUX && !SANITIZER_GLIBC && !SANITIZER_ANDROID
+# define SANITIZER_MUSL 1
+#else
+# define SANITIZER_MUSL 0
+#endif
+
 #define SANITIZER_POSIX \
   (SANITIZER_FREEBSD || SANITIZER_LINUX || SANITIZER_MAC || \
     SANITIZER_NETBSD || SANITIZER_SOLARIS)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
index d69b344dd613..97f026977e90 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -370,7 +370,7 @@ struct __sanitizer_group {
   char **gr_mem;
 };
 
-#  if (defined(__x86_64__) && !defined(_LP64)) || defined(__hexagon__)
+#if SANITIZER_MUSL || (defined(__x86_64__) && !defined(_LP64)) || defined(__hexagon__)
 typedef long long __sanitizer_time_t;
 #else
 typedef long __sanitizer_time_t;
@@ -478,7 +478,7 @@ struct __sanitizer_dirent {
   unsigned short d_reclen;
   // more fields that we don't care about
 };
-#  elif SANITIZER_ANDROID || defined(__x86_64__) || defined(__hexagon__)
+#  elif SANITIZER_ANDROID || SANITIZER_MUSL || defined(__x86_64__) || defined(__hexagon__)
 struct __sanitizer_dirent {
   unsigned long long d_ino;
   unsigned long long d_off;

Hi, nice to see that musl gets more use. Can you clarify the Android use case? SANITIZER_ANDROID is defined while musl is used?

When I was porting sanitizers to musl, I mostly take SANITIZER_ANDROID as whatever available in my cloned bionic (git clone https://android.googlesource.com/platform/bionic)
(I do not have an Android environment.)

pirama added a comment.Feb 8 2022, 2:07 PM

Hi, nice to see that musl gets more use. Can you clarify the Android use case? SANITIZER_ANDROID is defined while musl is used?

You had suggested in a comment to use SANITIZER_LINUX && !SANITIZER_GLIBC to detect MUSL but bionic would pass that check as well. If bionic and musl have different types/definitions, we'd then need SANITIZER_LINUX && !SANITIZER_GLIBC && !SANITIZER_ANDROID, as used in the diff I posted.

When I was porting sanitizers to musl, I mostly take SANITIZER_ANDROID as whatever available in my cloned bionic (git clone https://android.googlesource.com/platform/bionic)
(I do not have an Android environment.)

That should mostly work for compiling with bionic headers. You could use an NDK sysroot (e.g. https://android.googlesource.com/toolchain/prebuilts/ndk/r23/+/refs/heads/master/toolchains/llvm/prebuilt/linux-x86_64/sysroot/) to get the libraries as well.

Hi, nice to see that musl gets more use. Can you clarify the Android use case? SANITIZER_ANDROID is defined while musl is used?

You had suggested in a comment to use SANITIZER_LINUX && !SANITIZER_GLIBC to detect MUSL but bionic would pass that check as well. If bionic and musl have different types/definitions, we'd then need SANITIZER_LINUX && !SANITIZER_GLIBC && !SANITIZER_ANDROID, as used in the diff I posted.

Not defining a musl macro is to follow its spirit https://wiki.musl-libc.org/faq.html "Why is there no MUSL macro?"

From my experience, most issues are related to glibc specific extensions or behaviors, less related to musl behaviors.
It seems that Android bionic may port quite a few glibc specific extensions as well.

So in many places SANITIZER_GLIBC || SANITIZER_ANDROID may be more appropriate than !SANITIZER_MUSL.

When I was porting sanitizers to musl, I mostly take SANITIZER_ANDROID as whatever available in my cloned bionic (git clone https://android.googlesource.com/platform/bionic)
(I do not have an Android environment.)

That should mostly work for compiling with bionic headers. You could use an NDK sysroot (e.g. https://android.googlesource.com/toolchain/prebuilts/ndk/r23/+/refs/heads/master/toolchains/llvm/prebuilt/linux-x86_64/sysroot/) to get the libraries as well.

I will try to figure out how to have a local build environment at some point.
I still occasionally build with musl and need to ensure I do not break Android.

pirama added a comment.Feb 8 2022, 3:13 PM

Hi, nice to see that musl gets more use. Can you clarify the Android use case? SANITIZER_ANDROID is defined while musl is used?

You had suggested in a comment to use SANITIZER_LINUX && !SANITIZER_GLIBC to detect MUSL but bionic would pass that check as well. If bionic and musl have different types/definitions, we'd then need SANITIZER_LINUX && !SANITIZER_GLIBC && !SANITIZER_ANDROID, as used in the diff I posted.

Not defining a musl macro is to follow its spirit https://wiki.musl-libc.org/faq.html "Why is there no MUSL macro?"

From my experience, most issues are related to glibc specific extensions or behaviors, less related to musl behaviors.
It seems that Android bionic may port quite a few glibc specific extensions as well.

So in many places SANITIZER_GLIBC || SANITIZER_ANDROID may be more appropriate than !SANITIZER_MUSL.

I agree with the spirit of the policy but it feels like a multi-platform project like compiler-rt which supports a lot of configurations could use a preprocessor check to improve readability. The check:

#if SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_MAC || SANITIZER_MUSL || \
     (SANITIZER_SOLARIS && (defined(_LP64) || _FILE_OFFSET_BITS == 64)) || \
     (SANITIZER_LINUX && (defined(__x86_64__) || defined(__hexagon__)))

looks complicated enough without SANITIZER_MUSL. But will give your suggestion a try.

When I was porting sanitizers to musl, I mostly take SANITIZER_ANDROID as whatever available in my cloned bionic (git clone https://android.googlesource.com/platform/bionic)
(I do not have an Android environment.)

That should mostly work for compiling with bionic headers. You could use an NDK sysroot (e.g. https://android.googlesource.com/toolchain/prebuilts/ndk/r23/+/refs/heads/master/toolchains/llvm/prebuilt/linux-x86_64/sysroot/) to get the libraries as well.

I will try to figure out how to have a local build environment at some point.
I still occasionally build with musl and need to ensure I do not break Android.

Seems like the more controversial preprocessor check in dirent was done in https://reviews.llvm.org/D119358, but we never got around to fixing the two header issues? We (Julia) are still carrying this patch downstream for just those two lines. @vchuravy can you rebase this with just those lines so we can close this?

Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 9:49 AM
Herald added a subscriber: Enna1. · View Herald Transcript