This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Define SANITIZER_GLIBC to refine SANITIZER_LINUX feature detection and support musl
ClosedPublic

Authored by MaskRay on Dec 27 2020, 6:48 PM.

Details

Summary

Several #if SANITIZER_LINUX && !SANITIZER_ANDROID guards are replaced
with the more appropriate #if SANITIZER_GLIBC (the headers are glibc
extensions, not specific to Linux (i.e. if we ever support GNU/kFreeBSD
or Hurd, the guards may automatically work)).

Several #if SANITIZER_LINUX && !SANITIZER_ANDROID guards are refined
with #if SANITIZER_GLIBC (the definitions are available on Linux glibc,
but may not be available on other libc (e.g. musl) implementations).

This patch makes ninja asan cfi lsan msan stats tsan ubsan xray build on a musl based Linux distribution (apk install musl-libintl)
Notes about disabled interceptors for musl:

  • SANITIZER_INTERCEPT_GLOB: musl does not implement GLOB_ALTDIRFUNC (GNU extension)
  • Some ioctl structs and functions operating on them.
  • SANITIZER_INTERCEPT___PRINTF_CHK: _FORTIFY_SOURCE functions are GNU extension
  • SANITIZER_INTERCEPT___STRNDUP: dlsym(RTLD_NEXT, "__strndup") errors so a diagnostic is formed. The diagnostic uses write which hasn't been intercepted => SIGSEGV
  • SANITIZER_INTERCEPT_*64: the _LARGEFILE64_SOURCE functions are glibc specific. musl does something like #define pread64 pread
  • Disabled msg_iovlen msg_controllen cmsg_len checks: musl is conforming while many implementations (Linux/FreeBSD/NetBSD/Solaris) are non-conforming. Since we pick the glibc definition, exclude the checks for musl (incompatible sizes but compatible offsets)

