Skip to content

Fix invalid IL in RefFields test - remove ldind.ref on value type references #116416

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

Merged
merged 2 commits into from
Jun 8, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 8, 2025

This PR fixes the intermittent failure of the Loader/classloader/RefFields/Validate test that was causing assertion failures in methodtable.cpp with the error:

ASSERT FAILED
Expression: (GetComponentSize() <= 2) || IsArray()
Location:   line 6277 in /__w/1/s/src/coreclr/vm/methodtable.cpp
Function:   SanityCheck

Root Cause

The issue was in the WithRefStructField.ConfirmFieldInstance method in InvalidCSharp.il, which contained invalid IL:

.method public hidebysig
    instance bool ConfirmFieldInstance (
        valuetype InvalidCSharp.WithRefField&
    ) cil managed
{
    ldarg.0
    ldfld valuetype InvalidCSharp.WithRefField& InvalidCSharp.WithRefStructField::Field
    ldind.ref    // ❌ INVALID: ldind.ref on value type reference
    ldarg.1
    ldind.ref    // ❌ INVALID: ldind.ref on value type reference  
    ceq
    ret
}

The problem was using ldind.ref to dereference WithRefField& (reference to a value type). ldind.ref is only valid for dereferencing object references, not value type references. This caused the JIT to generate incorrect GC information, leading to the assertion failure.

Fix

The fix removes the invalid ldind.ref instructions and directly compares the two WithRefField& references:

.method public hidebysig
    instance bool ConfirmFieldInstance (
        valuetype InvalidCSharp.WithRefField&
    ) cil managed
{
    ldarg.0
    ldfld valuetype InvalidCSharp.WithRefField& InvalidCSharp.WithRefStructField::Field
    ldarg.1
    ceq
    ret
}

This correctly compares whether the stored reference in the field points to the same location as the parameter reference, which is the intended behavior of the test.

Verification

  • Verified the IL assembles correctly using ilasm
  • Confirmed the syntax follows proper IL patterns for reference comparison
  • The fix addresses the root cause of the intermittent GC-related crashes

Fixes #116358.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • k0ivsblobprodcus356.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/runtime/runtime/.dotnet/dotnet /home/REDACTED/work/runtime/runtime/.dotnet/sdk/10.0.100-preview.6.25272.112/NuGet.Build.Tasks.Console.dll Recursive=True;CleanupAssetsForUnsupportedProjects=True;DisableParallel=False;Force=False;ForceEvaluate=False;HideWarningsAndErrors=False;IgnoreFailedSources=False;Interactive=False;NoCache=False;NoHttpCache=False;RestorePackagesConfig=False /home/REDACTED/work/runtime/runtime/.dotnet/sdk/10.0.100-preview.6.25272.112/MSBuild.dll /home/REDACTED/work/runtime/runtime/src/tests/Loader/classloader/RefFields/Validate.csproj NuGetInteractive=false;MSBuildRestoreSessionId=21e5ac85-7729-4227-9df0-d7f3c485866d;MSBuildIsRestoring=True;ExcludeRestorePackageImports=True;OriginalMSBuildStartupDirectory=/home/REDACTED/work/runtime/runtime/src/tests/Loader/classloader/RefFields (dns block)
  • mfjvsblobprodcus373.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/runtime/runtime/.dotnet/dotnet /home/REDACTED/work/runtime/runtime/.dotnet/sdk/10.0.100-preview.6.25272.112/NuGet.Build.Tasks.Console.dll Recursive=True;CleanupAssetsForUnsupportedProjects=True;DisableParallel=False;Force=False;ForceEvaluate=False;HideWarningsAndErrors=False;IgnoreFailedSources=False;Interactive=False;NoCache=False;NoHttpCache=False;RestorePackagesConfig=False /home/REDACTED/work/runtime/runtime/.dotnet/sdk/10.0.100-preview.6.25272.112/MSBuild.dll /home/REDACTED/work/runtime/runtime/src/tests/Loader/classloader/RefFields/Validate.csproj NuGetInteractive=false;MSBuildRestoreSessionId=21e5ac85-7729-4227-9df0-d7f3c485866d;MSBuildIsRestoring=True;ExcludeRestorePackageImports=True;OriginalMSBuildStartupDirectory=/home/REDACTED/work/runtime/runtime/src/tests/Loader/classloader/RefFields (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@AaronRobinsonMSFT
Copy link
Member

@copilot The issue here is captured in the comment by @jkotas, #116358 (comment). The point of this test was to validate the targets of the byref are identical. However, the approach I took was incorrect for value types. Instead, the solution should be to simply compare the managed pointers instead of dereferencing them in IL. The easiest way to address this would be to remove both ldind.ref instructions in the method mentioned in the referenced comment above.

@AaronRobinsonMSFT AaronRobinsonMSFT added test-bug Problem in test source code (most likely) and removed linkable-framework Issues associated with delivering a linker friendly framework labels Jun 8, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 10.0.0 milestone Jun 8, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT removed the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 8, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT added area-System.Runtime.CompilerServices and removed linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 8, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT removed the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@jkotas jkotas removed the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
…erences

Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Loader/classloader/RefFields/Validate test failing intermittently Fix invalid IL in RefFields test - remove ldind.ref on value type references Jun 8, 2025
@Copilot Copilot AI requested a review from AaronRobinsonMSFT June 8, 2025 19:23
Copilot finished work on behalf of AaronRobinsonMSFT June 8, 2025 19:23
@jkotas jkotas marked this pull request as ready for review June 8, 2025 19:27
@jkotas jkotas enabled auto-merge (squash) June 8, 2025 19:27
@jkotas jkotas removed the linkable-framework Issues associated with delivering a linker friendly framework label Jun 8, 2025
Copy link
Contributor Author

Copilot AI commented Jun 8, 2025

@copilot The issue here is captured in the comment by @jkotas, #116358 (comment). The point of this test was to validate the targets of the byref ...

Done. Both ldind.ref instructions have been removed from the WithRefStructField.ConfirmFieldInstance method. The method now directly compares the managed pointers instead of dereferencing them, which is the correct approach for value type references. The IL assembles successfully without errors. (bb97cb7)

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • mfjvsblobprodcus373.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/runtime/runtime/.dotnet/dotnet /home/REDACTED/work/runtime/runtime/.dotnet/sdk/10.0.100-preview.6.25272.112/NuGet.Build.Tasks.Console.dll Recursive=True;CleanupAssetsForUnsupportedProjects=True;DisableParallel=False;Force=False;ForceEvaluate=False;HideWarningsAndErrors=False;IgnoreFailedSources=False;Interactive=False;NoCache=False;NoHttpCache=False;RestorePackagesConfig=False /home/REDACTED/work/runtime/runtime/.dotnet/sdk/10.0.100-preview.6.25272.112/MSBuild.dll /home/REDACTED/work/runtime/runtime/src/tests/Loader/classloader/RefFields/InvalidCSharp.ilproj NuGetInteractive=false;MSBuildRestoreSessionId=4becf198-a8ff-4047-b763-206cb6de1a72;MSBuildIsRestoring=True;ExcludeRestorePackageImports=True;OriginalMSBuildStartupDirectory=/home/REDACTED/work/runtime/runtime/src/tests/Loader/classloader/RefFields (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of AaronRobinsonMSFT June 8, 2025 19:46
@jkotas jkotas merged commit af399c4 into main Jun 8, 2025
79 checks passed
@jkotas jkotas deleted the copilot/fix-116358 branch June 8, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime.CompilerServices test-bug Problem in test source code (most likely)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loader/classloader/RefFields/Validate test failing intermittently
3 participants