- Notifications
You must be signed in to change notification settings - Fork5.2k
[mono] Stop exporting ICU symbols from Mono#99449
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
rolfbjarne commentedMar 8, 2024
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. |
ivanpovazan commentedMar 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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: <PropertyGroupCondition="$(LocalBuild) == 'true'"> <RestoreAdditionalProjectSources>/Users/ivan/repos/runtime-mono/artifacts/packages/Release/Shipping</RestoreAdditionalProjectSources> </PropertyGroup> <ItemGroupCondition="$(LocalBuild) == 'true'"> <FrameworkReferenceUpdate="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) |
rolfbjarne commentedMar 8, 2024
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. |
ivanpovazan commentedMar 8, 2024
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 in80c0df6
and finally here: runtime/src/mono/mono/mini/driver.c Lines 2852 to 2865 in80c0df6
which sets the: runtime/src/mono/mono/mini/mini-trampolines.c Lines 1666 to 1677 ine7e3dee
Will have to set up debug builds of everything I suppose .. :) |
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
matouskozak commentedMar 8, 2024
/azp run runtime-extra-platforms |
| Azure Pipelines successfully started running 1 pipeline(s). |
matouskozak commentedMar 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This fix probably won't be enough. I was able to re-produce the original issue (#98941) without Xamarin by trying the Hybrid Globalization testshttps://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 |
rolfbjarne commentedMar 11, 2024
You can run |
akoeplinger commentedMar 11, 2024
The code in src/mono/mono/mini/CMakeLists.txtshould have fixed it but yeah check with nm. |
am11 commentedMar 15, 2024
I think#98495 has superseded this. |
vitek-karas commentedMar 15, 2024
@am11 - that's true for .NET 9, but this change is trying to fix this for .NET 8. |
steveisok left a comment
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 inUnmanagedCallersOnly and hide everything else.
matouskozak commentedMar 18, 2024
/azp run runtime-extra-platforms |
| Azure Pipelines successfully started running 1 pipeline(s). |
jeffschwMSFT left a comment
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
Uh oh!
There was an error while loading.Please reload this page.
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-lxso 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.dylibandlibSystem.Globalization.Native.dylibto 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