Pass through LIBCXX_HAS_MUSL_LIBC to make check-msan/check-tsan able to build libc++ (https://bugs.llvm.org/show_bug.cgi?id=48618).

Many sanitizer features are available now.

% ninja check-asan
(known issues:
* ASAN_OPTIONS=fast_unwind_on_malloc=0 odr-violations hangs
)
...
Testing Time: 53.69s
  Unsupported      : 185
  Passed           : 512
  Expectedly Failed:   1
  Failed           :  12

% ninja check-ubsan check-ubsan-minimal check-memprof # all passed

% ninja check-cfi
( all cross-dso/)
...
Testing Time: 8.68s
  Unsupported      : 264
  Passed           :  80
  Expectedly Failed:   8
  Failed           :  32

% ninja check-lsan
(With GetTls (D93972), 10 failures)
Testing Time: 4.09s
  Unsupported:  7
  Passed     : 65
  Failed     : 22

% ninja check-msan
(Many are due to functions not marked unsupported.)
Testing Time: 23.09s
  Unsupported      :   6
  Passed           : 764
  Expectedly Failed:   2
  Failed           :  58

% ninja check-tsan
Testing Time: 23.21s
  Unsupported      :  86
  Passed           : 295
  Expectedly Failed:   1
  Failed           :  25

Used ASAN_OPTIONS=verbosity=2 to verify there is no unneeded interceptor.

Partly based on Jari Ronkainen's https://reviews.llvm.org/D63785#1921014

Note: we need to place _FILE_OFFSET_BITS above #include "sanitizer_platform.h" to avoid #define __USE_FILE_OFFSET64 1 in 32-bit ARM features.h

Diff Detail

Event Timeline

MaskRay created this revision.Dec 27 2020, 6:48 PM
MaskRay requested review of this revision.Dec 27 2020, 6:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2020, 6:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 313820.Dec 27 2020, 8:43 PM
MaskRay retitled this revision from [sanitizer-common] Use __GLIBC__ to improve feature detection to [sanitizer-common] Use __GLIBC__ to improve SANITIZER_LINUX feature detection.
MaskRay edited the summary of this revision. (Show Details)

.

MaskRay edited the summary of this revision. (Show Details)Dec 27 2020, 8:45 PM
MaskRay added a subscriber: dalias.
MaskRay added subscribers: mjeveritt, ronchaine.
MaskRay added a comment.EditedDec 27 2020, 10:41 PM

I pushed some trivial portability fixes.

Made some tests on Alpine Linux (musl based distribution) with this patch.

There is a GetTls assertion failure. Simply making
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:GetTls return 0
gets sanitizers into a quite decent situation.

% ninja check-asan
...
Testing Time: 332.62s
  Unsupported      : 177
  Passed           : 482
  Expectedly Failed:   1
  Failed           :  20

% ninja check-ubsan
... (TestCases/Misc/monitor.cpp failed)
Testing Time: 98.92s
  Unsupported      :  50
  Passed           : 596
  Expectedly Failed:   2
  Failed           :   8

% ninja check-memprof # all passed

% ninja check-cfi
...
Testing Time: 8.68s
  Unsupported      : 264
  Passed           :  80
  Expectedly Failed:   8
  Failed           :  32

check-xray all failed.

For msan/tsan, I'll need to figure out some libc++ related compile issues.


About GetTlsSize. It is used in two places (1) msan/tsan/lsan call AdjustStackSize (https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp#L407) in their pthread_create interceptors (https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/msan/msan_interceptors.cpp#L1037 ) (2) https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp#L547-L555 (this looks suspicious and can probably be removed; sent D93866)

MaskRay edited the summary of this revision. (Show Details)

Well... technically speaking GLIBC is at least: GNU/Linux, GNU/FreeBSD and HURD.

Well... technically speaking GLIBC is at least: GNU/Linux, GNU/FreeBSD and HURD.

Hurd and GNU/kFreeBSD are not supported. Even if they do, many defined(__GLIBC__) is inside a #if SANITIZER_LINUX, so they will work automatically.

/usr/include/features.h is needed to get __GLIBC__. tsan is built with a standalone sysroot so /usr/include/features.h is unavailable. So it is not straightforward to create a SANITIZER_GLIBC in lib/sanitizer_common/sanitizer_platform.h

Well... technically speaking GLIBC is at least: GNU/Linux, GNU/FreeBSD and HURD.

When I made the original patch, I was looking through what was defined in what standard, and SANITIZER_GNU (or __GLIBC__) here
without SANITIZER_LINUX should be available on those platforms. I can't say for certain since it's been a while since I made that, but
it *should* work on those platforms too. I used SANITIZER_LINUX && SANITIZER_GNU when both were needed by the standard.

If I made a mistake with those, I would concider it a bug.

I would also do declare the SANITIZER_GNU as well, while it can be just __GLIBC__ here in practice, it is not technically the same,
and it's more in line with the style of the rest of the file. It also allows some future-proofing if some platform is crazy enough to provide
GNU extensions without defining __GLIBC__ it could just be added to where we declare SANITIZER_GNU instead of needing to
figure out how to refactor the file again. (i.e. use SANITIZER_GNU to mean "we have GNU extensions")

MaskRay added a comment.EditedDec 28 2020, 10:51 AM

Well... technically speaking GLIBC is at least: GNU/Linux, GNU/FreeBSD and HURD.

When I made the original patch, I was looking through what was defined in what standard, and SANITIZER_GNU (or __GLIBC__) here
without SANITIZER_LINUX should be available on those platforms. I can't say for certain since it's been a while since I made that, but
it *should* work on those platforms too. I used SANITIZER_LINUX && SANITIZER_GNU when both were needed by the standard.

If I made a mistake with those, I would concider it a bug.

I would also do declare the SANITIZER_GNU as well, while it can be just __GLIBC__ here in practice, it is not technically the same,
and it's more in line with the style of the rest of the file. It also allows some future-proofing if some platform is crazy enough to provide
GNU extensions without defining __GLIBC__ it could just be added to where we declare SANITIZER_GNU instead of needing to
figure out how to refactor the file again. (i.e. use SANITIZER_GNU to mean "we have GNU extensions")

I think having GLIBC in the macro name is important. There are GNU extensions and glibc headers. The problem with GNU is that it is less specific. Some platforms (FreeBSD) have implemented many GNU extensions (compiler extensions/ELF extensions) but may not provide the header files (obstack.h fstab.h ...; Some of the headers collide with Linux headers). For such headers, I guard them with nested #if SANITIZER_LINUX # ifdef __GLIBC__.

For things may likely work on non-Linux glibc, I just use`#ifdef GLIBC`, e.g.

#ifdef __GLIBC__
int glob_nomatch = GLOB_NOMATCH;  // musl has this but not the following, for simplicity just exclude all
int glob_altdirfunc = GLOB_ALTDIRFUNC;
#endif

In the end I still feel the current organization is great:)

Well... technically speaking GLIBC is at least: GNU/Linux, GNU/FreeBSD and HURD.

Hurd and GNU/kFreeBSD are not supported. Even if they do, many defined(__GLIBC__) is inside a #if SANITIZER_LINUX, so they will work automatically.

At least, GNU/kFreeBSD is supported in other components of LLVM and introducing it in sanitizers should be relatively simple. However I don't plan to maintain these targets myself.

I would also do declare the SANITIZER_GNU as well, while it can be just __GLIBC__ here in practice, it is not technically the same,
and it's more in line with the style of the rest of the file.

Please beware that __GNU__ is HURD, not Linux, thus SANITIZER_GNU should be HURD.

ronchaine added a comment.EditedDec 28 2020, 12:21 PM

I think having GLIBC in the macro name is important. There are GNU extensions and glibc headers. The problem with GNU is that it is less specific. Some platforms (FreeBSD) have implemented many GNU extensions (compiler extensions/ELF extensions) but may not provide the header files (obstack.h fstab.h ...; Some of the headers collide with Linux headers). For such headers, I guard them with nested #if SANITIZER_LINUX # ifdef __GLIBC__.

Yeah, that makes sense.

In the end I still feel the current organization is great:)

