Page MenuHomePhabricator

Support fiber API on macOS
Needs ReviewPublic

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

Details

Diff Detail

Event Timeline

yuri created this revision.Tue, Feb 12, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Feb 12, 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.Wed, Feb 13, 2:50 PM

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

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

Cleanup tests

yuri marked an inline comment as done.Thu, Feb 14, 8:51 AM
yuri added a comment.Thu, Feb 14, 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.EditedThu, Feb 14, 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.Fri, Feb 15, 1:16 AM

Added

#if !SANITIZER_WATCHOS && !SANITIZER_TVOS

around swapcontext interceptor