[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

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

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

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__)
+#define HAS_48_BIT_ADDRESS_SPACE 1
+#elif SANITIZER_IOSSIM // arm64 iOS simulators (order of #if matters)
+#define HAS_48_BIT_ADDRESS_SPACE 1
+#elif SANITIZER_IOS // arm64 iOS devices (order of #if matters)
+#define HAS_48_BIT_ADDRESS_SPACE 0
+#elif SANITIZER_MAC // arm64 macOS (order of #if matters)
+#define HAS_48_BIT_ADDRESS_SPACE 1
+#else
+#define HAS_48_BIT_ADDRESS_SPACE 0
+#endif
+
+#if HAS_48_BIT_ADDRESS_SPACE
 /*
 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: -
struct Mapping {
   static const uptr kTraceMemEnd   = 0x0fc0000000ull;
   static const uptr kHiAppMemBeg   = 0x0fc0000000ull;
   static const uptr kHiAppMemEnd   = 0x0fc0000000ull;
   static const uptr kAppMemMsk     =          0x0ull;
   static const uptr kAppMemXor     =          0x0ull;
   static const uptr kVdsoBeg       = 0x7000000000000000ull;
 };
 
-#elif defined(__aarch64__)
+#elif defined(__aarch64__) && !defined(__APPLE__)
 // AArch64 supports multiple VMA which leads to multiple address transformation
 // functions.  To support these multiple VMAS transformations and mappings TSAN
 // runtime for AArch64 uses an external memory read (vmaSize) to select which
 // mapping to use.  Although slower, it make a same instrumented binary run on
 // multiple kernels.
 
 /*
 C/C++ on linux/aarch64 (39-bit VMA)
struct Mapping47 {
   static const uptr kAppMemXor     = 0x020000000000ull;
   static const uptr kVdsoBeg       = 0x7800000000000000ull;
 };
 
 // Indicates the runtime will define the memory regions at runtime.
 #define TSAN_RUNTIME_VMA 1
 #endif
 
-#elif SANITIZER_GO && !SANITIZER_WINDOWS && defined(__x86_64__)
+#elif SANITIZER_GO && !SANITIZER_WINDOWS && HAS_48_BIT_ADDRESS_SPACE
 
 /* Go on linux, darwin and freebsd on x86_64
 0000 0000 1000 - 0000 1000 0000: executable
 0000 1000 0000 - 00c0 0000 0000: -
 00c0 0000 0000 - 00e0 0000 0000: heap
 00e0 0000 0000 - 2000 0000 0000: -
 2000 0000 0000 - 2380 0000 0000: shadow
 2380 0000 0000 - 3000 0000 0000: -

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

static void my_pthread_introspection_hook(unsigned int event, pthread_t thread,
   }
 
   if (prev_pthread_introspection_hook != nullptr)
     prev_pthread_introspection_hook(event, thread, addr, size);
 }
 #endif
 
 void InitializePlatformEarly() {
-#if !SANITIZER_GO && defined(__aarch64__)
+#if !SANITIZER_GO && !HAS_48_BIT_ADDRESS_SPACE
   uptr max_vm = GetMaxUserVirtualAddress() + 1;
   if (max_vm != Mapping::kHiAppMemEnd) {
     Printf("ThreadSanitizer: unsupported vm address limit %p, expected %p.\n",
            max_vm, Mapping::kHiAppMemEnd);
     Die();
   }
 #endif
 }

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__) && !HAS_48_BIT_ADDRESS_SPACE
   ProtectRange(HeapMemEnd(), ShadowBeg());
   ProtectRange(ShadowEnd(), MetaShadowBeg());
   ProtectRange(MetaShadowEnd(), TraceMemBeg());
 #else
   ProtectRange(LoAppMemEnd(), ShadowBeg());
   ProtectRange(ShadowEnd(), MetaShadowBeg());
 #ifdef TSAN_MID_APP_RANGE
   ProtectRange(MetaShadowEnd(), MidAppMemBeg());
   ProtectRange(MidAppMemEnd(), TraceMemBeg());