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

Fri Aug 21 16:12:53 PDT 2020

yln added a comment.

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.

================
Comment at: compiler-rt/lib/tsan/rtl/tsan_platform.h:29
+#if defined(__x86_64__) || (SANITIZER_MAC && \
+    !(SANITIZER_IOS && !SANITIZER_IOSSIM))
 /*
----------------
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)"

================
Comment at: compiler-rt/lib/tsan/rtl/tsan_platform_posix.cpp:103
+#if defined(__aarch64__) && SANITIZER_MAC && \
+    (SANITIZER_IOS && !SANITIZER_IOSSIM)
   ProtectRange(HeapMemEnd(), ShadowBeg());
----------------
"iOS (and derivatives) on arm64 (but not the simulators)" ?
because "arm64 && apple" doesn't imply iOS anymore.

Is the `SANITIZER_MAC` unnecessary then?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86377/new/

https://reviews.llvm.org/D86377