[hwasan] Fix HWASAN_WITH_INTERCEPTORS=OFF build on not-android.

[eugenis]

Summary

I'm not aware of any platforms where this will work, but the code should at least compile.
HWASAN_WITH_INTERCEPTORS=OFF means there is magic in libc that would call hwasan_thread_enter /
hwasan_thread_exit as appropriate.

[winksaville]

compiler-rt/lib/hwasan/hwasan_linux.cpp

250 ↗

(On Diff #197427)

This seems weird, what happens if HAWSAN_WITH_INTERCEPTORS is true and SANITIZER_ANDROID is true. We would get this code but the GetCurrentThreadLongPtr() would be the version that calls get_android_tls_ptr() is that valid?

310 ↗

(On Diff #197427)

Since __hwasan_thread_enter() is always defined why is this code conditional? The other possibility is that the condition should be !SANITIZER_ANDROID so that it matches the value returned by GetCurrentThreadLongPtr().

[eugenis]

HWASAN_WITH_INTERCEPTORS=OFF is a mode where we rely on libc to call hwasan_thread_enter and hwasan_thread_exit as appropriate. This is particularly important when libc is built with hwasan, because then instrumented code may run on a thread before GetCurrentThread is called. At this moment, only bionic supports this, as far as I know.

These two conditional blocks are independent. The one with HwasanTSDThreadInit implements calling __hwasan_thread_exit in the HWASAN_WITH_INTERCEPTORS=OFF mode. The one with GetCurrentThreadLongPtr defines access to the thread local storage (which is also part of the instrumentation ABI). Android uses the fast fixed slot in either mode.

[winksaville]

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF
HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=ON
HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=OFF
HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=ON

[eugenis]

In D61337#1486551, @winksaville wrote:

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF

This configuration does not exist.

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=ON

This needs custom-built android to test. We've got some integration tests downstream.
Now that I think of it, the upstream test harness could provide something to emulate that on stock Android, but that would still be quite far from the real thing.

HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=OFF

This is check-hwasan on x86_64-linux.

HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=ON

This is check-hwasan on aarch64-linux-android.

[winksaville]

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF

This configuration does not exist.

Should this be an error and cause an "unsupported configuration"?

[eugenis]

In D61337#1487120, @winksaville wrote:

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF

This configuration does not exist.

Should this be an error and cause an "unsupported configuration"?

But we don't really know that, because a change that makes this configuration supported needs to be in libc or in platform build files, not in compiler-rt. I'd rather not block anyone from experimenting.

[winksaville]

In D61337#1488735, @eugenis wrote:

In D61337#1487120, @winksaville wrote:

@eugenis, which of the following conditions have tests?

HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF

This configuration does not exist.

Should this be an error and cause an "unsupported configuration"?

But we don't really know that, because a change that makes this configuration supported needs to be in libc or in platform build files, not in compiler-rt. I'd rather not block anyone from experimenting.

What about makeing it a warning, "untested configuration"? Obviously your call, just seems prudent to me.

[eugenis]

Should this be an error and cause an "unsupported configuration"?

But we don't really know that, because a change that makes this configuration supported needs to be in libc or in platform build files, not in compiler-rt. I'd rather not block anyone from experimenting.

What about makeing it a warning, "untested configuration"? Obviously your call, just seems prudent to me.

Sorry, but I don't like it for the same reason.

[winksaville]

In D61337#1488898, @eugenis wrote:

Should this be an error and cause an "unsupported configuration"?

But we don't really know that, because a change that makes this configuration supported needs to be in libc or in platform build files, not in compiler-rt. I'd rather not block anyone from experimenting.

What about makeing it a warning, "untested configuration"? Obviously your call, just seems prudent to me.

Sorry, but I don't like it for the same reason.

How about a comment something like:

// Confiturations of HWASAN_WITH_INTERCEPTORS and SANITIZER_ANDROID
//
// HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF
//
//   Not currently tested
//
// HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=ON
//
//   Integration tests downstream exit
//
// HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=OFF
//
//    Tested with check-hwasan on x86_64-linux.
//
// HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=ON
//
//    Tested with check-hwasan on aarch64-linux-android.

[eugenis]

add a comment

[winksaville]

LGTM

[winksaville]

Adding action "Accept Revision", I'm not sure this is the processes so correct me if this is inapprporpriate and let me know what I should do.

[eugenis]

That's fine, thank you.
I need to submit the change manually anyway.

compiler-rt/trunk/lib/hwasan/hwasan_linux.cpp

 #include <sys/resource.h>
 #include <sys/time.h>
 #include <unistd.h>
 #include <unwind.h>
 
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_procmaps.h"
 
-#if HWASAN_WITH_INTERCEPTORS && !SANITIZER_ANDROID
+// Configurations of HWASAN_WITH_INTERCEPTORS and SANITIZER_ANDROID.
+//
+// HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=OFF
+//   Not currently tested.
+// HWASAN_WITH_INTERCEPTORS=OFF, SANITIZER_ANDROID=ON
+//   Integration tests downstream exist.
+// HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=OFF
+//    Tested with check-hwasan on x86_64-linux.
+// HWASAN_WITH_INTERCEPTORS=ON, SANITIZER_ANDROID=ON
+//    Tested with check-hwasan on aarch64-linux-android.
+#if !SANITIZER_ANDROID
 SANITIZER_INTERFACE_ATTRIBUTE
 THREADLOCAL uptr __hwasan_tls;
 #endif
 
 namespace __hwasan {
 
 static void ReserveShadowMemoryRange(uptr beg, uptr end, const char *name) {
   CHECK_EQ((beg % GetMmapGranularity()), 0);