This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kubamracek on Jun 21 2018, 10:56 AM.

Details

Summary

This speeds up process startup and teardown and also reduces lock contention when running multiple ASanified/TSanified processes simultaneously. Should greatly improve lit testing time.

Diff Detail

Event Timeline

kubamracek created this revision.Jun 21 2018, 10:56 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJun 21 2018, 10:56 AM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_mac.cc
754

Would you like to resolve this by moving into your new function?
Probably in the next patch.

kubamracek added inline comments.Jun 21 2018, 11:29 AM
lib/sanitizer_common/sanitizer_mac.cc
754

Yes, nice catch! Will send another patch after this one.

delcypher requested changes to this revision.Jun 21 2018, 11:54 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_posix_libcdep.cc
333

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?

398

Again can't we move this into the Darwin implementation of internal_mmap(). This will also mean we avoid duplicating code.

lib/tsan/rtl/tsan_rtl.cc
365–367

__sanitizer::InitializePlatformEarly() is different from InitializePlatformEarly() that's called immediately afterwards?

This revision now requires changes to proceed.Jun 21 2018, 11:54 AM
kubamracek added inline comments.Jun 21 2018, 12:29 PM
lib/sanitizer_common/sanitizer_posix_libcdep.cc
333

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.

lib/tsan/rtl/tsan_rtl.cc
365–367

Yes. Tsan has __tsan::InitializePlatformEarly.

delcypher added inline comments.Jun 22 2018, 10:54 AM
lib/sanitizer_common/sanitizer_posix_libcdep.cc
333

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.

lib/tsan/rtl/tsan_rtl.cc
365–367

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.

kubamracek added inline comments.Jun 22 2018, 11:03 AM
lib/sanitizer_common/sanitizer_posix_libcdep.cc
333

I'll add the comment with a bit of explanation, but I think you won't be fully satisfied because I don't understand what exactly the behavior of this flag is either. It was added into xnu specifically for the sanitizers and it's supposed to only be used for the shadow memory (giant region sizes), and not for other memory allocations. I don't know why.

The interface of internal_mmap is exactly the same as mmap, so I don't think we want to change it, or pass "fd" into "flags".

lib/tsan/rtl/tsan_rtl.cc
365–367

Sure, I'll change it.

delcypher added inline comments.Jul 20 2018, 10:10 AM
lib/sanitizer_common/sanitizer_posix_libcdep.cc
336

Side note. This implementation will prevent the VM tag being set (see https://reviews.llvm.org/D49603 ) with fast mmap is being used. This needs to be fixed.

398

Side note. This implementation will prevent the VM tag being set (see https://reviews.llvm.org/D49603 ) with fast mmap is being used. This needs to be fixed.

Updating patch.

delcypher added inline comments.Jul 25 2018, 8:38 AM
lib/sanitizer_common/sanitizer_mac.cc
127

Thanks for fixing the VM tag issue!

lib/sanitizer_common/sanitizer_posix_libcdep.cc
338–340

The name internal_mmap_shadow(...) implies that this function is only for use in allocating shadow regions. However the name MmapFixedNoReserve() doesn't imply that. However, looking at the uses
of MmapFixedNoReserve() it looks like they are only used for allocating "shadow" memory currently, so this is probably okay but maybe it should be named internal_mmap_large() instead because semantically that's what differentiates it from internal_mmap()?

kubamracek added inline comments.Jul 25 2018, 9:03 AM
lib/sanitizer_common/sanitizer_posix_libcdep.cc
338–340

“large” to me doesn’t imply what I mean here. A 10 MB mmap is “large”. But “shadow” regions imply they are proportional in size to the entire VM address space.

delcypher added inline comments.Jul 26 2018, 3:56 AM
lib/sanitizer_common/sanitizer_posix_libcdep.cc
338–340

Ok. By "large" I mean extremely "large" but I guess that could also be confusing. I guess the remaining thing to address is MmapFixedNoReserve() calling internal_mmap_shadow(). I don't like MmapFixedNoReserve() (which has a name that does not imply it is for shadow memory) calling a function whose name implies it is for use for shadow memory.

A comment on MmapFixedNoReserve() saying it is meant for allocating shadow memory might be sufficient.

Ok. By "large" I mean extremely "large" but I guess that could also be confusing. I guess the remaining thing to address is MmapFixedNoReserve() calling internal_mmap_shadow(). I don't like MmapFixedNoReserve() (which has a name that does not imply it is for shadow memory) calling a function whose name implies it is for use for shadow memory.

I've updated the patch to now split out MmapFixedNoReserve into MmapFixedNoReserveShadow. The latter is used when allocating shadow regions.

delcypher accepted this revision.Aug 22 2018, 1:44 AM

Other than the kXnuFastMmapFd declaration. LGTM.

lib/sanitizer_common/sanitizer_mac.cc
120

Any particular reason to use a macro rather than static const int kXnuFastMmapFd = 0x4;? The later style seems used more in the sanitizer code base (at least the bits I've seen). This is a very minor issue though so I'll leave changing this to your discretion.

This revision is now accepted and ready to land.Aug 22 2018, 1:44 AM

Changing the #define to a constant. Rebasing patch.

Adding MmapFixedNoAccessShadow.

This revision is marked as "Accepted", but the last round of changes are touching other files than before. @kcc, @dvyukov, @eugenis, are you okay with these changes?

I don't like the new Mmap*Shadow functions. The number of different Mmap* functions is growing exponentially with any new feature, let's use flags and/or arguments instead.

It is also not clear from the user side what this Shadow suffix means exactly. If that's an optimization for huge mappings, can't we define a size threshold when it is more efficient than the alternative, and use it under the hood in common Mmap* calls?

lib/sanitizer_common/sanitizer_mac.cc
113

Leftover debug? More of the same below.

eugenis requested changes to this revision.Sep 17 2018, 1:04 PM
This revision now requires changes to proceed.Sep 17 2018, 1:04 PM

Okay, simplifying this patch back, and only applying the "fast mmap" flag based on a size threshold.

eugenis accepted this revision.Nov 5 2018, 3:21 PM

LGTM

This revision is now accepted and ready to land.Nov 5 2018, 3:21 PM
This revision was automatically updated to reflect the committed changes.

@earthdok

r346264 | d0k | 2018-11-06 12:42:19 -0800 (Tue, 06 Nov 2018) | 11 lines

[dfsan] Fix build after r346262

compiler-rt/lib/dfsan/dfsan.cc:426:3: error: call to 'InitializePlatformEarly' is ambiguous
  InitializePlatformEarly();
  ^~~~~~~~~~~~~~~~~~~~~~~
compiler-rt/lib/dfsan/../sanitizer_common/sanitizer_common.h:901:6: note: candidate function
void InitializePlatformEarly();
     ^
compiler-rt/lib/dfsan/dfsan.cc:391:13: note: candidate function
static void InitializePlatformEarly() {
            ^

Thanks and sorry about this! Please let me know if the failure is still there!