I just feel like the defines should follow the specifications, otherwise this is just pushing the problem to the next libc.
If the defines followed specifications in the first place, musl wouldn't have needed special treatment to compile.

Please beware that GNU is HURD, not Linux, thus SANITIZER_GNU should be HURD.

Makes sense, I would just like that there would be something else than the __GLIBC__ macro, that would be
controlled inside platform interceptors, like the other defines there. I don't really care that much what it would
be named.

In the end I still feel the current organization is great:)

I just feel like the defines should follow the specifications, otherwise this is just pushing the problem to the next libc.
If the defines followed specifications in the first place, musl wouldn't have needed special treatment to compile.

Yes, I am aware. Several __GLIBC__ guards are really indeed glibc mess (conflicting with Linux header; likely available on other OSes glibc supports) so glibc instead of GNU is more appropriate.
These are things I think are highly glibc specific and other libc implementation adopting a few GNU extensions may not support (or even if they do decide support, annotating that on top of this patch should not be too difficult).

vitalybuka added inline comments.Dec 28 2020, 1:10 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
272 ↗(On Diff #313820)
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
170–171

please clang format the patch and resolve clang-tidy warnings

LGTM, I'll accept in a day or so if no other feedback

MaskRay added inline comments.Dec 28 2020, 1:21 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
272 ↗(On Diff #313820)

*Nod*
Will fix once the other comment is answered

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
170–171

The clang-tidy warning asks me to remove the spaces in # include.

IMO this will make it less readable. If you it looks fine I'll delete the spaces.

vitalybuka added a comment.EditedDec 28 2020, 1:37 PM

check-asan on debian complains like thins:

ninja: Entering directory `../out/z/'
[0/1] Running the AddressSanitizer tests
FAIL: AddressSanitizer-i386-linux :: TestCases/Linux/pthread_create_version.cpp (551 of 1438)
******************** TEST 'AddressSanitizer-i386-linux :: TestCases/Linux/pthread_create_version.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';      /usr/local/google/home/vitalybuka/src/llvm.git/out/z/./bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  -m32  -std=c++11 -pthread /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/asan/TestCases/Linux/pthread_create_version.cpp -o /usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/pthread_create_version.cpp.tmp &&  /usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/pthread_create_version.cpp.tmp 2>&1
--
Exit Code: 1

Command Output (stdout):
--
AddressSanitizer:DEADLYSIGNAL
=================================================================
==974260==ERROR: AddressSanitizer: SEGV on unknown address 0xfffffb40 (pc 0xf7bcca11 bp 0xffb65548 sp 0xffb654c0 T0)
==974260==The signal is caused by a WRITE memory access.
    #0 0xf7bcca11 in pthread_create@@GLIBC_2.1 (/lib/i386-linux-gnu/libpthread.so.0+0x8a11)
    #1 0xf7bcd42b in pthread_create@GLIBC_2.0 (/lib/i386-linux-gnu/libpthread.so.0+0x942b)
    #2 0x80caff0 in pthread_create /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:230:14
    #3 0x81186b7 in main /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/asan/TestCases/Linux/pthread_create_version.cpp:19:3
    #4 0xf79c9e45 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x1ee45)
    #5 0x8065321 in _start (/usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/pthread_create_version.cpp.tmp+0x8065321)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/i386-linux-gnu/libpthread.so.0+0x8a11) in pthread_create@@GLIBC_2.1
==974260==ABORTING
vitalybuka added inline comments.Dec 28 2020, 1:40 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
170–171

Yes, that's what I am asking too. It's not worth of hassle correcting auto format in future.

MaskRay added a comment.EditedDec 28 2020, 2:08 PM

check-asan on debian complains like thins:

ninja: Entering directory `../out/z/'
[0/1] Running the AddressSanitizer tests
FAIL: AddressSanitizer-i386-linux :: TestCases/Linux/pthread_create_version.cpp (551 of 1438)
******************** TEST 'AddressSanitizer-i386-linux :: TestCases/Linux/pthread_create_version.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';      /usr/local/google/home/vitalybuka/src/llvm.git/out/z/./bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  -m32  -std=c++11 -pthread /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/asan/TestCases/Linux/pthread_create_version.cpp -o /usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/pthread_create_version.cpp.tmp &&  /usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/pthread_create_version.cpp.tmp 2>&1
--
Exit Code: 1

Command Output (stdout):
--
AddressSanitizer:DEADLYSIGNAL
=================================================================
==974260==ERROR: AddressSanitizer: SEGV on unknown address 0xfffffb40 (pc 0xf7bcca11 bp 0xffb65548 sp 0xffb654c0 T0)
==974260==The signal is caused by a WRITE memory access.
    #0 0xf7bcca11 in pthread_create@@GLIBC_2.1 (/lib/i386-linux-gnu/libpthread.so.0+0x8a11)
    #1 0xf7bcd42b in pthread_create@GLIBC_2.0 (/lib/i386-linux-gnu/libpthread.so.0+0x942b)
    #2 0x80caff0 in pthread_create /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:230:14
    #3 0x81186b7 in main /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/asan/TestCases/Linux/pthread_create_version.cpp:19:3
    #4 0xf79c9e45 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x1ee45)
    #5 0x8065321 in _start (/usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/pthread_create_version.cpp.tmp+0x8065321)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/i386-linux-gnu/libpthread.so.0+0x8a11) in pthread_create@@GLIBC_2.1
==974260==ABORTING

The problem is that __GLIBC__ is not defined in lib/interception/interception_linux.h (so the unversioned pthread_create is selected and fails on i386 rL248325).

#include <features.h> (or any other glibc header) needs to be included to get __GLIBC__. However, ninja tsan specifies --sysroot=. (lib/tsan/CMakeLists.txt:246) and /usr/include is not in the search path so features.h cannot be found by tsan.

Any thought making __GLIBC__ available on tsan?

Can we add #include "sanitizer_common/sanitizer_glibc_version.h" into interception_linux.h ?

MaskRay updated this revision to Diff 313907.Dec 28 2020, 2:57 PM
MaskRay edited the summary of this revision. (Show Details)

Address comments.

Properly set GLIBC in lib/interception/interception_linux.h

vitalybuka added inline comments.
compiler-rt/lib/interception/interception_linux.h
24 ↗(On Diff #313907)

@eugenis @dvyukov I propose to remove sysroot=. from tsan. There was idea that we should do that for all sanitizers to avoid random unwanted includes, but now it looks to me as unnecessary hassle.

krytarowski added inline comments.Dec 28 2020, 3:42 PM
compiler-rt/lib/interception/interception_linux.cpp
67

dlvsym is available on NetBSD too.

compiler-rt/lib/interception/interception_linux.h
48 ↗(On Diff #313907)

dlvsym is available on NetBSD too.

emaste added inline comments.Dec 28 2020, 4:27 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
243 ↗(On Diff #313907)

FreeBSD has glob(3), NetBSD probably does but I'm not certain. Is there an issue with intercepting glob there?

272 ↗(On Diff #313820)

!LINUX_NOT_ANDROID seems confusing; how do FreeBSD/NetBSD factor into ioctl?

MaskRay updated this revision to Diff 313911.Dec 28 2020, 4:28 PM
MaskRay edited the summary of this revision. (Show Details)

Enable dlvsym on NetBSD. Some other adjustment

MaskRay added inline comments.Dec 28 2020, 4:29 PM
compiler-rt/lib/interception/interception_linux.h
24 ↗(On Diff #313907)

I second the idea.

It caused hassle to FreeBSD and NetBSD
https://bugs.llvm.org/show_bug.cgi?id=26651

MaskRay added inline comments.Dec 28 2020, 4:34 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
243 ↗(On Diff #313907)

This is related to

#ifdef __GLIBC__
int glob_nomatch = GLOB_NOMATCH;
int glob_altdirfunc = GLOB_ALTDIRFUNC;
#endif

glob is simple so I can send a separate patch for FreeBSD. I'll not make functional changes for *BSD in this patch, though.

272 ↗(On Diff #313820)

ioctl is very complex. It requires many headers. This patch intends to be a NFC for non-Linux and I'll not touch ioctl for FreeBSD.

MaskRay updated this revision to Diff 313933.Dec 28 2020, 8:36 PM
MaskRay retitled this revision from [sanitizer-common] Use __GLIBC__ to improve SANITIZER_LINUX feature detection to [sanitizer] Use __GLIBC__ to improve SANITIZER_LINUX feature detection.
MaskRay edited the summary of this revision. (Show Details)

Drop --sysroot=.
Mark more glibc specific functions. Tested with ASAN_OPTIONS=verbosity=2.
Pushed a semarate commit. ninja check-ubsan passed now
UNSUPPORTED some tests separately. ninja check-asan has only 13 failures now (several symbolizer tests are related to a dead futex __sanitizer::BufferedStackTrace::Unwind->_Unwind_Backtrace->_Unwind_Find_FDE->__pthread_mutex_timedlock->__timedwait->__timedwait_cp->__futex4_cp. I don't investigate further)

MaskRay marked 6 inline comments as done.Dec 28 2020, 8:37 PM
MaskRay updated this revision to Diff 313934.Dec 28 2020, 9:38 PM
MaskRay edited the summary of this revision. (Show Details)

Make check-msan build

MaskRay edited the summary of this revision. (Show Details)Dec 28 2020, 9:43 PM
MaskRay updated this revision to Diff 313977.Dec 29 2020, 9:59 AM
MaskRay edited the summary of this revision. (Show Details)

Make ioctl work (some like struct termio have to be disabled)
FreeBSD does not have features.h. Add a guard
Tested Android

MaskRay marked an inline comment as done.Dec 29 2020, 10:05 AM
MaskRay added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
243 ↗(On Diff #313907)

sanitizer_platform_limits_posix.h:752

__sanitizer_glob_t does not match FreeBSD include/glob.h. So glob cannot be enabled on FreeBSD.

MaskRay updated this revision to Diff 313988.Dec 29 2020, 11:09 AM
MaskRay edited the summary of this revision. (Show Details)

Enable sendmsg/recvmsg on musl

Most implementations are non-conforming (FreeBSD/NetBSD/OpenBSD/Solaris/macOS/Linux). Exclude the size checks for the conforming musl fields.

vitalybuka added inline comments.Dec 29 2020, 1:55 PM
compiler-rt/lib/tsan/CMakeLists.txt
252 ↗(On Diff #313988)

please extract this into a separate patch

MaskRay updated this revision to Diff 314018.Dec 29 2020, 2:10 PM
MaskRay edited the summary of this revision. (Show Details)

Rebase on D93921 (Remove --sysroot=.)

vitalybuka added inline comments.Dec 29 2020, 2:29 PM
compiler-rt/lib/asan/asan_interceptors.h
63 ↗(On Diff #314018)

what if we define SANITIZER_GLIBC in sanitizer_platform.h

as is if we remove include features.h, defined(GLIBC) will silently calculated as false.

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
192–193 ↗(On Diff #314018)
MaskRay added inline comments.Dec 29 2020, 3:16 PM
compiler-rt/lib/asan/asan_interceptors.h
63 ↗(On Diff #314018)

An undefined macro evaluates to 0. Adding SANITIZER_GLIBC does not seem to improve the situation...

MaskRay updated this revision to Diff 314031.Dec 29 2020, 3:19 PM
MaskRay marked an inline comment as done.

Add parentheses around SI_GLIBC || something

vitalybuka added inline comments.Dec 30 2020, 12:44 PM
compiler-rt/lib/asan/asan_interceptors.h
63 ↗(On Diff #314018)

My mistake.
Still we pretty sure that platform.h is included everywhere.
and it will make look these #ifs more consistent

MaskRay updated this revision to Diff 314147.Dec 30 2020, 1:35 PM
MaskRay retitled this revision from [sanitizer] Use __GLIBC__ to improve SANITIZER_LINUX feature detection to [sanitizer] Define SANITIZER_GLIBC to refine SANITIZER_LINUX feature detection and support musl.
MaskRay edited the summary of this revision. (Show Details)

Use SANITIZER_GLIBC

MaskRay marked 3 inline comments as done.Dec 30 2020, 3:03 PM
vitalybuka accepted this revision.Dec 30 2020, 9:12 PM
This revision is now accepted and ready to land.Dec 30 2020, 9:12 PM
This revision was landed with ongoing or failed builds.Dec 31 2020, 12:44 AM
This revision was automatically updated to reflect the committed changes.

I believe this is causing issues on the Chrome Android bots:

FAILED: lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o
/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/bin/clang++ -DASAN_DYNAMIC=1 -I/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/.. --target=aarch64-linux-android21 --sysroot=/b/s/w/ir/cache/builder/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/b/s/w/ir/cache/builder/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wall -std=c++14 -Wno-unused-parameter -O3 -DNDEBUG -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -ftls-model=initial-exec -MD -MT lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o -MF lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o.d -o lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o -c /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp
/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:262:3: error: unknown type name 'fake_mallinfo'

fake_mallinfo (*mallinfo)(void);
^

/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:277:53: error: use of undeclared identifier 'interceptor_mallinfo'; did you mean 'interceptor_malloc'?

WRAP(calloc),         WRAP(free),               WRAP(mallinfo),
                                                ^~~~~~~~~~~~~~
                                                __interceptor_malloc

/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/../interception/interception.h:149:18: note: expanded from macro 'WRAP'

define WRAP(x) __interceptor_ ## x

^

<scratch space>:144:1: note: expanded from here
interceptor_mallinfo
^
/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:140:1: note: '
interceptor_malloc' declared here
INTERCEPTOR(void*, malloc, uptr size) {
^
/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/../interception/interception.h:232:12: note: expanded from macro 'INTERCEPTOR'

ret_type WRAP(func)(__VA_ARGS__)
         ^

/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/../interception/interception.h:149:18: note: expanded from macro 'WRAP'

define WRAP(x) __interceptor_ ## x

^

<scratch space>:60:1: note: expanded from here
__interceptor_malloc
^

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8859402008562911568/+/steps/gclient_runhooks/0/stdout?format=raw

I believe this is causing issues on the Chrome Android bots:

FAILED: lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o
/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/bin/clang++ -DASAN_DYNAMIC=1 -I/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/.. --target=aarch64-linux-android21 --sysroot=/b/s/w/ir/cache/builder/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/b/s/w/ir/cache/builder/src/third_party/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wall -std=c++14 -Wno-unused-parameter -O3 -DNDEBUG -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -ftls-model=initial-exec -MD -MT lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o -MF lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o.d -o lib/asan/CMakeFiles/RTAsan_dynamic.aarch64.dir/asan_malloc_linux.cpp.o -c /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp
/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:262:3: error: unknown type name 'fake_mallinfo'

fake_mallinfo (*mallinfo)(void);
^

/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:277:53: error: use of undeclared identifier 'interceptor_mallinfo'; did you mean 'interceptor_malloc'?

WRAP(calloc),         WRAP(free),               WRAP(mallinfo),
                                                ^~~~~~~~~~~~~~
                                                __interceptor_malloc

/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/../interception/interception.h:149:18: note: expanded from macro 'WRAP'

define WRAP(x) __interceptor_ ## x

^

<scratch space>:144:1: note: expanded from here
interceptor_mallinfo
^
/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:140:1: note: '
interceptor_malloc' declared here
INTERCEPTOR(void*, malloc, uptr size) {
^
/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/../interception/interception.h:232:12: note: expanded from macro 'INTERCEPTOR'

ret_type WRAP(func)(__VA_ARGS__)
         ^

/b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/../interception/interception.h:149:18: note: expanded from macro 'WRAP'

define WRAP(x) __interceptor_ ## x

^

<scratch space>:60:1: note: expanded from here
__interceptor_malloc
^

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8859402008562911568/+/steps/gclient_runhooks/0/stdout?format=raw

I was misled by a disabled mallinfo guard in tsan for Android. Bionic seems to provide mallinfo: D93970
(I don't know how to test, though)

aeubanks added inline comments.Jan 2 2021, 1:32 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
24 ↗(On Diff #314174)

compiler-rt/lib/hwasan/hwasan_setjmp.S includes sanitizer_platform.h. With this new include, on Android it ends up including C function declarations into a .S file which doesn't compile. https://crbug.com/1162741

thakis added a subscriber: thakis.Jan 2 2021, 4:01 PM

Reverted in fe9976c02c09f105751f787ec998abeb3414a235 for now. If you could wait with relanding during working hours, that'd be super appreciated. Thanks!

uabelho added a subscriber: uabelho.Jan 4 2021, 1:46 AM
MaskRay marked an inline comment as done.Jan 5 2021, 3:58 PM
MaskRay added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
24 ↗(On Diff #314174)

Thanks for the information. This looks like an Android header file bug. Including features.h should not pull in additional random symbols which cause a compile error.

This is causing a build failure on the 32-bit ARM bots:

/usr/local/bin/c++  -DHAVE_RPC_XDR_H=1 -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/sanitizer_common -I/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common -Iinclude -I/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/llvm/include -I/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/.. -mcpu=cortex-a57 -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -std=c++14 -Wno-unused-parameter -O3     -march=armv7-a -mfloat-abi=hard -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -Wframe-larger-than=570 -Wglobal-constructors -UNDEBUG -std=c++14 -MD -MT projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.armhf.dir/sanitizer_platform_limits_posix.cpp.o -MF projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.armhf.dir/sanitizer_platform_limits_posix.cpp.o.d -o projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.armhf.dir/sanitizer_platform_limits_posix.cpp.o -c /home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:1046:1: error: static_assert failed due to requirement 'sizeof (((__sanitizer::__sanitizer_dirent *)__null)->d_ino) == sizeof (((dirent *)__null)->d_ino)' ""
CHECK_SIZE_AND_OFFSET(dirent, d_ino);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h:1438:3: note: expanded from macro 'CHECK_SIZE_AND_OFFSET'
  COMPILER_CHECK(sizeof(((__sanitizer_##CLASS *)NULL)->MEMBER) == \
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:332:30: note: expanded from macro 'COMPILER_CHECK'
#define COMPILER_CHECK(pred) static_assert(pred, "")
                             ^             ~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:1052:1: error: static_assert failed due to requirement 'sizeof (((__sanitizer::__sanitizer_dirent *)__null)->d_off) == sizeof (((dirent *)__null)->d_off)' ""
CHECK_SIZE_AND_OFFSET(dirent, d_off);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h:1438:3: note: expanded from macro 'CHECK_SIZE_AND_OFFSET'
  COMPILER_CHECK(sizeof(((__sanitizer_##CLASS *)NULL)->MEMBER) == \
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:332:30: note: expanded from macro 'COMPILER_CHECK'
#define COMPILER_CHECK(pred) static_assert(pred, "")
                             ^             ~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:1052:1: error: static_assert failed due to requirement '__builtin_offsetof(__sanitizer::__sanitizer_dirent, d_off) == __builtin_offsetof(dirent, d_off)' ""
CHECK_SIZE_AND_OFFSET(dirent, d_off);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h:1440:3: note: expanded from macro 'CHECK_SIZE_AND_OFFSET'
  COMPILER_CHECK(offsetof(__sanitizer_##CLASS, MEMBER) ==         \
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:332:30: note: expanded from macro 'COMPILER_CHECK'
#define COMPILER_CHECK(pred) static_assert(pred, "")
                             ^             ~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:1054:1: error: static_assert failed due to requirement '__builtin_offsetof(__sanitizer::__sanitizer_dirent, d_reclen) == __builtin_offsetof(dirent, d_reclen)' ""
CHECK_SIZE_AND_OFFSET(dirent, d_reclen);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h:1440:3: note: expanded from macro 'CHECK_SIZE_AND_OFFSET'
  COMPILER_CHECK(offsetof(__sanitizer_##CLASS, MEMBER) ==         \
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-armv8-lld/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:332:30: note: expanded from macro 'COMPILER_CHECK'
#define COMPILER_CHECK(pred) static_assert(pred, "")
                             ^             ~~~~
4 errors generated.

Full log at http://lab.llvm.org:8011/#/builders/126/builds/1192. This is affecting quite a few bots (all of the ARM bots which build compiler-rt), so I've reverted it as 4839378ca05f88faed53ea25457fd93fcad93460 for now.

MaskRay reopened this revision.Jan 6 2021, 10:42 AM
This revision is now accepted and ready to land.Jan 6 2021, 10:42 AM
MaskRay updated this revision to Diff 314939.Jan 6 2021, 10:42 AM
MaskRay edited the summary of this revision. (Show Details)

Move _FILE_OFFSET_BITS above to appease 32-bit ARM features.h
Add a release note

Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2021, 10:42 AM
This revision was landed with ongoing or failed builds.Jan 6 2021, 10:55 AM
This revision was automatically updated to reflect the committed changes.