Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

i#5383: Fix macOS preload on a64 #7170

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

i#5383: Fix macOS preload on a64 #7170

wants to merge 15 commits into from

Conversation

ndrewh
Copy link
Contributor

@ndrewh ndrewh commented Dec 26, 2024 •
edited
Loading

This fixes several build and runtime issues under macOS on ARM64 on macOS 14.4.1 (Sonoma)

  • FEATURE_PAUTH is now detected on macOS using a sysctl (capability MSRs cannot be read in userspace on M3 at least)
  • In the preload library and os.c, _init needs __attribute__((constructor)) to be run by dyld
  • PLATFORM_SUPPORTS_SCATTER_GATHER is broken, disabled for now on a64 macOS
  • Fixed sigcontext/mcontext conversion which assumes old simd size and was failing on an assert

Known issues that remain:

  • VM allocation frequently but nondeterministically fails reachability constraints. -vm_size 500M seems to fix this. i#5383: Fix macOS arm64 test build/run #7171 adds this flag to tests on macOS+ARM64. Another option is to make this the default on macOS+ARM64.
  • Sometimes some bookkeeping asserts in vmh_exit are reached upon process exit

The end result is that the bbcount tool works on a simple toy program on macOS, in debug mode, without hitting any asserts:

bin64/drrun -debug -vm_size 500M -c api/bin/libbbcount.dylib -- ./main

@ndrewh
Copy link
Contributor Author

ndrewh commented Dec 26, 2024

It appears I broke the x86 macOS tests, not sure what's going on there yet

@ndrewh ndrewh changed the title i#5383: Fix macOS hello world on a64 i#5383: Fix macOS preload on a64 Dec 26, 2024
@abhinav92003
Copy link
Contributor

It appears I broke the x86 macOS tests, not sure what's going on there yet

If it helps: check out the "Debugging Tests on Github Actions Runner" section at https://dynamorio.org/page_test_suite.html, which documents a way to ssh into the failing test machine.

@ndrewh
Copy link
Contributor Author

ndrewh commented Dec 27, 2024 •
edited
Loading

Alright somehow adding IF_MACOS(__attribute__((constructor))) to the _init function in preload.c is breaking the x86 tests. I can't say I understand the situation at all:

  • on ARM64 macOS 14.4 the preload doesn't work at all without __attribute__((constructor)), which causes ~all of the client tests to fail
  • on the x86 macOS runner (which is on macOS 13):
    • All of the tests seem to pass without the attribute (??? did dyld stop running _init in macOS 14? I searched https://github.com/apple-oss-distributions/dyld and i'm not seeing where they ever ran _init -- maybe some compilers were automatically putting it in the appropriate section but recently changed this behavior?)
    • Once you add the attribute some (but not all!) of the client tests fail with some seemingly random instrumentation assert.

@ndrewh
Copy link
Contributor Author

ndrewh commented Dec 27, 2024

I don't think api.startstop fail is my fault, but I also don't see anything to suggest it's #6928

edeiana
edeiana requested changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@edeiana edeiana left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "Issue #5383" at the end of the PR description as suggested in the commit template.

(Thank you for contributing in making DynamoRIO work better on MacOS!)

core/iox.h Show resolved Hide resolved
core/arch/aarch64/proc.c Outdated Show resolved Hide resolved
core/arch/aarch64/proc.c Outdated Show resolved Hide resolved
core/arch/aarch64/proc.c Outdated Show resolved Hide resolved
core/unix/os_public.h Show resolved Hide resolved
core/unix/signal_macos.c
ASSERT(sizeof(mc->simd) == sizeof(fpc->__v));
memcpy(&mc->simd, &fpc->__v, sizeof(mc->simd));
for (int i = 0; i < proc_num_simd_registers(); i++) {
memcpy(&mc->simd[i], &fpc->__v[i], sizeof(fpc->__v[i]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ASSERT(sizeof(mc->simd[i]) == sizeof(fpc->__v[i])) before this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below comment

core/unix/signal_macos.c
ASSERT(sizeof(mc->simd) == sizeof(fpc->__v));
memcpy(&fpc->__v, &mc->simd, sizeof(mc->simd));
for (int i = 0; i < proc_num_simd_registers(); i++) {
memcpy(&fpc->__v[i], &mc->simd[i], sizeof(fpc->__v[i]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ASSERT(sizeof(mc->simd[i]) == sizeof(fpc->__v[i])) before this line.

Copy link
Contributor Author

@ndrewh ndrewh Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh uh these things aren't the same size since simd got enlarged. We might need to zero the upper bits then? But none of the other arches in sigcontext_to_mcontext_simd seem to zero the upper bits...

dr_simd_t is 512-bit on aarch64
__v is __uint128_t https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/osfmk/mach/arm/_structs.h#L573

ext/drx/drx.c Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edeiana edeiana edeiana requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ndrewh @abhinav92003 @edeiana