-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
TypedByReference should be treated as a normal valuetype for calling convention purposes #71675
Conversation
@AaronRobinsonMSFT tagging you as this may have an impact on ref structures, and places we'd like to test. |
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<CLRTestPriority>0</CLRTestPriority> |
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.
Change to Pri1 before checkin, but leave as Pri0 for initial test pass to make sure that this works on all architectures and OSs.
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
using System; | ||
using System.Runtime.CompilerServices; |
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.
I don't think this test is appropriate. The github number thing here makes sense with the JIT tests, but not with defined behaviors. I'd prefer to see this test added to an existing collection for TypedReference
under src/tests/Loader/classloader
or, since this is pure managed, in TypedReferenceTests.cs
.
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.
Reasonable. I originally put the test here, as I guessed it was weird optimizer thing.... as it was only reproducing on Linux then I looked a bit deeper.
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.
Instead of moving this, I'd also be fine with clearly documenting in the test the state we are validating here.
The previous logic would cause the runtime to compute an incorrect size for a TypedByReference argument, and then only shuffle a single register of data instead of two.
Fixes #42732