Skip to content

Conversation

jgiannuzzi
Copy link
Contributor

Closes #65341.

This PR changes how R2R images are loaded on Apple Silicon by following Apple's guidelines. Without this change, debugging is broken on this platform, as the R2R memory cannot be written to.

@ghost
Copy link

ghost commented Mar 24, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 24, 2022
@jgiannuzzi
Copy link
Contributor Author

@janvorli @hoyosjs please take a look!

This allows to keep the instruction cache flush logic identical between platforms.
Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Not sure if pulsing the write mode is a perf concern as before. Seems in line with what we did before and seems pretty clean now in terms of readability. Also confirmed that the debugger breakpoints work as expected and that the code that's running comes from the R2R image itself. Thanks @jgiannuzzi

@hoyosjs
Copy link
Member

hoyosjs commented Mar 28, 2022

@janvorli @jkotas does this look ok now? If it's going to make the next 6.0 shipping date, it needs to be ported relatively soon.

if (((pSection->Characteristics & VAL32(IMAGE_SCN_MEM_EXECUTE)) != 0))
{
#ifdef __APPLE__
dwNewProtection = PAGE_READWRITE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkotas Note that this was introduced in #61938 for fixing the loading of R2R images on Apple Silicon. I believe this was never needed for Intel, hence why I removed it here. @VSadov could you please confirm that this sounds right to you?

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mangod9 mangod9 merged commit 6409642 into dotnet:main Mar 31, 2022
@mangod9
Copy link
Member

mangod9 commented Mar 31, 2022

Thanks for fixing @jgiannuzzi

@mangod9
Copy link
Member

mangod9 commented Mar 31, 2022

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2074029121

@github-actions
Copy link
Contributor

@mangod9 backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Load R2R images on osx-arm64 according to Apple's guidelines
Using index info to reconstruct a base tree...
M	src/coreclr/pal/src/map/map.cpp
M	src/coreclr/vm/peimagelayout.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/peimagelayout.cpp
Auto-merging src/coreclr/pal/src/map/map.cpp
CONFLICT (content): Merge conflict in src/coreclr/pal/src/map/map.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Load R2R images on osx-arm64 according to Apple's guidelines
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@jgiannuzzi
Copy link
Contributor Author

@mangod9 I can take care of the backport — it will have to include my reverted PR #64104 as well to be effective.

@jgiannuzzi jgiannuzzi deleted the debug-fix-pthread_jit branch April 1, 2022 12:40
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Apple ARM64] dotnet crashing when debugging in .NET 7 (7.0.100-preview.2.22101.4)

7 participants