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

Add System.Diagnostics.StackFrame.GetMethodFromNativeIP API for VS4Mac #61289

Merged
merged 6 commits into from
Nov 9, 2021

Conversation

mikem8361
Copy link
Member

VS4Mac needs a way to symbolize managed IPs when rethrowing a native NSException from Objective C++. This is a short term API needed for the next 6.0 service release discoverable only through reflection.

Issue: #61186

@ghost
Copy link

ghost commented Nov 6, 2021

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

Issue Details

VS4Mac needs a way to symbolize managed IPs when rethrowing a native NSException from Objective C++. This is a short term API needed for the next 6.0 service release discoverable only through reflection.

Issue: #61186

Author: mikem8361
Assignees: mikem8361
Labels:

area-Diagnostics-coreclr

Milestone: -

@mikem8361 mikem8361 changed the title Add System.Diagnostics.StackFrame.GetMethodInfoFromNativeIP API for VS4Mac Add System.Diagnostics.StackFrame.GetMethodFromNativeIP API for VS4Mac Nov 7, 2021
@am11
Copy link
Member

am11 commented Nov 8, 2021

mono apparently has mono_pmip native function for debuggers, which returns char* methodName for a given instruction pointer.

If VS4Mac's debugger also uses mono_pmip and method name is enough, can we expose it the same way from CoreCLR (without a QCall)?

@jkotas
Copy link
Member

jkotas commented Nov 8, 2021

There are design options for the crash diagnostic: managed APIs, unmanaged APIs, out-of-proc APIs, ... . We believe that adding this internal managed API is the best first step to enable VS Mac to get crash dump diagnostic. We plan to do some more refactoring on top of this and expose this functionality as a proper public managed API.

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.

A few comment suggestions, but functionally LGTM : )
Thanks Mike!

/// </summary>
/// <param name="ip">code address</param>
/// <returns>MethodBase instance for the method or null if IP not found</returns>
internal static MethodBase? GetMethodFromNativeIP(IntPtr ip)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a comment right here saying that the implementation of this method has known race conditions for unloadable code + MethodBase isn't a good design choice for working with NativeAOT, therefore don't make it public in the future without considering those issues. Reduces the chance we forget the history when we need a public API later.

Copy link
Member

Choose a reason for hiding this comment

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

Or even better - create issue that tracks adding the public API and link it from here

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that good enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did add a comment to the method's header.

Copy link
Member

Choose a reason for hiding this comment

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

I do think having the message that you added right here in the header is valuable. When people convert a private API to public nothing enforces that they will look at implementation code in a separate file. The comment you added calls out the race condition but doesn't mention the design issue for using MethodBase in NativeAOT. As @jkotas suggested filing an issue and linking to that is an improvement over my original suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to use issue #61186 to detail these limitations and to propose the new 7.0 API.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2021
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.

4 participants