This is an archive of the discontinued LLVM Phabricator instance.

[asan][aarch64] Don't use 64 bit allocator for Apple ios family
ClosedPublic

Authored by rsundahl on Nov 30 2022, 10:07 AM.

Details

Summary

The recent change (https://reviews.llvm.org/D137136) to unconditionally
choose the 64 bit allocator on aarch64 breaks Apple iOS family of devices
which purposely use a smaller address space than is used with macOS.

rdar://102527313

Diff Detail

Event Timeline

rsundahl created this revision.Nov 30 2022, 10:07 AM
rsundahl requested review of this revision.Nov 30 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 10:07 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
rsundahl removed rG LLVM Github Monorepo as the repository for this revision.
thetruestblue accepted this revision.EditedNov 30 2022, 10:24 AM

This looks fine to me.

Although this comment:

//   - SANITIZER_IOS: devices (iOS and iOS-like)
//     - SANITIZER_WATCHOS
//     - SANITIZER_TVOS

Seems to suggest that SANITIZER_IOS includes all IOS-like devices.
Can we not do (SANITIZER_APPLE && SANITIZER_IOS) ?

And if the logic is that simple can we add it to the existing condition?

Edit: or, actually, can we not just add SANITIZER_IOS to existing condition?

This revision is now accepted and ready to land.Nov 30 2022, 10:24 AM
rsundahl edited the summary of this revision. (Show Details)Nov 30 2022, 10:28 AM

Can we not do (SANITIZER_APPLE && SANITIZER_IOS) ?

I didn't want to depend on the overloading of SANITIZER_IOS which means both actual iOS and similar to iOS, so I chose to come at the logic the way I did.

yln added inline comments.Nov 30 2022, 11:09 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
289–290
// - SANITIZER_APPLE: all Apple code
//   - TARGET_OS_OSX: macOS
//   - SANITIZER_IOS: devices (iOS and iOS-like)
//     - SANITIZER_WATCHOS
//     - SANITIZER_TVOS
//   - SANITIZER_IOSSIM: simulators (iOS and iOS-like)
//   - SANITIZER_DRIVERKIT

And if the logic is that simple can we add it to the existing condition?

Let's have a separate branch SANITIZER_APPLE to insulate a bit better from future changes.

Edit: or, actually, can we not just add SANITIZER_IOS to existing condition?

I agree, I think we should list the platforms to opt-out to avoid the double negation.

I think there is some additional complications:

  • For DriverKit, we can't tell if we are compiling for iPhone or macOS.
  • Did we consider the simulators?

I am assuming that the using ALLOCATOR64 is better if we can. The default is "yes" for 64-bit platforms (SANITIZER_WORDSIZE == 64), which is the fall through if no other branch says "don't use". So let's specify all those cases; include DriverKit (just to make sure DriverKit on iOS is covered).

yln added a comment.EditedNov 30 2022, 11:10 AM

Can we not do (SANITIZER_APPLE && SANITIZER_IOS) ?

I didn't want to depend on the overloading of SANITIZER_IOS which means both actual iOS and similar to iOS, so I chose to come at the logic the way I did.

I think we want to actually cover watchOS & tvOS which has the same (or even stricter) constraints...

thetruestblue added inline comments.Nov 30 2022, 11:18 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
289–290

+1 on this.

yln added inline comments.Nov 30 2022, 11:33 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
289–290
rsundahl updated this revision to Diff 479039.Nov 30 2022, 11:36 AM

Integrate suggestions from review.

rsundahl marked an inline comment as done.Nov 30 2022, 11:37 AM
rsundahl added inline comments.Nov 30 2022, 11:48 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
289–290

This will have the effect of unconditionally using the 32 bit allocator on Driverkit for x86_64 platforms.

thetruestblue added inline comments.Nov 30 2022, 12:28 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
289–290

Why do we need to specify DriverKit at all? Wouldn't the allocator be determined elsewhere in this logic? i.e. platform/wordsize?

The change as I originally made it was to use the 32 bit allocator in all of the cases that it had been used in up until things broke. I was conservative and explicit about enabling the 64 bit allocator for macOS because I tested it and it worked. The reviews here tell me that we want to go further but have questions so I'm going to revert to the original behavior of using the 32 bit allocator for SANITIZER_APPLE on aarch64.

rsundahl updated this revision to Diff 479053.Nov 30 2022, 1:05 PM

Revert to original use of 32 bit allocator in all SANITIZER_APPLE on aarch64

yln accepted this revision.Nov 30 2022, 1:12 PM

The change as I originally made it was to use the 32 bit allocator in all of the cases that it had been used in up until things broke. I was conservative and explicit about enabling the 64 bit allocator for macOS because I tested it and it worked. The reviews here tell me that we want to go further but have questions so I'm going to revert to the original behavior of using the 32 bit allocator for SANITIZER_APPLE on aarch64.

That's a good strategy, sorry for the distractions!

Yes, let's revert to previous state. We can then make incremental improvements once we are back in a working state.

rsundahl edited the summary of this revision. (Show Details)Nov 30 2022, 1:15 PM
This revision was landed with ongoing or failed builds.Nov 30 2022, 1:17 PM
This revision was automatically updated to reflect the committed changes.