Page MenuHomePhabricator

Reimplement Thread Static Data ASan routines with TLS
ClosedPublic

Authored by krytarowski on Dec 12 2018, 3:39 AM.

Details

Summary

Thread Static Data cannot be used in early init on NetBSD
and FreeBSD. Reuse the ASan TSD API for compatibility with
existing code with an alternative implementation using Thread
Local Storage.

New version uses Thread Local Storage to store a pointer
with thread specific data. The destructor from TSD has been
replaced with a TLS destrucutor that is called upon thread
exit.

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Dec 12 2018, 3:39 AM

@vitalybuka this code can became the only one as it shall be portable to others and it might be needed for them too.

I recall that FreeBSD had problems too.

I recall that FreeBSD had problems too.

True.

  • add missing chunk of patch

I will follow up with analogous code in MSan and HWASan.

krytarowski planned changes to this revision.Dec 13 2018, 2:10 AM

I made a mistake, the destructor must be called for all threads, not just the main one.

krytarowski edited the summary of this revision. (Show Details)
  • new version
  • replace __weak with a compiler-rt style attribute
krytarowski edited the summary of this revision. (Show Details)Dec 15 2018, 12:45 AM
krytarowski edited the summary of this revision. (Show Details)

I would like to make this version as the mainline for all POSIX OSes.

@devnexen does it work on FreeBSD?

@devnexen does it work on FreeBSD?

i get more warning messages with the patch than without

__cxa_thread_call_dtors: dtr <address> from unloaded so, skipping

@devnexen does it work on FreeBSD?

i get more warning messages with the patch than without

__cxa_thread_call_dtors: dtr <address> from unloaded so, skipping

I can try to rework it into a static TLS object with a destructor that will call the cleanup thread routine... but I will wait for feedback from @vitalybuka.

@devnexen does it work on FreeBSD?

i get more warning messages with the patch than without

__cxa_thread_call_dtors: dtr <address> from unloaded so, skipping

How about this version: http://netbsd.org/~kamil/llvm/asan-posix-tls.txt
Does it work on FreeBSD? If so, how about Linux and Darwin.

How about this version: http://netbsd.org/~kamil/llvm/asan-posix-tls.txt
Does it work on FreeBSD? If so, how about Linux and Darwin.

Seems to work fine with this version.

How about this version: http://netbsd.org/~kamil/llvm/asan-posix-tls.txt
Does it work on FreeBSD? If so, how about Linux and Darwin.

Seems to work fine with this version.

On what OSes?

I'm going to set is as the only version but I need confirmation from Darwin and Linux.

Sorry I meant FreeBSD here.

krytarowski added a subscriber: ro.Dec 18 2018, 5:20 AM

@ro could you please test this patch for Solaris? I need to know whether it works with TLS destructor on SunOS.

krytarowski retitled this revision from Reimplement Thread Static Data ASan routines for NetBSD to Reimplement Thread Static Data ASan routines with TLS.
krytarowski edited the summary of this revision. (Show Details)
  • add new implementation
  • include FreeBSD

Sorry I meant FreeBSD here.

Please test Linux and Darwin too.

devnexen added a comment.EditedDec 18 2018, 8:03 AM

Sorry I meant FreeBSD here.

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

So for Linux, seems it is a more an issue get bunch of undefined reference as

undefined reference to `__cxa_thread_atexit'

Sorry I meant FreeBSD here.

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

So for Linux, seems it is a more an issue get bunch of undefined reference as

undefined reference to `__cxa_thread_atexit'

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

devnexen added a comment.EditedDec 18 2018, 2:49 PM

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

Sorry I redid a full build and finally there is an issue
thread-local storage is not supported for the current target changing to THREADLOCAL won t fix it.

