This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Use large address space mapping on Apple Silicon Macs
ClosedPublic

Authored by kubamracek on Aug 21 2020, 3:07 PM.

Diff Detail

Event Timeline

kubamracek created this revision.Aug 21 2020, 3:07 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptAug 21 2020, 3:07 PM
kubamracek requested review of this revision.Aug 21 2020, 3:07 PM
yln added a comment.Aug 21 2020, 4:12 PM

I feel these #ifdefs becoming quite unwieldy. One of the reasons that I have run into a couple of times myself is that we don't have a specific one for "macOS only".

From TargetConditionals.h:

+----------------------------------------------------------------+
|                TARGET_OS_MAC                                   |
| +---+  +-----------------------------------------------------+ |
| |   |  |          TARGET_OS_IPHONE                           | |
| |OSX|  | +-----+ +----+ +-------+ +--------+ +-------------+ | |
| |   |  | | IOS | | TV | | WATCH | | BRIDGE | | MACCATALYST | | |
| |   |  | +-----+ +----+ +-------+ +--------+ +-------------+ | |
| +---+  +-----------------------------------------------------+ |
+----------------------------------------------------------------+

We are missing the OSX one and and SANITIZER_MAC is probably a misnomer (maybe SANITIZER_DARWIN would be better)?

LGTM, not blocking this change on improving the macros, but I think it would be worth clearing this up.

compiler-rt/lib/tsan/rtl/tsan_platform.h
29

Just checking I am reading this correctly:

arch=x86_64
-or-
macOS (all archs)

last part is expressed as "not (iOS (and derivatives) and not simulators)"

compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
103

"iOS (and derivatives) on arm64 (but not the simulators)" ?
because "arm64 && apple" doesn't imply iOS anymore.

Is the SANITIZER_MAC unnecessary then?

yln accepted this revision.Aug 21 2020, 4:13 PM
This revision is now accepted and ready to land.Aug 21 2020, 4:13 PM

Definitely agree that this is very annoying to read. I think it would be best if we were able to change the ifdef to use something like “HAS_48_BIT_ADDRESS_SPACE” and compute that bool separately. @yln, @dvyukov WDYT?

yln added a comment.Aug 24 2020, 10:12 AM

Definitely agree that this is very annoying to read. I think it would be best if we were able to change the ifdef to use something like “HAS_48_BIT_ADDRESS_SPACE” and compute that bool separately. @yln, @dvyukov WDYT?

That sounds good to me and even more expressive than what I suggested.

yln accepted this revision.Aug 25 2020, 10:01 AM

@dvyukov Any feedback here, or is this OK to go in?

@dvyukov a bit more ping :)

dvyukov accepted this revision.Dec 14 2020, 2:27 AM

Gosh, I somehow missed all pings... I don't know how it happened.
Please add vitalybuka@ for future tsan changes, having several people may help to avoid delays.
I don't see any issues with the change.

dyung added a subscriber: dyung.Mar 8 2021, 1:53 AM

Hi, our internal x86_64 linux builder is failing after your commit with the error:

In file included from ../rtl/tsan_rtl.h:44,
                 from /tmp/gotsan.OVwCLlYX3b/gotsan.cpp:13:
../rtl/tsan_platform.h:528:3: error: #error "Unknown platform"
  528 | # error "Unknown platform"
      |   ^~~~~

Can you please take a look?

kubamracek reopened this revision.Mar 8 2021, 6:54 AM
This revision is now accepted and ready to land.Mar 8 2021, 6:54 AM
kubamracek updated this revision to Diff 329018.Mar 8 2021, 8:00 AM
kubamracek added a subscriber: jmorse.

I've uploaded a new version of the patch (just moving the definition of HAS_48_BIT_ADDRESS_SPACE outside of the SANITIZER_GO branch). @dyung @jmorse could I ask you to check this patch whether it builds correctly on x86_64/linux? Thanks!

jmorse added a comment.Mar 8 2021, 8:33 AM

Hi,

I've uploaded a new version of the patch (just moving the definition of HAS_48_BIT_ADDRESS_SPACE outside of the SANITIZER_GO branch). @dyung @jmorse could I ask you to check this patch whether it builds correctly on x86_64/linux? Thanks!

Yup, for me that passes check-tsan and the 'GotsanRuntimeCheck' target that was failing for me before.

This revision was landed with ongoing or failed builds.Mar 8 2021, 2:10 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
102

This change is not correct for aarch64-*-linux.

I've fixed it in 32b684acc4490fbddce39d501e577cb029728e41

kubamracek added inline comments.Mar 11 2021, 9:28 AM
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
102

Thank you for fixing this!

Since I cannot find the mentioned hash anywhere, here's the fix for anyone else's reference: https://github.com/llvm/llvm-project/commit/5af991d46497c9473d2a0fd0989aa4ff4e6a0643

dblaikie added inline comments.
compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp
102

I can't seem to find that commit hash - could you check it's correct?