-
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
Align up structs to IntPtr.Size if they have gc pointers #100289
Conversation
src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
Outdated
Show resolved
Hide resolved
...ystem.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.StructLayoutAttribute.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
Show resolved
Hide resolved
…fix-structsize-vm
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Probably needs #100618 since the GC.* trick is not reliable to get object size in tests. Otherwise should be ready. The rules turn out to be a bit complex, e.g. [StructLayout(LayoutKind.Sequential, Size = 1000)]
class ClassA
{
long A;
}
[StructLayout(LayoutKind.Explicit, Size = 1000)]
class ClassB
{
[FieldOffset(0)]
long A;
} Here, CoreCLR allocates 1016 bytes for an instance of
So for classes, the only case when we respect Questionable behavior in CoreCLR (Main) for (boxed) object size:
cc @jkotas |
I do not think it would help you. This API gives you the exact same result as |
} | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] | ||
private static int GetManagedSize(Func<object> allocator) |
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.
This is not reliable as you have pointed out.
I think a reliable version of this check would be to inherit from the type you want to check, add a byte field, and verify offset of the byte field.
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.
Would an UnsafeAccessor
to RuntimeHelpers.GetRawObjectDataSize
work better here instead maybe?
EDIT: Ah wait, that's a static class, nevermind.
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.
Invoked GetRawObjectDataSize
via reflection
/azp run runtime-coreclr r2r |
Azure Pipelines successfully started running 1 pipeline(s). |
// Also, Mono needs RuntimeHelpers.GetRawObjectDataSize or equivalent to get an object size | ||
public static int TestEntryPoint() | ||
{ | ||
if (IntPtr.Size == 8) |
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.
Should this maybe not have the if and multiply the pointer size appropriately in each case?
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 prefer expected value to be a constant rather than computed expression for Asserts
@jkotas anything else here? looks like R2R and NAOT outerloops passed |
The maui team is waiting for a resolution on this |
I assume Jan is OOF, so maybe @MichalStrehovsky can review this? |
There is a reason I moved this to crossgen2 team (#100220 (comment)). Type layout needs to exactly match CoreCLR type layout for ReadyToRun to work. It has been a bug farm in the past. I don't see anything obviously wrong in this PR but that's not a high quality bar. I know we have modes of operation of crossgen2 where it serializes type layout into the image and validates it at runtime, but I don't know what CI legs that runs in and if this would cover it too. Can you get @davidwrighton to have a look? I really only have surface-level knowledge of R2R testing and very much hope I'm not the biggest expert. |
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.
LGTM. Thank you for adding a lot of tests!
@lewing do we need to backport the fix to a specific release preview branch? if so, which? |
Test introduced in dotnet#100289 failed the build of CLR tests for e.g. RISC-V: ``` /runtime/src/tests/Loader/classloader/explicitlayout/Regressions/100220/Runtime_100220.cs(15,23): error XUW1002: Tests should not unconditionally return 100. Convert to a void return. ```
Test introduced in #100289 failed the build of CLR tests for e.g. RISC-V: ``` /runtime/src/tests/Loader/classloader/explicitlayout/Regressions/100220/Runtime_100220.cs(15,23): error XUW1002: Tests should not unconditionally return 100. Convert to a void return. ```
Test introduced in dotnet#100289 failed the build of CLR tests for e.g. RISC-V: ``` /runtime/src/tests/Loader/classloader/explicitlayout/Regressions/100220/Runtime_100220.cs(15,23): error XUW1002: Tests should not unconditionally return 100. Convert to a void return. ```
Fixes #100220
I am not an expert in R2R/NAOT type system, but looks like it fixes the issue