This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers] Modified aarch64 to use the 64 bit version of the allocator.
ClosedPublic

Authored by kstoimenov on Oct 31 2022, 4:42 PM.

Download Raw Diff

Details

Reviewers

vitalybuka
thurston
MaskRay

Commits

rGa588cfe37ea3: [Sanitizers] Modified __aarch64__ to use the 64 bit version of the allocator.
rG75a8cdbc0659: [Sanitizers] Modified __aarch64__ to use the 64 bit version of the allocator.

Summary

This change will switch SizeClassAllocator32 to SizeClassAllocator64 on ARM. This might potentially affect ARM platforms with 39-bit address space. This addresses issues/703, but unlike D60243 it defaults to 64 bit allocator.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

kstoimenov created this revision.Oct 31 2022, 4:42 PM

Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:42 PM

Herald added subscribers: Enna1, s.egerton, simoncook, kristof.beyls. · View Herald Transcript

kstoimenov requested review of this revision.Oct 31 2022, 4:42 PM

Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:42 PM

Herald added subscribers: Restricted Project, • pcwang-thead. · View Herald Transcript

kstoimenov edited the summary of this revision. (Show Details)Oct 31 2022, 4:44 PM

kstoimenov added reviewers: vitalybuka, thurston.

does check-all works?

Can you please include https://github.com/google/sanitizers/issues/703 to git commit and review message

Harbormaster completed remote builds in B195390: Diff 472184.Oct 31 2022, 5:02 PM

kstoimenov edited the summary of this revision. (Show Details)Oct 31 2022, 5:23 PM

vitalybuka accepted this revision.Oct 31 2022, 5:49 PM

This revision is now accepted and ready to land.Oct 31 2022, 5:49 PM

vitalybuka added inline comments.Oct 31 2022, 5:53 PM

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
287–290	Maybe

I included issues/703 and check-all passes on my machine.

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
287–290	But this would affect SANITIZER_ANDROID, right?

I noticed lsan slowness on aarch64 as well. Thanks for the fix!

Herald added a subscriber: StephenFan. · View Herald TranscriptNov 1 2022, 10:30 AM

In D137136#3899601, @MaskRay wrote:

I noticed lsan slowness on aarch64 as well. Thanks for the fix!

It's one of required patches, @kstoimenov can upload other patches as well, and link using stack?

vitalybuka added a subscriber: phosek.Nov 1 2022, 10:48 AM

vitalybuka added inline comments.

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
287–290	Not on ARM64 I believe android and fuchsia IF here is to override aarch64 parrt you already removing. now they will fallback into "# else" branch and have 1 because of SANITIZER_WORDSIZE == 64 Some concerns if someone actually care about mips64 or RISCV64 on android/fuchsia, and if existing state is working there at all. @phosek But I think you should try to simplify that.

kstoimenov added inline comments.Nov 1 2022, 1:47 PM

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
287–290	I will submit as is and will send a patch which simplifies it.

Closed by commit rG75a8cdbc0659: [Sanitizers] Modified __aarch64__ to use the 64 bit version of the allocator. (authored by kstoimenov). · Explain WhyNov 1 2022, 1:49 PM

This revision was automatically updated to reflect the committed changes.

kstoimenov added a commit: rG75a8cdbc0659: [Sanitizers] Modified __aarch64__ to use the 64 bit version of the allocator..

Is it possible that this broke this fuzzer?

https://lab.llvm.org/buildbot/#/builders/240/builds/981/steps/8/logs/stdio

AddressSanitizer: CHECK failed: asan_poisoning.cpp:38 "((AddrIsInMem(addr))) != (0)" (0x0, 0x0) (tid=3685666)
    <empty stack>

vitalybuka added a reverting change: rGe02110e2ab4d: Revert "[Sanitizers] Modified __aarch64__ to use the 64 bit version of the….Nov 1 2022, 5:06 PM

vitalybuka reopened this revision.Nov 1 2022, 5:06 PM

This revision is now accepted and ready to land.Nov 1 2022, 5:06 PM

kstoimenov added a parent revision: D137265: [ASAN] Removed special case controlling allocator constants for __aarch64__..Nov 2 2022, 9:18 AM

