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

[tests] Fix recently added mono test #110682

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Dec 13, 2024

There are two issues with the test. Firstly, the BigArray_x_2 structs were expected to throw type load exception in the test, due to size limit, but their size was decremented by one instead of incremented. Secondly, inlining of Environment.Is64BitProcess fails due to class initialization not yet being performed. This means that we would end up initializing both 32bit and 64bit types in the test.

This test has been failing on mono ever since it was added in #104393. It seems to have gone undercover because it is not being run as apart of normal desktop runtime tests due to incorrect configuration where it is removed in Loader.csproj. This hack was apparently done because disabling the test on coreclr hits a separate issue #105964.

Test was consistently failing on mobile where we use a different mechanism for running runtime tests.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 13, 2024
@BrzVlad BrzVlad requested a review from fanyang-mono December 13, 2024 09:04
@BrzVlad
Copy link
Member Author

BrzVlad commented Dec 13, 2024

#110149. CC @matouskozak @kotlarmilos

@matouskozak
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matouskozak matouskozak added area-VM-meta-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 16, 2024
Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, I started extra-platforms to make sure it fixes the issue on CI as well.

For refence, here is the original issue #106797 after #104393 got merged. I think it got superseded later by #110149.

{
if (Environment.Is64BitProcess)
{
Test64Bit ();
Copy link
Member

Choose a reason for hiding this comment

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

Why does moving the code to separate subroutine helps with:

Secondly, inlining of Environment.Is64BitProcess fails due to class initialization not yet being performed.

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider code like:

if (true)
    print sizeof (T1)
else
    print sizeof (T2)

sizeof (T1) needs to run some class initialization before being computed. Also the size gets included in the emitted code. In this scenario, during method compilation, sizeof (T1) will be computed and we won't emit any code in the else branch. However if the if condition is not obviously known at method compile time, then both branches will be compiled and we would end up running initialization for T1 as well as for T2. This is not about resolving the condition better, but rather preventing the initialization of both types.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, thanks. Everything can be solved by another layer of indirection :)

However, thinking about it more, why would if (Environment.Is64BitProcess) (which is just IntPtr.Size == 8) be unknown, especially during AOT. Maybe we are not optimizing the compilation and always emitting code for both branches and discarding the else branch later?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is not the case. We don't inline any methods from classes that have a cctor that hasn't been run yet. In this particular case, the cctor wouldn't have any consequence on the result.

How does CoreCLR handle Environment.Is64BitProcess for example ? Does it rely on the tiered method to have the optimized code generated while the untiered method doesn't hardcode the value ? @AndyAyersMS

Copy link
Member

Choose a reason for hiding this comment

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

How does CoreCLR handle Environment.Is64BitProcess for example ? Does it rely on the tiered method to have the optimized code generated while the untiered method doesn't hardcode the value ? @AndyAyersMS

I don't see any special treatment (eg intrisic attribute) in CoreCLR for Is64BitProcess, so it will be a call in Tier0 and inlined / folded to a constant in Tier1.

@BrzVlad BrzVlad merged commit e9c95f4 into dotnet:main Dec 16, 2024
97 of 106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono] Runtime tests LargeStructSize_Mono and xprecise1_cs_do failing on Apple mobile
3 participants