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

[NativeAOT-LLVM] Stub out hybrid globalization #2337

Merged
merged 3 commits into from
Jul 10, 2023

Conversation

SingleAccretion
Copy link

@SingleAccretion SingleAccretion commented Jul 7, 2023

We do not have the bandwidth to support it properly.

While ostensibly the upstream work could be reused, practically it is too tied to the Mono runtime internals.

Upstream ref: #79989. See also https://github.com/dotnet/runtime/blob/main/docs/design/features/globalization-hybrid-mode.md.

This will fix the (harmless) warnings about the missing internal calls we've been getting:

  ILC: Method '[S.P.CoreLib]Interop+JsGlobalization.CompareString(string&,char*,int32,char*,int32,CompareOptions,int32&,object&)' will always throw because: Invalid IL or CLR metadata in 'Int
  32 JsGlobalization.CompareString(System.String ByRef, Char*, Int32, Char*, Int32, System.Globalization.CompareOptions, Int32 ByRef, System.Object ByRef)'
  ILC: Method '[S.P.CoreLib]Interop+JsGlobalization.StartsWith(string&,char*,int32,char*,int32,CompareOptions,int32&,object&)' will always throw because: Invalid IL or CLR metadata in 'Boolea
  n JsGlobalization.StartsWith(System.String ByRef, Char*, Int32, Char*, Int32, System.Globalization.CompareOptions, Int32 ByRef, System.Object ByRef)'
  ILC: Method '[S.P.CoreLib]Interop+JsGlobalization.ChangeCaseInvariant(char*,int32,char*,int32,bool,int32&,object&)' will always throw because: Invalid IL or CLR metadata in 'Void JsGlobaliz
  ation.ChangeCaseInvariant(Char*, Int32, Char*, Int32, Boolean, Int32 ByRef, System.Object ByRef)'
  ILC: Method '[S.P.CoreLib]Interop+JsGlobalization.ChangeCase(string&,char*,int32,char*,int32,bool,int32&,object&)' will always throw because: Invalid IL or CLR metadata in 'Void JsGlobaliza
  tion.ChangeCase(System.String ByRef, Char*, Int32, Char*, Int32, Boolean, Int32 ByRef, System.Object ByRef)'
  ILC: Method '[S.P.CoreLib]Interop+JsGlobalization.EndsWith(string&,char*,int32,char*,int32,CompareOptions,int32&,object&)' will always throw because: Invalid IL or CLR metadata in 'Boolean
  JsGlobalization.EndsWith(System.String ByRef, Char*, Int32, Char*, Int32, System.Globalization.CompareOptions, Int32 ByRef, System.Object ByRef)'
  ILC: Method '[S.P.CoreLib]Interop+JsGlobalization.IndexOf(string&,char*,int32,char*,int32,CompareOptions,bool,int32&,object&)' will always throw because: Invalid IL or CLR metadata in 'Int3
  2 JsGlobalization.IndexOf(System.String ByRef, Char*, Int32, Char*, Int32, System.Globalization.CompareOptions, Boolean, Int32 ByRef, System.Object ByRef)'

We do not have the bandwidth to support it properly.

While ostensibly the upstream work could be reused, practically it is too tied to the Mono runtime internals.
@SingleAccretion
Copy link
Author

@dotnet/nativeaot-llvm

@jkotas
Copy link
Member

jkotas commented Jul 8, 2023

Would forcing GlobalizationMode.Hybrid to false using ILLinker substitutions work better?

@SingleAccretion
Copy link
Author

Would forcing GlobalizationMode.Hybrid to false using ILLinker substitutions work better?

I checked and it does not because the compiler still gets to see the invalid internal calls when not eliminating dead code (empirically, with -c Debug).

@jkotas
Copy link
Member

jkotas commented Jul 10, 2023

It is really the case? I thought that FeatureSwitchManager kicks in unconditionally, independent on the optimizations.

@SingleAccretion
Copy link
Author

SingleAccretion commented Jul 10, 2023

It is really the case? I thought that FeatureSwitchManager kicks in unconditionally, independent on the optimizations.

It does indeed. There was a certain amount of operator error: I substituted the inner Settings.Hybrid value, which did not work in Debug, but did in Release. Substituting GlobalizationMode.Hybrid works fine both ways. Have updated the change accordingly.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 5d79481 into dotnet:feature/NativeAOT-LLVM Jul 10, 2023
@SingleAccretion SingleAccretion deleted the Hybrid-Glob branch July 14, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants