This is an archive of the discontinued LLVM Phabricator instance.

Differential D100234

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

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

Details

Reviewers
delcypher
kubamracek
yln
Commits
rG94ba3b6e3bb3: [compiler-rt][asan] use full vm range on apple silicon macs
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
Herald added a subscriber: dberris. · View Herald TranscriptApr 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 added a parent revision: D100157: [compiler-rt] add SANITIZER_OSX.Apr 9 2021, 3:32 PM
aralisza added reviewers: delcypher, kubamracek, yln.Apr 9 2021, 3:50 PM
aralisza edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B98076: Diff 336570.Apr 9 2021, 3:53 PM
aralisza updated this revision to Diff 336918.Apr 12 2021, 11:50 AM

rebase after landing previous stack

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

put back whitespace

yln accepted this revision.Apr 12 2021, 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.Apr 12 2021, 12:00 PM
Harbormaster completed remote builds in B98329: Diff 336918.Apr 12 2021, 12:58 PM
Harbormaster completed remote builds in B98330: Diff 336919.Apr 12 2021, 1:34 PM
delcypher requested changes to this revision.Apr 12 2021, 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.Apr 12 2021, 3:32 PM
delcypher added a comment.Apr 12 2021, 3:38 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.Apr 14 2021, 11:21 AM

make 47 for apple only

Harbormaster completed remote builds in B98726: Diff 337502.Apr 14 2021, 11:56 AM
aralisza updated this revision to Diff 337941.Apr 15 2021, 4:38 PM

use larger address space for iossim

aralisza added a child revision: D100239: [compiler-rt] assert max virtual address is <= mmap range size.Apr 15 2021, 4:43 PM
aralisza edited the summary of this revision. (Show Details)
aralisza updated this revision to Diff 337945.Apr 15 2021, 4:47 PM

remove trailing whitespace

Harbormaster completed remote builds in B99053: Diff 337941.Apr 15 2021, 6:04 PM
Harbormaster completed remote builds in B99056: Diff 337945.
aralisza updated this revision to Diff 338176.Apr 16 2021, 11:25 AM

trailing whitespace

delcypher accepted this revision.Apr 16 2021, 12:32 PM

LGTM

This revision is now accepted and ready to land.Apr 16 2021, 12:32 PM
Harbormaster completed remote builds in B99219: Diff 338176.Apr 16 2021, 1:36 PM
kubamracek accepted this revision.Apr 18 2021, 4:32 PM
aralisza updated this revision to Diff 338572.Apr 19 2021, 11:20 AM

rebase main

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

put back indents

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

indent one more time

This revision was landed with ongoing or failed builds.Apr 19 2021, 12:12 PM
Closed by commit rG94ba3b6e3bb3: [compiler-rt][asan] use full vm range on apple silicon macs (authored by aralisza). · Explain Why
This revision was automatically updated to reflect the committed changes.
aralisza added a commit: rG94ba3b6e3bb3: [compiler-rt][asan] use full vm range on apple silicon macs.
Harbormaster completed remote builds in B99519: Diff 338572.Apr 19 2021, 12:20 PM
Harbormaster completed remote builds in B99533: Diff 338592.Apr 19 2021, 1:04 PM
Harbormaster completed remote builds in B99532: Diff 338591.Apr 19 2021, 1:29 PM

Revision Contents

PathSize
compiler-rt/
lib/
sanitizer_common/
2 lines

Diff 336918

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

Show First 20 Lines • Show All 259 Lines • ▼ Show 20 Lines
#if SANITIZER_GO && defined(__mips64) #if SANITIZER_GO && defined(__mips64)
#define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47) #define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47)
#else #else
# define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 40) # define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 40)
#endif #endif
#elif SANITIZER_RISCV64 #elif SANITIZER_RISCV64
#define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 38) #define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 38)
#elif defined(__aarch64__) #elif defined(__aarch64__)
# if SANITIZER_MAC #if SANITIZER_MAC && !SANITIZER_OSX
delcypherUnsubmitted
Not Done
ReplyInline Actions

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__)
....
delcypher: This doesn't look quite right. This change will mean we will use ``` define…
// Darwin iOS/ARM64 has a 36-bit VMA, 64GiB VM // Darwin iOS/ARM64 has a 36-bit VMA, 64GiB VM
# define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 36) # define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 36)
# else # else
# define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 48) # define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 48)
# endif # endif
#elif defined(__sparc__) #elif defined(__sparc__)
#define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 52) #define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 52)
#else #else
▲ Show 20 Lines • Show All 113 Lines • Show Last 20 Lines