-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Numerous GlobalizationNative P/Invokes in System.Private.CoreLib.dll don't have a corresponding native function anymore #96251
Comments
Tagging subscribers to this area: @dotnet/area-system-globalization Issue DetailsDescriptionI've looked through the P/Invokes in System.Private.CoreLib.dll, and these don't have a corresponding native entry poin in libSystem.Globalization.Native.a:
Tested with: 9.0.0-alpha.1.23619.7 This started happening in this maestro bump: xamarin/xamarin-macios#19690, which has this range of commits: c282395...6f4f268, and looking through the commits I'm guessing this is the culprit: #93220 CC @mkhamoyan Reproduction StepsDo something like this: cd packages/microsoft.netcore.app.runtime.mono.ios-arm64/9.0.0-alpha.1.23619.7/runtimes/ios-arm64/native
for symbol in $(monodis System.Private.CoreLib.dll | grep 'pinvokeimpl.*libSystem.Globalization.Native' | sed -e 's/.*as "//' -e 's/".*//'); do
if ! nm libSystem.Globalization.Native.a|grep "_$symbol\$">/dev/null; then
echo "NOT FOUND: $symbol"
fi
done Expected behaviorNo symbols should be reported. Actual behavior
Regression?Yes. Known WorkaroundsNone - note that this is breaking our build, so we can't take any maestro bumps until this is fixed. ConfigurationNo response Other informationNo response
|
@rolfbjarne yes it is related to #93220 . |
@mkhamoyan that doesn't change anything, our build fails in the same way:
which happens because there are P/Invokes to native functions that don't exist anymore. AFAIK setting HybridGlobalization=true won't change anything unless the trimmer runs, and that doesn't always happen. |
@rolfbjarne we have new set of native functions that should be P/Invoked on hybrid mode https://github.com/dotnet/runtime/blob/main/src/native/libs/System.Globalization.Native/entrypoints.c#L60-#L81 , trimmer run shouldn't be related to hybrid mode. |
@rolfbjarne can we try defining in these section https://github.com/xamarin/xamarin-macios/blob/2bd23381e64494f4dbdcfcd71d161365d111a780/runtime/Makefile#L640 targets |
@mkhamoyan not sure what that would accomplish? We're not compiling runtime code, we're just consuming your already built static/dynamic libraries, so definining those symbols won't change anything on our side. In other words: those are linker flags, not compiler flags. |
Created #96270 for target definitions, once it will be merged we can check for xamarin-macios . |
You can run |
The thing is I cannot reproduce same error in |
Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos Issue DetailsDescriptionI've looked through the P/Invokes in System.Private.CoreLib.dll, and these don't have a corresponding native entry point in libSystem.Globalization.Native.a:
Tested with: 9.0.0-alpha.1.23619.7 This started happening in this maestro bump: xamarin/xamarin-macios#19690, which has this range of commits: c282395...6f4f268, and looking through the commits I'm guessing this is the culprit: #93220 CC @mkhamoyan Reproduction StepsDo something like this: cd packages/microsoft.netcore.app.runtime.mono.ios-arm64/9.0.0-alpha.1.23619.7/runtimes/ios-arm64/native
for symbol in $(monodis System.Private.CoreLib.dll | grep 'pinvokeimpl.*libSystem.Globalization.Native' | sed -e 's/.*as "//' -e 's/".*//'); do
if ! nm libSystem.Globalization.Native.a|grep "_$symbol\$">/dev/null; then
echo "NOT FOUND: $symbol"
fi
done Expected behaviorNo symbols should be reported. Actual behavior
Regression?Yes. Known WorkaroundsNone - note that this is breaking our build, so we can't take any maestro bumps until this is fixed. ConfigurationNo response Other informationNo response
|
@mkhamoyan any news about this? More and more people are running into this by installing a newer .NET 9 build. |
@rolfbjarne #96270 is already merged. Can we update xamarin/xamarin-macios#19690 to see if that helps? |
We'll get it once it flows through maestro (looks like it's stuck here at the moment: dotnet/installer#18133) |
@rolfbjarne @mkhamoyan I suspect the change is not going to help. @rolfbjarne I think the way the pinvokes are validated needs to change. We are shipping with hybrid globalization always on, so there's a smaller subset of pinvokes actually used. We don't want to trim things out because we plan on adding the ability to turn HG off and use our ICU in a follow up change. |
It didn't.
We're not actually validating, this is a consequence of:
In this case we need to tell the native linker which native functions to keep, so we collect all the P/Invokes (whose target library is a static library), and ask the native linker to keep those. The problem is if there are P/Invokes to native functions that don't exist - in which case the native linker will complain and show an error. Technically we could stop asking the native linker to trim unused code, but it would turn into a size regression (potentially quite big when using third-party native libraries). |
I build dotnet/runtime like this: $ ./build.sh mono+libs -os ios -arch arm64 and then I checked that $ monodis ./artifacts/bin/microsoft.netcore.app.runtime.ios-arm64/Debug/runtimes/ios-arm64/native/System.Private.CoreLib.dll | grep GlobalizationNative_CloseSortHandle
.method assembly static hidebysig pinvokeimpl ("libSystem.Globalization.Native" as "GlobalizationNative_CloseSortHandle" winapi nomangle ) but $ nm ./artifacts/bin/microsoft.netcore.app.runtime.ios-arm64/Debug/runtimes/ios-arm64/native/libSystem.Globalization.Native.a | grep GlobalizationNative_CloseSortHandle
[ no output ] |
runtime/src/native/libs/System.Globalization.Native/entrypoints.c Lines 60 to 85 in 1d5f483
If there is some validation between |
I think a linker substitution around the hybrid globalization checks may set SPC right. Assuming I'm correct, @rolfbjarne that is something you would have to do. |
What @rolfbjarne is pointing out is that libSystem.Globalization.Native.a does not have GlobalizationNative_CloseSortHandle symbol. It does not matter if hybrid globalization is on or off because that native symbol is never available but it's referenced by managed code. |
We can't rely on the linker, because it doesn't always run. |
Got it. I think that might mean we would need to ship two different runtimes. If that's the case, I would rather not support our ICU any longer and adjust the pinvokes from SPC so that they match. |
I'm not quite sure I understand the problem. Is there any reason you can't remove the P/Invokes from An alternative solution is to add placeholder/empty native symbols, so that the native linker finds something. |
Sorry, I'm mashing up what needs to come after to support HG as the default and being able to flip back to our ICU. It's clear what we need to do to fix this. |
The last PR fixed the issue with regards to the disconnect between native functions and P/Invokes. However, the build problem isn't fully resolved yet, it fails with this now:
These symbols used to be provided by |
We ship |
There's no
|
My mistake, we don't ship the lib but use the one from Apple sdks, like |
Ah, ok, I made it work that way, so this is fixed now then. |
Description
I've looked through the P/Invokes in System.Private.CoreLib.dll, and these don't have a corresponding native entry point in libSystem.Globalization.Native.a:
Tested with: 9.0.0-alpha.1.23619.7
This started happening in this maestro bump: xamarin/xamarin-macios#19690, which has this range of commits: c282395...6f4f268, and looking through the commits I'm guessing this is the culprit: #93220
CC @mkhamoyan
Reproduction Steps
Do something like this:
Expected behavior
No symbols should be reported.
Actual behavior
Regression?
Yes.
Known Workarounds
None - note that this is breaking our build, so we can't take any maestro bumps until this is fixed.
Configuration
No response
Other information
No response
The text was updated successfully, but these errors were encountered: