Page MenuHomePhabricator

[TSan][libdispatch] Ensure TSan dylib works on old systems
ClosedPublic

Authored by yln on Aug 17 2020, 12:52 PM.

Details

Summary

dispatch_async_and_wait() was introduced in macOS 10.14, which is
greater than our minimal deployment target. We need to forward declare
it as a "weak import" to ensure we generate a weak reference so the TSan
dylib continues to work on older systems. We cannot simply `#include
<dispatch.h>` or use the Darwin availability macros since this file is
multi-platform.

Before:

➤ dyldinfo -bind ./lib/clang/12.0.0/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib | grep dispatch_async_and_wait
__DATA  __interpose      0x000F5E68    pointer      0 libSystem        _dispatch_async_and_wait_f

After:

➤ dyldinfo -bind ./lib/clang/12.0.0/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib | grep dispatch_async_and_wait
__DATA  __got            0x000EC0A8    pointer      0 libSystem        _dispatch_async_and_wait (weak import)
__DATA  __interpose      0x000F5E78    pointer      0 libSystem        _dispatch_async_and_wait (weak import)

This is a follow-up to D85854 and should fix:
https://reviews.llvm.org/D85854#2221529

Diff Detail

Event Timeline

yln created this revision.Aug 17 2020, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 12:52 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
yln requested review of this revision.Aug 17 2020, 12:52 PM
yln added a comment.Aug 17 2020, 12:54 PM

@dmajor: do you have a quick way to check if this unbreaks your builds?
Another follow-up might be required to make the test work: https://reviews.llvm.org/D85995

In D86103#2222302, @yln wrote:

@dmajor: do you have a quick way to check if this unbreaks your builds?
Another follow-up might be required to make the test work: https://reviews.llvm.org/D85995

Not immediate, it will take me a few hours to check the builds, and we don't run the tests on this platform.

delcypher added inline comments.Aug 17 2020, 2:18 PM
compiler-rt/lib/tsan/rtl/tsan_dispatch_defs.h
60

Is this not defined elsewhere in sanitizer_common?

yln added a comment.Aug 17 2020, 2:28 PM

I just discovered that this is still not enough; linking still fails with SDK < 10.14. Only options left are:

  • Raise required SDK for building (deployment target remains unchanged)
  • #ifdef out these interceptors when building with older SDKs
compiler-rt/lib/tsan/rtl/tsan_dispatch_defs.h
60
In D86103#2222585, @yln wrote:

I just discovered that this is still not enough; linking still fails with SDK < 10.14.

Yeah, my bots just reported back with the same results. I also tried changing __attribute(( to __attribute__(( just in case, but no change.

Only options left are:

  • Raise required SDK for building (deployment target remains unchanged)
  • #ifdef out these interceptors when building with older SDKs

Is there harm in going the ifdef route? We only just recently managed to adapt to the 10.12 requirement from D74501 and it'd be nice to have a bit of a break before going through that again. :-)

yln added a comment.Aug 17 2020, 2:49 PM

Clarification: the problem is *building* with SDK < 10.14. *Running* on macOS < 10.14 works (with this patch).

I will try to go the #ifdef route. The downside is that the resulting runtime, provides (or doesn't provide) certain interceptors depending on the SDK that was used to build the runtime.

yln updated this revision to Diff 286170.Aug 17 2020, 4:07 PM

Guard compilation of interceptors with #ifdef, in addition to forcing weak references.
This means we are not building those when compiling with older SDKs.

I verified that both building with SDK < 10.14 and running on macOS < 10.14 works.

If we use #if !SANITIZER_MAC || defined(__MAC_10_14), do we still need the forward declarations and weak_import stuff?

yln added a comment.Aug 18 2020, 8:16 AM

If we use #if !SANITIZER_MAC || defined(__MAC_10_14), do we still need the forward declarations and weak_import stuff?

Unfortunately, yes.

The #if !SANITIZER_MAC || defined(__MAC_10_14) skips building the interceptors when older SDKs is used to build to avoid a linker error.
The SANITIZER_WEAK_IMPORT declarations make sure we emit a weak reference so we can run on older systems. (We can't include the libdispatch header to accomplish this, because this is a multi-platform file.)

kubamracek accepted this revision.Aug 18 2020, 5:59 PM
This revision is now accepted and ready to land.Aug 18 2020, 5:59 PM
This revision was landed with ongoing or failed builds.Aug 18 2020, 6:35 PM
This revision was automatically updated to reflect the committed changes.