[PATCH] D48445: [sanitizer] Use "fast mmap" kernel flag for shadow memory on macOS 10.13.4+

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_posix_libcdep.cc:331
+// giant memory regions.
+#define kXnuFastMmapFd 0x4
+bool use_xnu_fast_mmap = false;
----------------
kubamracek wrote:
> delcypher wrote:
> > Is there a reason the `use_xnu_fast_mmap` isn't pushed into the Darwin implementation of `internal_mmap()`? Wouldn't it make more sense to be there given that xnu_fast_mmap is Darwin specific?
> We don't want to use this flag in internal_mmap, that would apply to all mmap calls. We really only want this flag for the shadow memory. I can add another argument to internal_mmap, but that looks to me like it would complicate things more than clarify them.
Why wouldn't you want to take the fast path for all mmap calls? I suppose it's the fast path for large allocations but not necessarily smaller ones?

If we're going with this approach there should probably be a comment or note in the commit message explaining why the decision to use fast_mmap has to belong in these locations and not in internal_mmap.

I'd kind of feel that the fast mmap option really should be a flag because that would allow us to pass that flag and then handle it in `internal_mmap()` (i.e. on non Darwin platforms, strip out the flag and use it appropriately on Darwin). That would avoid having to change all call sites to `internal_mmap()`.

The problem is mmap flag values are likely reserved and adding our own special value makes me feel uneasy because if another platform (or even Darwin) ever uses that flag value to mean something else, then semantics of internal_mmap will be broken. So I guess we have to do something else.


================
Comment at: lib/tsan/rtl/tsan_rtl.cc:361
+  __sanitizer::InitializePlatformEarly();
   InitializePlatformEarly();
+
----------------
kubamracek wrote:
> delcypher wrote:
> > `__sanitizer::InitializePlatformEarly()` is different from `InitializePlatformEarly()` that's called immediately afterwards?
> Yes. Tsan has `__tsan::InitializePlatformEarly`.
Ah. Personally I'd rather it read as

```
__sanitizer::InitializePlatformEarly();
__tsan::InitializePlatformEarly();
```

for clarity but I won't insist on this if you really want to write it how it is now.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48445