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.

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

Skip putting patches in certain stubs to prevent crashing on step-in on M1 with JMC disabled #106105

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

mikelle-rogers
Copy link
Member

We set breakpoints in stubs when stepping into functions. On MacOS, it is not allowed to change binaries while code is running. Thus, if we attempt to put a patch into certain stubs which are in the text section of libcoreclr, the program will crash.

To prevent this crashing, we return false and skip/slide the step-in which then turns on the JMC probe.

The stubs that we are skipping putting patches in are VarargPInvokeStub, GenericPInvokeCalliHelper, JIT_TailCallLeave, JIT_TailCallVSDLeave, ThePreStubPatchLabel, NDirectImportThunk, ExternalMethodFixupPatchLabel, MulticastDebuggerTraceHelper

@mikelle-rogers mikelle-rogers self-assigned this Aug 8, 2024
@mikelle-rogers mikelle-rogers added this to the 9.0.0 milestone Aug 8, 2024
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@mikelle-rogers mikelle-rogers marked this pull request as ready for review August 8, 2024 17:12
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/debug/ee/controller.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
@AaronRobinsonMSFT AaronRobinsonMSFT self-requested a review August 8, 2024 18:39
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM, one comment inline.

src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
@tommcdon
Copy link
Member

@mikelle-rogers is everything ready to go for this PR, and if yes can we merge it?

@hoyosjs
Copy link
Member

hoyosjs commented Aug 12, 2024

contributes to #68018

@mikelle-rogers
Copy link
Member Author

mikelle-rogers commented Aug 12, 2024

I was waiting to see if @hoyosjs has anything else he wants to say before merging.

@hoyosjs
Copy link
Member

hoyosjs commented Aug 12, 2024

Generally, this looks good. My only question is what happens when you step into optimized code (e.g. step into an r2r function). What's the new expected behavior. We should go back to the issue and try to document this.

@mikelle-rogers mikelle-rogers merged commit 2ba659b into dotnet:main Aug 12, 2024
90 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants