Page MenuHomePhabricator

Implement `internal_start/join_thread` on Mac OS X
ClosedPublic

Authored by ismailp on May 9 2015, 9:31 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ismailp updated this revision to Diff 25412.May 9 2015, 9:31 AM
ismailp retitled this revision from to Implement `internal_start/join_thread` on Mac OS X.
ismailp updated this object.
ismailp edited the test plan for this revision. (Show Details)
ismailp added reviewers: kcc, glider, samsonov.
ismailp added a subscriber: Unknown Object (MLST).
glider edited edge metadata.May 9 2015, 12:30 PM

Hi Ismail,

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

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 added inline comments.May 12 2015, 2:40 AM
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 added inline comments.May 13 2015, 12:06 PM
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 added inline comments.May 19 2015, 3:21 AM
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 added inline comments.Jun 22 2015, 9:52 AM
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 updated this revision to Diff 28123.Jun 22 2015, 9:57 AM
ismailp edited edge metadata.

Allow only Mac OS X implementation of internal_start_thread and internal_join_thread

samsonov added inline comments.Jun 23 2015, 2:38 PM
lib/sanitizer_common/sanitizer_mac.cc
359 ↗(On Diff #28123)

Please use SANITIZER_MAC, SANITIZER_IOSSIM and SANITIZER_IOS instead.

beanz added inline comments.Jun 24 2015, 2:50 PM
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 added inline comments.Jun 26 2015, 10:36 AM
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 updated this revision to Diff 30596.Jul 24 2015, 12:57 PM
  • Removed preprocessor directives guarding pthread.
kcc edited edge metadata.Sep 18 2015, 3:04 PM

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 accepted this revision.Sep 18 2015, 4:01 PM
samsonov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 18 2015, 4:01 PM
glider accepted this revision.Sep 22 2015, 3:28 AM
glider edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.

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 added a comment.Nov 11 2015, 11:32 AM

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.