[release/6.0.x] Add System.Diagnostics.StackFrame.GetMethodInfoFromNativeIP API for VS4Mac

[mikem8361]

Customer Impact

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

Testing

Local testing. Verified by the VS4Mac team.

Risk

Low.

[ghost]

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

Issue Details

---

Customer Impact

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

Testing

Local testing. Verified by the VS4Mac team.

Risk

Low.

Author:	mikem8361
Assignees:	mikem8361
Labels:	area-Diagnostics-coreclr
Milestone:	-

[mikem8361]

VS4Mac uses reflection to get to and call it and if it isn't in the linker file it gets removed.

[MichalStrehovsky]

VS4Mac uses reflection to get to and call it and if it isn't in the linker file it gets removed.

I assume this happens during the repo build because the API is not public. ILLink.Descriptors.Shared.xml are used both at repo build time and end user app build time. We don't want to root an unused API in all .NET apps just because VS4Mac uses it.

This directive should go in the LibraryBuild.xml file like here:

runtime/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

Line 53 in 7cd8f19

<ILLinkDescriptorsLibraryBuildXml>$(ILLinkSharedDirectory)ILLink.Descriptors.LibraryBuild.xml</ILLinkDescriptorsLibraryBuildXml>

LibraryBuild directives are only used when building the repo. They will not root things when building/trimming end user apps.

Cc @eerhardt

[eerhardt]

VS4Mac uses reflection to get to and call it

Can we instead propose a public API for this? That is MUCH better than using private reflection.

[mikem8361]

VS4Mac uses reflection to get to and call it

Can we instead propose a public API for this? That is MUCH better than using private reflection.

I thought adding a new public API to a service release wasn't possible and this is temporary until we have time to design a public one. This needs to go into 6.0.1.

Or are you saying that if I make it public I won't need the ILLink.Descriptors.Shared.xml entry to keep it from being elided?

[mikem8361]

@noahfalk can you approve this 6.0 PR also? I'll make the same changes as the 7.0 one you reviewed.

[eerhardt]

I didn't notice initially that this is targeting 6.0. For 7.0 I think we should try to make a full public API instead of reflection.

Or are you saying that if I make it public I won't need the ILLink.Descriptors.Shared.xml entry to keep it from being elided?

I guess making it public in the implementation assembly, and leaving it out of the ref assembly would actually work. The linker wouldn't trim it during the repo build. But I think @MichalStrehovsky's suggestion above is better - to add the ILLink.Descriptors entry into this file:

runtime/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.LibraryBuild.xml

Lines 2 to 6 in 3dfdcb9

	<assembly fullname="System.Private.CoreLib">
	<type fullname="Interop/Globalization">
	<!-- Internal API used by tests only. -->
	<method name="GetICUVersion" />
	</type>

And also adding a comment to the entry stating why it is there.

See https://github.com/dotnet/runtime/blob/main/docs/workflow/trimming/ILLink-files.md#illinkdescriptorsxml for more information on how to use these ILLink files.

[jkotas]

Or are you saying that if I make it public I won't need the ILLink.Descriptors.Shared.xml entry to keep it from being elided?

Yes, you can make it public in CoreLib, without adding it to the reference assembly. You won't need to mess with the linker directives if you do it that way. Instead, you will need to suppress warning that complains about the implementation and reference assembly being out of sync.

[eerhardt]

Does it make sense / is it possible to add tests?

[jeffschwMSFT]

Approved. We should take this for consideration in 6.0.x

[mikem8361]

There is no easy way to test this API. I have something that only works on Linux using the native libunwind library to get IPs to check but it won't work on OSX (libunwind isn't discoverable and the simple unwind function doesn't exist) and Windows. And there is time constraints getting this into 6.0.x and upcoming vacations.

[jkotas]

Also, the eventual public API is going to have tests. We have discussed strategies to implement the tests.

[eerhardt]

ILLink changes look good to me.

[Anipik]

@mikem8361 is this ready to merge or do we need any review here ?

[jeffschwMSFT]

@Anipik let me follow-up with @tommcdon.

[mikem8361]

Jan: The API that we want to make public should have different shape (it should be StackFrame factory). It requires refactoring that would not appropriate for servicing, and that would not be possible to pull off in the few days that this have to be done in.

[jeffschwMSFT]

@mikem8361 have we looked at the failing CI? With that @Anipik we are ready to merge.

[mikem8361]

The failing CI all look like timeouts/existing infrastructure problems. I'm still waiting for some feedback from Kyle before we merge it.

[mikem8361]

We are ready to merge.