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

[yln]

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?

[kubamracek]

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]

In D86377#2231641, @kubamracek wrote:

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.

[kubamracek]

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

[kubamracek]

@dvyukov friendly ping :)

[kubamracek]

@dvyukov more ping :)

[kubamracek]

@dvyukov a bit more ping :)

[dvyukov]

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]

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]

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]

Hi,

In D86377#2611282, @kubamracek wrote:

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.

[MaskRay]

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

102–103

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

I've fixed it in 32b684acc4490fbddce39d501e577cb029728e41

[kubamracek]

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

102–103

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]

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

102–103

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

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

 
 #include "tsan_defs.h"
 #include "tsan_trace.h"
 
 namespace __tsan {
 
 #if !SANITIZER_GO
 
-#if defined(__x86_64__)
+#if defined(__x86_64__) || (SANITIZER_MAC && \
+    !(SANITIZER_IOS && !SANITIZER_IOSSIM))
 /*
 C/C++ on linux/x86_64 and freebsd/x86_64
 0000 0000 1000 - 0080 0000 0000: main binary and/or MAP_32BIT mappings (512GB)
 0040 0000 0000 - 0100 0000 0000: -
 0100 0000 0000 - 2000 0000 0000: shadow
 2000 0000 0000 - 3000 0000 0000: -
 3000 0000 0000 - 4000 0000 0000: metainfo (memory blocks and sync objects)
 4000 0000 0000 - 5500 0000 0000: -

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

if (segment.protection == 0)  // Zero page or mprotected.
       continue;
     if (segment.start >= VdsoBeg())  // vdso
       break;
     Printf("FATAL: ThreadSanitizer: unexpected memory mapping %p-%p\n",
            segment.start, segment.end);
     Die();
   }
 
-#if defined(__aarch64__) && defined(__APPLE__)
+#if defined(__aarch64__) && SANITIZER_MAC && \
+    (SANITIZER_IOS && !SANITIZER_IOSSIM)
   ProtectRange(HeapMemEnd(), ShadowBeg());
   ProtectRange(ShadowEnd(), MetaShadowBeg());
   ProtectRange(MetaShadowEnd(), TraceMemBeg());
 #else
   ProtectRange(LoAppMemEnd(), ShadowBeg());
   ProtectRange(ShadowEnd(), MetaShadowBeg());
 #ifdef TSAN_MID_APP_RANGE
   ProtectRange(MetaShadowEnd(), MidAppMemBeg());