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

Prefix exported symbols via UnmanagedCallersOnly attribute #79880

Merged
merged 4 commits into from
Jan 19, 2023

Conversation

ivanpovazan
Copy link
Member

@ivanpovazan ivanpovazan commented Dec 21, 2022

This PR includes the following:

  • All symbols exported via EntryPoint of UnmanagedCallersOnlyAttribute will be prefixed with a required _ on Apple platforms (as it is expected by the linker and dlsym)
  • All symbols exported via undocumented (and legacy) ExportSymbol of MonoPInvokeCallbackAttribute will not be prefixed (in order to prevent breaking existing code)
  • The change has been tested with the added functional test for iOS platforms

Fixes: #79491

/cc: @steveisok @mdh1418 @lateralusX

@ivanpovazan
Copy link
Member Author

I have two questions regarding this change:

  1. Should this PR also cover handling of: https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/aot-compiler.c#L5203-L5212
  2. Should we add a functional test for apple devices which is testing this fix and the [mono] Export unmanagedcallersonly method symbols #79424

@steveisok
Copy link
Member

  1. Should we add a functional test for apple devices which is testing this fix and the [mono] Export unmanagedcallersonly method symbols #79424

Since iOS/tvOS doesn't run on PR's, it would be better to add a test for osx.

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

  1. Should we add a functional test for apple devices which is testing this fix and the [mono] Export unmanagedcallersonly method symbols #79424

Since iOS/tvOS doesn't run on PR's, it would be better to add a test for osx.

@steveisok, regarding your comment:

Afaik calling exported symbols is supposed to work only in full AOT mode, which makes the testing of this feature really specific.
I was thinking of adding it to the src/tests/Interop/UnmanagedCallersOnly runtime tests, but excluding its execution on other platforms or including it only in full AOT mode seems a bit impractical.
Instead I have added a functional test case for this purpose.

Does that make sense?


Additionally, with this change I also had to adapt: https://github.com/ivanpovazan/runtime/blob/unmanagedcallersonly-mangling/src/mono/msbuild/apple/build/AppleApp.targets#L143
to enable functional test to have a custom native main.
I can create a separate PR if you agree with this change.

@ivanpovazan
Copy link
Member Author

Failures are unrelated

@steveisok
Copy link
Member

Does that make sense?

Additionally, with this change I also had to adapt: https://github.com/ivanpovazan/runtime/blob/unmanagedcallersonly-mangling/src/mono/msbuild/apple/build/AppleApp.targets#L143 to enable functional test to have a custom native main. I can create a separate PR if you agree with this change.

Yeah, you're right in that we don't run many full aot legs. I'm fine with it.

It looks like the tvOS run timed out.

@ivanpovazan
Copy link
Member Author

Created a separate PR to enable specifying custom native main for functional tests: #80546

@ivanpovazan ivanpovazan force-pushed the unmanagedcallersonly-mangling branch from 7dd0df2 to b76425a Compare January 13, 2023 09:51
@ivanpovazan
Copy link
Member Author

Mentioning: xamarin/xamarin-macios#10470 for a reference

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpovazan ivanpovazan changed the title Prefix exported symbols defined via EntryPoint of UnmanagedCallersOnl… Prefix exported symbols via UnmanagedCallersOnly attribute Jan 19, 2023
@ivanpovazan ivanpovazan merged commit f429780 into dotnet:main Jan 19, 2023
@ivanpovazan ivanpovazan deleted the unmanagedcallersonly-mangling branch February 1, 2023 08:37
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2023
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.

Mono AOT compiler does not prefix exported UnmanagedCallersOnly symbols with underscore when targeting iOS
4 participants