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

[aot][browser] Stop throwing in CultureInfo getter #95910

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Dec 12, 2023

Fixes #94225. This workaround ensures HybridGlobalization works with AOT.

The logic in CultureInfo is following:

CultureInfo..cctor
    -> CultureData..cctor
    -> interp_runtime_invoke: >> method: System.Globalization.CalendarData:.cctor ()
    -> interp_runtime_invoke: >> method: System.Globalization.GlobalizationMode/Settings:.cctor ()
       -> Environment..cctor
       -> Marshal..cctor
       -> CompareInfo..cctor
            - tries to use CultureInfo.s_invariant .. which hasn't been initialized yet!

because of this flow, Debug.Assert(s_InvariantCultureInfo != null); is hit, causing a failure:

  • Encountered infinite recursion while looking up resource 'Arg_NullReferenceException' in System.Private.CoreLib when UseSystemResourceKeys=false OR
  • Error: TypeInitialization_Type, Microsoft.DotNet.XHarness.TestRunners.Common.ApplicationOptions (The type initializer for 'Microsoft.DotNet.XHarness.TestRunners.Common.ApplicationOptions' threw an exception. when UseSystemResourceKeys=true.

We could change the behavior of AOT on mono side but it would affect the performance negatively.
Original workaround approach:
We make sure that we don't create the method dependency loop and move the initialization of s_InvariantCultureInfo to the getter.
problems with this approach:
GlobalizationMode.Invarant is still locked in a cctor loop, so it fixes only part of the problem, connected with GlobalizationMode.Hybrid.

Changes in this PR:

  • Logging a message when the assert fails makes it easier to find the place with the assert in debug mode (we do not log code line).
  • The source of the problem was circular dependency on constructors. The circle can be broken by avoiding usage of CultureInfo in GlobalizationMode.Settings. Ordinal comparison of strings is under the hood a loop over these strings with == operator on corresponding characters. I do not see a reason why str.Equals("1", StringComparison.Ordinal) would be better than str == "1". If it exists, please ask for changes.

@ghost
Copy link

ghost commented Dec 12, 2023

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #94225. This workaround assures HybridGlobalization works with AOT.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization

Milestone: -

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm-libtests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm-libtests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm-libtests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm-libtests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

ilonatommy commented Dec 20, 2023

Failures are known.

Failures are same as original issue, but for Invariant.

@pavelsavara
Copy link
Member

I see

[10:49:54] info: Initializing dotnet version 9.0.0-ci commit hash 31f130654185256c6695a534f220807f0bddd680
[10:49:54] warn: [2023-12-19T10:49:54.948Z] [MONO] D:/a/_work/1/s/src/mono/mono/metadata/icall.c:6171 <disabled>
[10:49:54] warn: [2023-12-19T10:49:54.949Z] [MONO] D:/a/_work/1/s/src/mono/mono/metadata/icall.c:6180 <disabled>
[10:49:55] info: WASM EXIT 1

Log

It's non-Hybrid AOT of WasmTestOnBrowser-Invariant.Tests

Which is some FailFast. I wonder if it's the same or different than one being fixed here.
Or new one, because the AzDO thinks that the history of that test is green ?

@ilonatommy
Copy link
Member Author

ilonatommy commented Dec 20, 2023

After reverting my changes, running Invariant tests to reproduce the failures dotnet build -bl /t:Test src/libraries/System.Runtime/tests/System.Globalization.Tests/Invariant/Invariant.Tests.csproj /p:Configuration=Debug /p:TargetOS=browser /p:EnableAgressiveTrimming=true /p:RunAOTCompilation=true /p:BuildAOTTestsOnHelix=true:

[MONO] Process terminated.
[MONO] Encountered infinite recursion while looking up resource 'Word_At' in System.Private.CoreLib. Verify the installation of .NET is complete and does not need repairing, and that the state of the process has not become corrupted.

with SystmKeys
Process terminated. Assertion failed.
Looks same as original Hybrid failures. Most probably it's the same cause.

Confirmed, it is the same assert being hit. However, the fix for Hybrid does not work for Invariant, new CultureInfo(CultureData.Invariant, isReadOnly: true); returns with null even inside of the getter. I would make another issue for Invariant case and investigate it separately.

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm-libtests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy ilonatommy merged commit f58429e into dotnet:main Dec 20, 2023
202 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
5 participants