-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[mono] Stop exporting ICU symbols from Mono #99449
[mono] Stop exporting ICU symbols from Mono #99449
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
I wonder if the dylib needs to be signed? Do you get the same error if you copy:
An alternative might be to try in the x64 simulator. |
A note regarding E2E testing and:
We are getting this even if we build the runtime locally and configure the project file to reference the locally built runtime like: <PropertyGroup Condition="$(LocalBuild) == 'true'">
<RestoreAdditionalProjectSources>/Users/ivan/repos/runtime-mono/artifacts/packages/Release/Shipping</RestoreAdditionalProjectSources>
</PropertyGroup>
<ItemGroup Condition="$(LocalBuild) == 'true'">
<FrameworkReference Update="Microsoft.NETCore.App" RuntimeFrameworkVersion="8.0.3-dev" />
</ItemGroup> However, if we build the app and manually copy I remember @lambdageek experienced the same problem a while back: #93860 (comment) |
IIRC it's related to creating executable + writable memory (or failing to do so). You get the same crash on Mac Catalyst if you enable the sandbox, but don't add the jit entitlement. |
If I understand the flow correctly, we shouldn't be getting to the For the app we generate:
which should be invoked from xamarin's main: that further gets here: runtime/src/mono/mono/mini/driver.c Lines 2910 to 2932 in 80c0df6
and finally here: runtime/src/mono/mono/mini/driver.c Lines 2852 to 2865 in 80c0df6
which sets the: runtime/src/mono/mono/mini/mini-trampolines.c Lines 1666 to 1677 in e7e3dee
Will have to set up debug builds of everything I suppose .. :) |
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
This fix probably won't be enough. I was able to re-produce the original issue (#98941) without Xamarin by trying the Hybrid Globalization tests https://github.com/dotnet/runtime/blob/release/8.0-staging/src/libraries/System.Globalization/tests/Hybrid/System.Globalization.IOS.Tests.csproj. The error message is the same
and it also only fails on the newest iOS simulator (17.2 works fine) + it works on the .NET 9 main. However, it doesn't get fixed by using this PR. Do you know if we possibly export ICU symbols somewhere else in the Mono Runtime? @akoeplinger @mkhamoyan |
You can run |
The code in src/mono/mono/mini/CMakeLists.txt should have fixed it but yeah check with nm. |
I think #98495 has superseded this. |
@am11 - that's true for .NET 9, but this change is trying to fix this for .NET 8. |
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.
LGTM! @matouskozak library mode should be ok because we only export symbols declared in UnmanagedCallersOnly
and hide everything else.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
approved. we will take for consideration in 8.0.x
afb3fca
into
dotnet:release/8.0-staging
Customer Impact
When running against the latest XCode, the iOS 17.4 simulator causes our apps to crash due to a symbols clash between their version of ICU and our version. Prior to this change, by default we had been exporting all our symbols and been lucky due to the ICU versions matching. To fix, we hide our symbols via
-hidden-lx
so that there is no longer a clash.NOTE: This does not impact .NET 9 as we have moved away from shipping our own version of ICU on iOS.
Regression
Testing
We verified the fix thru manually copying new versions of
libmonosgen-2.0.dylib
andlibSystem.Globalization.Native.dylib
to previously crashing app and re-running it on the simulator. Currently, we are unable to verify this E2E by using a template iOS app with custom runtime due to issues with trampolines.Risk
Low, but since we were unable to verify E2E with a template app, there may be more churn required if the iOS SDK team finds any issues.
Contributed: @ivanpovazan
fyi: @vitek-karas @akoeplinger @steveisok @dalexsoto @mkhamoyan