Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add System.Diagnostics.StackFrame.GetMethodFromNativeIP API for VS4Mac #61289
Changes from 2 commits
6b7f5a0
dbe2368
9a14e5d
b3a9a60
3aeaed6
308f640
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a TODO describing most of this here: https://github.com/mikem8361/runtime/blob/b3a9a6086dcef769b82a5750489738254b6aee80/src/coreclr/vm/debugdebugger.cpp#L788
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that good enough?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.