Implement `internal_start/join_thread` on Mac OS X

[ismailp]

Summary

Depends on D9637

[glider]

Hi Ismail,

I'll take a look on Tuesday, sorry for the delay.

[ismailp]

Hi Alexander,

Thank you for your time. No real rush; I am still splitting my patches.

I've got TSan "mostly" working on Mac OS X; 92% of tests pass (and a few of them are Linux-only). I am sure there are some dirty hacks among my patches that cannot go upstream. I'll submit them for review anyway, so that we can discuss, and find better fixes.

[glider]

lib/sanitizer_common/sanitizer_mac.cc

340 ↗

(On Diff #25412)

Why is this disabled on certain iOS versions?
Do we really need TSan on iOS? (note that the shadow memory overhead is quite big and the 32-bit address space may be too little).

[ismailp]

lib/sanitizer_common/sanitizer_mac.cc

340 ↗

(On Diff #25412)

I didn't try porting TSan to iOS, but I thought about trying it out for iPhone 6, which has 64-bit processor. I have no idea whether this is possible, as I didn't investigate it in detail.

We must get it working reliably on Mac OS X first. Therefore, it might be better to replace this line with:

#if TARGET_OS_MAC && !TARGET_OS_IPHONE && !TARGET_IPHONE_SIMULATOR

[glider]

lib/sanitizer_common/sanitizer_mac.cc

340 ↗

(On Diff #25412)

Agreed.

341 ↗

(On Diff #25412)

Looks like you've taken this from sanitizer_linux.cc
Can you please move this function to sanitizer_posix.cc instead?

346 ↗

(On Diff #25412)

I think we need a __sanitizer_pthread_t declaration somewhere. No need to add it right now though.

[ismailp]

lib/sanitizer_common/sanitizer_mac.cc

341 ↗

(On Diff #25412)

I've tried moving this in sanitizer_posix.cc, but it made the code really ugly. It needed #includes -- 1 for Linux, and one for Mac. We must move these #ifs (Mac's target conditionals) and SANITIZER_LINUX -- because one line of code in sanitizer_linux.cc needs to be surrounded with it -- in sanitizer_posix.cc. It also needs declaration of internal_sigfillset, which is missing from sanitizer_posix.cc. If you really want to see it, I can certainly submit it. But IMO, that must be a separate commit, only containing refactored and readable code.

[ismailp]

Allow only Mac OS X implementation of internal_start_thread and internal_join_thread

[samsonov]

lib/sanitizer_common/sanitizer_mac.cc

359 ↗

(On Diff #28123)

Please use SANITIZER_MAC, SANITIZER_IOSSIM and SANITIZER_IOS instead.

[beanz]

lib/sanitizer_common/sanitizer_mac.cc

359 ↗

(On Diff #28123)

You may not need these ifdefs at all. Does this work on the Simulator? I can't think of any reason why it wouldn't.

I've proposed adding experimental support for building iOS using CMake in D10710, and I expect this should all work on iOS too.

[ismailp]

lib/sanitizer_common/sanitizer_mac.cc

359 ↗

(On Diff #28123)

@samsonov Will do, if suggestion of @beanz doesn't work.

@beanz This is part of my TSan porting effort, and it seems like this function is called from TSan only. I haven't tried it on Simulator yet. I will try this patch, possibly without any #ifs on Simulator, if Simulator is 64-bit. TSan requires 64-bit OS.

[ismailp]

• Removed preprocessor directives guarding pthread.

[ismailp]

Ping!

[kcc]

I am ok with the change.
AFIACT, it does not add any new functionality as is but is required for other changes, right?
I wonder, is it possible to add a test for this?

lib/sanitizer_common/sanitizer_mac.cc

368 ↗

(On Diff #30596)

I am not convinced we need to move this to a common place. You may drop this TODO.

[samsonov]

LGTM

[glider]

LGTM

[ismailp]

Thanks to all for their time and reviewing the patch.

@kcc I've removed FIXME, but not sure how to unit test this (for all platforms). Do we expose this functionality through an API?

[kcc]

In D9638#286779, @ismailp wrote:

Thanks to all for their time and reviewing the patch.

@kcc I've removed FIXME, but not sure how to unit test this (for all platforms). Do we expose this functionality through an API?

No good suggestion here.
This is an internal functionality and is not exposed as an API.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc

kern_return_t result =
       task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&info, &count);
   if (UNLIKELY(result != KERN_SUCCESS)) {
     Report("Cannot get task info. Error: %d\n", result);
     Die();
   }
   return info.resident_size;
 }
 
-void *internal_start_thread(void (*func)(void *arg), void *arg) { return 0; }
-void internal_join_thread(void *th) { }
+void *internal_start_thread(void(*func)(void *arg), void *arg) {
+  // Start the thread with signals blocked, otherwise it can steal user signals.
+  __sanitizer_sigset_t set, old;
+  internal_sigfillset(&set);
+  internal_sigprocmask(SIG_SETMASK, &set, &old);
+  pthread_t th;
+  pthread_create(&th, 0, (void*(*)(void *arg))func, arg);
+  internal_sigprocmask(SIG_SETMASK, &old, 0);
+  return th;
+}
+
+void internal_join_thread(void *th) { pthread_join((pthread_t)th, 0); }
 
 void GetPcSpBp(void *context, uptr *pc, uptr *sp, uptr *bp) {
   ucontext_t *ucontext = (ucontext_t*)context;
 # if defined(__aarch64__)
   *pc = ucontext->uc_mcontext->__ss.__pc;
 #   if defined(__IPHONE_8_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_8_0
   *bp = ucontext->uc_mcontext->__ss.__fp;
 #   else