kstoimenov mentioned this in D137265: [ASAN] Removed special case controlling allocator constants for __aarch64__..Nov 2 2022, 9:21 AM

kstoimenov mentioned this in rGa8604f2254f6: [ASAN] Removed special case controlling allocator constants for __aarch64__..Nov 2 2022, 10:28 AM

Closed by commit rGa588cfe37ea3: [Sanitizers] Modified __aarch64__ to use the 64 bit version of the allocator. (authored by kstoimenov). · Explain WhyNov 2 2022, 2:22 PM

This revision was automatically updated to reflect the committed changes.

kstoimenov added a commit: rGa588cfe37ea3: [Sanitizers] Modified __aarch64__ to use the 64 bit version of the allocator..

rsundahl mentioned this in D139030: [asan][aarch64] Don't use 64 bit allocator for Apple ios family.Nov 30 2022, 10:07 AM

rsundahl mentioned this in rGad6c8092dcb2: [asan][aarch64] Don't use 64 bit allocator for Apple ios family.Nov 30 2022, 1:17 PM

thetruestblue mentioned this in D141171: [Sanitizer] Clean up SANITIZER_CAN_USE_ALLOCATOR64 logic.Jan 10 2023, 11:30 AM

Blue Gaston <bgaston2@apple.com> mentioned this in rG87850e9bb155: [Sanitizer] Clean up SANITIZER_CAN_USE_ALLOCATOR64 logic.Jan 13 2023, 9:58 AM

Revision Contents

Path

Size

compiler-rt/

lib/

sanitizer_common/

sanitizer_platform.h

4 lines

Diff 472389

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

Show First 20 Lines • Show All 278 Lines • ▼ Show 20 Lines

#endif

// By default we allow to use SizeClassAllocator64 on 64-bit platform.

// But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64

// does not work well and we need to fallback to SizeClassAllocator32.

// For such platforms build this code with -DSANITIZER_CAN_USE_ALLOCATOR64=0 or

// change the definition of SANITIZER_CAN_USE_ALLOCATOR64 here.

#ifndef SANITIZER_CAN_USE_ALLOCATOR64

# if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA

# define SANITIZER_CAN_USE_ALLOCATOR64 1

# elif defined(__mips64) || defined(__aarch64__) || defined(__i386__) || \

# elif defined(__mips64) || defined(__arm__) || defined(__i386__) || \

defined(__arm__) || SANITIZER_RISCV64 || defined(__hexagon__)

SANITIZER_RISCV64 || defined(__hexagon__)

vitalybukaUnsubmitted

Not Done

#ifndef SANITIZER_CAN_USE_ALLOCATOR64

- # if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA

- # define SANITIZER_CAN_USE_ALLOCATOR64 1

- # elif defined(__mips64) || defined(__arm__) || defined(__i386__) || \

+ # if defined(__mips64) || defined(__arm__) || defined(__i386__) || \

SANITIZER_RISCV64 || defined(__hexagon__)

Maybe

vitalybuka: Maybe

kstoimenovAuthorUnsubmitted

Done

But this would affect SANITIZER_ANDROID, right?

kstoimenov: But this would affect SANITIZER_ANDROID, right?

vitalybukaUnsubmitted

Not Done

Not on ARM64

I believe android and fuchsia IF here is to override aarch64 parrt you already removing.
now they will fallback into "# else" branch and have 1 because of SANITIZER_WORDSIZE == 64

Some concerns if someone actually care about mips64 or RISCV64 on android/fuchsia, and if existing state is working there at all. @phosek
But I think you should try to simplify that.

vitalybuka: Not on ARM64 I believe android and fuchsia IF here is to override __aarch64__ parrt you…

kstoimenovAuthorUnsubmitted

Done

I will submit as is and will send a patch which simplifies it.

kstoimenov: I will submit as is and will send a patch which simplifies it.

# define SANITIZER_CAN_USE_ALLOCATOR64 0

# else

# define SANITIZER_CAN_USE_ALLOCATOR64 (SANITIZER_WORDSIZE == 64)

# endif

#endif

// The range of addresses which can be returned my mmap.

// FIXME: this value should be different on different platforms. Larger values

▲ Show 20 Lines • Show All 151 Lines • Show Last 20 Lines