Depends on D9637
Details
Diff Detail
Event Timeline
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.
lib/sanitizer_common/sanitizer_mac.cc | ||
---|---|---|
369 | Why is this disabled on certain iOS versions? |
lib/sanitizer_common/sanitizer_mac.cc | ||
---|---|---|
369 | 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 |
lib/sanitizer_common/sanitizer_mac.cc | ||
---|---|---|
370 | 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. |
lib/sanitizer_common/sanitizer_mac.cc | ||
---|---|---|
371 | Please use SANITIZER_MAC, SANITIZER_IOSSIM and SANITIZER_IOS instead. |
lib/sanitizer_common/sanitizer_mac.cc | ||
---|---|---|
371 | @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. |
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 | I am not convinced we need to move this to a common place. You may drop this TODO. |
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.
I am not convinced we need to move this to a common place. You may drop this TODO.