vitalybuka added inline comments.Dec 19 2018, 12:54 AM
lib/asan/asan_posix.cc
49 ↗(On Diff #178646)

tsd_key_inited is redundant here?

devnexen added a subscriber: dim.Dec 19 2018, 12:56 AM

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

Might be good as is indeed. Maybe you want to hear from @dim first though ?

krytarowski marked an inline comment as done.Dec 19 2018, 1:10 AM

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

Sorry I redid a full build and finally there is an issue
thread-local storage is not supported for the current target changing to THREADLOCAL won t fix it.

THREADLOCAL evaluates to __thread and it does not support TLS destructors.

It must use C++11 thread_local.

thread-local storage is not supported for the current target

C++11 thread_local not supported for FreeBSD?

lib/asan/asan_posix.cc
49 ↗(On Diff #178646)

It serves just as an additional assert, I can drop it.

thread-local storage is not supported for the current target

C++11 thread_local not supported for FreeBSD?

Nope that was Darwin's test.

thread-local storage is not supported for the current target

C++11 thread_local not supported for FreeBSD?

Nope that was Darwin's test.

OK, so let's restrict this patch to FreeBSD and NetBSD only.

krytarowski marked an inline comment as done.Dec 19 2018, 2:39 AM
vitalybuka accepted this revision.Dec 19 2018, 2:49 AM

should this cause any changes in tests? maybe something is going to pass now?

lib/asan/asan_posix.cc
49 ↗(On Diff #178646)

I'd prefer if you drop, for simplicity.

This revision is now accepted and ready to land.Dec 19 2018, 2:49 AM

should this cause any changes in tests? maybe something is going to pass now?

With this patch, ASan is functional at all, no longer with local modifications.

I'm going to update equivalent code for HWASan and MSan soon.

This revision was automatically updated to reflect the committed changes.

should this cause any changes in tests? maybe something is going to pass now?

With this patch, ASan is functional at all, no longer with local modifications.

I'm going to update equivalent code for HWASan and MSan soon.

Actually, I've noted that HWASan has been refactored.. and it would help to test it. How to get hardware with HWASan? Is there a simulator?

HWASAN tests should run on any ARMv8 Android device.
Generally, a patched kernel is required to run HWASAN code, but tests are written to avoid passing tagged pointers to system calls, and they don't have this requirement.

ro added a comment.Jan 2 2019, 5:48 AM

@ro could you please test this patch for Solaris? I need to know whether it works with TLS destructor on SunOS.

I ran into two compilation failures on unmodified trunk first and got sidetracked (also by the Christmas holidays). However, after
fixing those, applying your patch locally and enabling it on PLATFORM_SOLARIS, it seemed to work fine.

In D55596#1343564, @ro wrote:

@ro could you please test this patch for Solaris? I need to know whether it works with TLS destructor on SunOS.

I ran into two compilation failures on unmodified trunk first and got sidetracked (also by the Christmas holidays). However, after
fixing those, applying your patch locally and enabling it on PLATFORM_SOLARIS, it seemed to work fine.

As we cannot switch Linux and get a single implementation for everybody, if you need this patch feel free to switch Solaris to it.

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 7:58 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
dim added a comment.Jul 24 2019, 9:49 AM

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

In D55596#1599456, @dim wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

I just spent some time debugging and it seems like https://reviews.llvm.org/D55596 works.

dim added a comment.Jul 24 2019, 10:50 AM
In D55596#1599456, @dim wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

I just spent some time debugging and it seems like https://reviews.llvm.org/D55596 works.

Ehm, that is exactly *this* review? Maybe you meant to paste another? :) (Btw, you can just post the abbreviation D55596 and Phabricator will pick it up, like so: D55596.)

In D55596#1599656, @dim wrote:
In D55596#1599456, @dim wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

I just spent some time debugging and it seems like https://reviews.llvm.org/D55596 works.

Ehm, that is exactly *this* review? Maybe you meant to paste another? :) (Btw, you can just post the abbreviation D55596 and Phabricator will pick it up, like so: D55596.)

Yes I actually mean D65221