Page MenuHomePhabricator

[compiler-rt][asan] use full vm range on apple silicon macs
ClosedPublic

Authored by aralisza on Apr 9 2021, 3:17 PM.

Details

Summary

We previously shrunk the mmap range size on ios, but those settings got inherited by apple silicon macs.
Don't shrink the vm range on apple silicon Mac since we have access to the full range.

Also don't shrink vm range for iOS simulators because they have the same range as the host OS, not the simulated OS.

rdar://75302812

Diff Detail

Event Timeline

aralisza created this revision.Apr 9 2021, 3:17 PM
aralisza requested review of this revision.Apr 9 2021, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 3:17 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aralisza edited the summary of this revision. (Show Details)
aralisza updated this revision to Diff 336918.Mon, Apr 12, 11:50 AM

rebase after landing previous stack

aralisza updated this revision to Diff 336919.Mon, Apr 12, 11:50 AM

put back whitespace

yln accepted this revision.Mon, Apr 12, 12:00 PM

LGTM. Do we have a test that fails without this, but succeeds with it (on AS)?
I am just asking to understand if we missed this because we aren't testing on AS or because it only triggers in rare corner cases.

@kubamracek anything else to think about?

This revision is now accepted and ready to land.Mon, Apr 12, 12:00 PM
delcypher requested changes to this revision.Mon, Apr 12, 3:32 PM
delcypher added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
268

This doesn't look quite right. This change will mean we will use

define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 48)

on arm64 macOS. But there we want to be use using (1 << 47) to match x86_64 macOS, not (1 << 48).

Maybe something like this instead?

#elif defined(__aarch64__)
#  if SANITIZER_MAC
#    if SANITIZER_OSX
#      define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47) 
#    else
         // Darwin iOS/ARM64 has a 36-bit VMA, 64GiB VM
#      define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 36) 
#    endif     
#  else
#    define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 48)
#  endif

#elif defined(__sparc__)
....
This revision now requires changes to proceed.Mon, Apr 12, 3:32 PM

LGTM. Do we have a test that fails without this, but succeeds with it (on AS)?

We have a reproducer but it's much too complicated to have in the compiler-rt test suite. To reproduce we basically need mmap to return an address bigger than (1ULL << 36). I wonder if we could write a test case that mallocs a sufficient amount of memory so at the primary allocator is forced to allocate a region where the returned address is > (1ULL >> 36).

aralisza updated this revision to Diff 337502.Wed, Apr 14, 11:21 AM

make 47 for apple only

aralisza updated this revision to Diff 337941.Thu, Apr 15, 4:38 PM

use larger address space for iossim

aralisza updated this revision to Diff 337945.Thu, Apr 15, 4:47 PM

remove trailing whitespace

Harbormaster completed remote builds in B99056: Diff 337945.
aralisza updated this revision to Diff 338176.Fri, Apr 16, 11:25 AM

trailing whitespace

This revision is now accepted and ready to land.Fri, Apr 16, 12:32 PM
kubamracek accepted this revision.Sun, Apr 18, 4:32 PM
aralisza updated this revision to Diff 338572.Mon, Apr 19, 11:20 AM

rebase main

aralisza updated this revision to Diff 338591.Mon, Apr 19, 12:06 PM

put back indents

aralisza updated this revision to Diff 338592.Mon, Apr 19, 12:07 PM

indent one more time

This revision was landed with ongoing or failed builds.Mon, Apr 19, 12:12 PM
This revision was automatically updated to reflect the committed changes.