This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by eugenis on Apr 30 2019, 1:28 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.Apr 30 2019, 1:28 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 30 2019, 1:28 PM
Herald added subscribers: Restricted Project, kubamracek, srhines. · View Herald Transcript
winksaville added inline comments.Apr 30 2019, 6:54 PM
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().

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.

@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, 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.

@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, 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.

@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.

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.

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 updated this revision to Diff 197907.May 2 2019, 6:23 PM

add a comment

winksaville accepted this revision.May 3 2019, 9:35 AM

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.

This revision is now accepted and ready to land.May 3 2019, 9:35 AM

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

This revision was automatically updated to reflect the committed changes.