This is an archive of the discontinued LLVM Phabricator instance.

Support fiber API on macOS
ClosedPublic

Authored by yuri on Feb 12 2019, 2:32 AM.

Diff Detail

Event Timeline

yuri created this revision.Feb 12 2019, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2019, 2:32 AM
Herald added subscribers: Restricted Project, llvm-commits, jfb and 2 others. · View Herald Transcript

Kuba, please take a look. This mostly touches Mac-specific files. I can't test this.

@yln Could you take a look? Do the tests actually pass on Darwin?

The _XOPEN_SOURCE stuff is ugly, but I guess it's necessary to use swapcontext on Darwin. For the pragma diagnostic push/ignore stuff, we might be better off just redeclaring the function ourselves without including ucontext.h. TBH, I'm not sure if there's a lot of value in supporting swapcontext on Darwin, as they are very likely broken when using many system libraries.

test/tsan/fiber_asm.cc
21–22 ↗(On Diff #186428)

Can we use something like the ASM_SYMBOL macro from sanitizer_asm.h and put it into test.h please?

yln added a comment.Feb 13 2019, 2:50 PM

ninja check-tsan reports no failing test on my machine.

yuri updated this revision to Diff 186852.Feb 14 2019, 8:50 AM

Cleanup tests

yuri marked an inline comment as done.Feb 14 2019, 8:51 AM
yuri added a comment.Feb 14 2019, 9:20 AM

For the pragma diagnostic push/ignore stuff, we might be better off just redeclaring the function ourselves without including ucontext.h.

For tsan_interceptors_mac.cc it can be done. For me such solution is more ugly than usage of #define and #pragma; but if you insist on it, I can do such change.

For tests it is bad idea because test should use API the same way as real-world clients. I moved declarations to the separate header. Now code looks better.

TBH, I'm not sure if there's a lot of value in supporting swapcontext on Darwin, as they are very likely broken when using many system libraries.

  • QEMU is an important projected user of fiber API and it actually use swapcontext.
  • We need to test fiber API somehow and swapcontext is the only portable way to do it.

PS. I want to explain why swapcontext interceptor is required on macOS and not on Linux.
__tsan_switch_to_fiber is called immediately before or after context switch.
Actual context switch code should not introduce memory dependancies; otherwise false positive races can be reported.
On Linux swapcontext implementation does not call any intercepted function, so condition is fulfilled.
On macOS we have to intercept swapcontext and disable interceptors inside it in order to achieve the same effect.

For the pragma diagnostic push/ignore stuff, we might be better off just redeclaring the function ourselves without including ucontext.h.

For tsan_interceptors_mac.cc it can be done. For me such solution is more ugly than usage of #define and #pragma; but if you insist on it, I can do such change.

I also think it's more ugly, but I'm afraid the code won't build against watchOS and tvOS, where swapcontext is not just deprecated, but it's completely unavailable. @yln Can you please try to build this for watchOS/tvOS?

yuri added a comment.EditedFeb 14 2019, 10:50 PM

@yln Can you please try to build this for watchOS/tvOS?

watchOS/tvOS build is broken right now in trunk:
https://pastebin.com/Y3gsxtA7

yuri updated this revision to Diff 186973.Feb 15 2019, 1:16 AM

Added

#if !SANITIZER_WATCHOS && !SANITIZER_TVOS

around swapcontext interceptor

yln added a comment.EditedFeb 18 2019, 10:54 AM

I verified that the watchOS/tvOS runtimes (libclang_rt.tsan_*) are building with this patch applied to our internal master.

watchOS/tvOS build is broken right now in trunk:
https://pastebin.com/Y3gsxtA7

The watchOS/tvOS parts of ASan require an internal SDK that's not publicly available to build. I guess that counts as "broken", but it's an expected state.

I think the tests are still going to fail on watchOS and tvOS, they need to be marked as UNSUPPORTED. At this point, I'm inclined to prefer to declare swapcontext ourselves (as swapcontext *is* available at runtime on watchOS/tvOS, it's just not allowed in the SDK) to avoid this much ifdef'ing and platform-specific workarounds. But if you strongly disagree, I'm okay with this approach also. (The tests still need updating.)

yuri updated this revision to Diff 187310.Feb 18 2019, 10:54 PM
yuri set the repository for this revision to rG LLVM Github Monorepo.

Do not include <ucontext.h> and declare getcontext/setcontext manually

yuri added a comment.Feb 18 2019, 10:57 PM

I think the tests are still going to fail on watchOS and tvOS, they need to be marked as UNSUPPORTED

I have not found any tests marked such way. What keywords to use for UNSUPPORTED?

yln added a comment.Feb 19 2019, 2:12 PM

I think the tests are still going to fail on watchOS and tvOS, they need to be marked as UNSUPPORTED

I have not found any tests marked such way. What keywords to use for UNSUPPORTED?

I don't know exactly what/how should be done (Kuba, please advise), but the definition of the features is here:
https://github.com/llvm/llvm-project/blob/master/compiler-rt/test/lit.common.cfg#L130

yuri updated this revision to Diff 187521.Feb 19 2019, 11:10 PM

Disable tests on tvOS and watchOS

kubamracek accepted this revision.Feb 20 2019, 7:12 AM
This revision is now accepted and ready to land.Feb 20 2019, 7:12 AM
yuri added a comment.Feb 20 2019, 11:23 PM

If everything is OK, can you commit this patch?

yuri added a comment.Feb 25 2019, 12:03 AM

If everything is OK, can you commit this patch?

Kuba?
Dmitry?

Kuba, please submit this. I can't test/build this.

yuri added a comment.Mar 25 2019, 1:44 AM

Kuba, please submit this. I can't test/build this.

Kuba, please do not ignore

Sorry for the huge delay. @yln can you test this patch (that is passes all tests) and if yes, land it?

This revision was automatically updated to reflect the committed changes.
yln added a comment.Jul 5 2019, 4:28 PM

Had to XFAIL a few tests for iOS (macOS works), see attached log.