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

Corrupted function parameter value in Release configuration (overwritten by the first argument value). #96306

Closed
GeneralGDA opened this issue Dec 25, 2023 · 4 comments · Fixed by #96439
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@GeneralGDA
Copy link

Description

After several calls function argument gets corrupted: it's value gets overwritten by the first field of the first argument.

Reproduction Steps

Compile and run next code in release (the 'scale' parameter of the CreateInclineOrIdentity will swap to 0 at some point):

using System.Numerics;

namespace JitBugReproduce;

internal class Program
{
    static void Main()
    {
        for (int j = 0; j < 10000; j++)
        {
            CreateInclineOrIdentity(new(), new(), new(), 42);
        }
    }

    public static void CreateInclineOrIdentity(Point2D fixedStart, Point2D fixedEnd, Point2D scalingDirection, float scale)
    {
        Foo();
        PrintSuspiciousArgument(scale); // just print the scale's value
    }

    internal static void PrintSuspiciousArgument(float probe)
    {
        Console.Out.Write(probe);
        Console.Out.Write(' ');
    }

    [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
    public static void Foo() { }

    public readonly struct Point2D
    {
        private readonly Vector2 _host;
    }
}

The project configuration:

[JitFailure.zip](https://github.com/dotnet/runtime/files/13764096/JitFailure.zip)

I've attached the whole project.

Expected behavior

The console full of 42 value.

Actual behavior

A span of zeroes in the console.

Regression?

No response

Known Workarounds

Mark corrupted parameter with the 'in' keyword.

Configuration

Windows 10 (10.0.19045.3803/22H2/2022Update)
AMD Ryzen 7 5800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2

Other information

The problem could be seen in the assembly. This is the function call site:

mov rcx,[rsp+30] ; 1-st argument (Point2D struct)
mov rdx,[rsp+28]; 2-nd argument (Point2D struct)
mov r8,[rsp+20]; 3-nd argument (Point2D struct)
vmovss xmm3,dword ptr [rax+4]; 4-th argument, a float
add rsp,38
jmp qword ptr [7FF8390FE910]; JitBugReproduce.AlongAxisScaling.CreateInclineOrIdentity(JitBugReproduce.Point2D, JitBugReproduce.Point2D, JitBugReproduce.Vector2D, Single)

And here is the function code being JITtered serveral times:

; JitBugReproduce.AlongAxisScaling.CreateInclineOrIdentity(JitBugReproduce.Point2D, JitBugReproduce.Point2D, JitBugReproduce.Vector2D, Single)
; var fixedLine = new Line2D(fixedStart, fixedEnd - fixedStart, FloatExtensions.DefaultEpsilon);
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
; PrintSuspiciousArgument(scale); // just print the scale's value
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
; return Identity;
; ^^^^^^^^^^^^^^^^
sub rsp,48
vzeroupper
vmovq xmm3,rcx; here the float value is overwritten by the first argument data
vmovq xmm1,rdx
vmovq xmm2,r8
vmovss dword ptr [rsp+68],xmm3

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 25, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 25, 2023
@ghost
Copy link

ghost commented Dec 25, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

After several calls function argument gets corrupted: it's value gets overwritten by the first field of the first argument.

Reproduction Steps

Compile and run next code in release (the 'scale' parameter of the CreateInclineOrIdentity will swap to 0 at some point):

using System.Numerics;

namespace JitBugReproduce;

internal class Program
{
    static void Main()
    {
        for (int j = 0; j < 10000; j++)
        {
            CreateInclineOrIdentity(new(), new(), new(), 42);
        }
    }

    public static void CreateInclineOrIdentity(Point2D fixedStart, Point2D fixedEnd, Point2D scalingDirection, float scale)
    {
        Foo();
        PrintSuspiciousArgument(scale); // just print the scale's value
    }

    internal static void PrintSuspiciousArgument(float probe)
    {
        Console.Out.Write(probe);
        Console.Out.Write(' ');
    }

    [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
    public static void Foo() { }

    public readonly struct Point2D
    {
        private readonly Vector2 _host;
    }
}

The project configuration:

[JitFailure.zip](https://github.com/dotnet/runtime/files/13764096/JitFailure.zip)

I've attached the whole project.

Expected behavior

The console full of 42 value.

Actual behavior

A span of zeroes in the console.

Regression?

No response

Known Workarounds

Mark corrupted parameter with the 'in' keyword.

Configuration

Windows 10 (10.0.19045.3803/22H2/2022Update)
AMD Ryzen 7 5800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2

Other information

The problem could be seen in the assembly. This is the function call site:

mov rcx,[rsp+30] ; 1-st argument (Point2D struct)
mov rdx,[rsp+28]; 2-nd argument (Point2D struct)
mov r8,[rsp+20]; 3-nd argument (Point2D struct)
vmovss xmm3,dword ptr [rax+4]; 4-th argument, a float
add rsp,38
jmp qword ptr [7FF8390FE910]; JitBugReproduce.AlongAxisScaling.CreateInclineOrIdentity(JitBugReproduce.Point2D, JitBugReproduce.Point2D, JitBugReproduce.Vector2D, Single)

And here is the function code being JITtered serveral times:

; JitBugReproduce.AlongAxisScaling.CreateInclineOrIdentity(JitBugReproduce.Point2D, JitBugReproduce.Point2D, JitBugReproduce.Vector2D, Single)
; var fixedLine = new Line2D(fixedStart, fixedEnd - fixedStart, FloatExtensions.DefaultEpsilon);
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
; PrintSuspiciousArgument(scale); // just print the scale's value
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
; return Identity;
; ^^^^^^^^^^^^^^^^
sub rsp,48
vzeroupper
vmovq xmm3,rcx; here the float value is overwritten by the first argument data
vmovq xmm1,rdx
vmovq xmm2,r8
vmovss dword ptr [rsp+68],xmm3

Author: GeneralGDA
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@huoyaoyuan
Copy link
Member

Doesn't reproduce in 7.0.14. Should be regression in 8.0.

@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Dec 25, 2023
@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Dec 25, 2023
@jakobbotsch
Copy link
Member

It looks like a bug in genFnPrologCalleeRegArgs and its handling of promoted structs with SIMD fields. In .NET 8 we started more consistently promoting such structs, but it's not really a regression, the issue reproduces also in .NET 7 if the function satisfies the necessary conditions for SIMD promotion. For example,

     public static void CreateInclineOrIdentity(Point2D fixedStart, Point2D fixedEnd, Point2D scalingDirection, float scale)
     {
         Foo();
         PrintSuspiciousArgument(scale); // just print the scale's value
+        GC.KeepAlive(new Vector2().X);
     }

causes the issue to reproduce in .NET 7 as well.

I can take a look (when I'm back from holiday).

@jakobbotsch jakobbotsch self-assigned this Dec 25, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jan 3, 2024
Parameters that are going into float registers can come from integer
registers in the presence of struct promotion. We need to home those
before integer parameters or the source register could have been
overridden by the integer parameter homing logic.

Ideally it seems like the homing logic should be unified to handle all
parameters simultaneously, but this seems like a simple enough fix. I do
not think we have ABIs where we have the opposite kind constraint
(integer parameters coming from float registers).

Fix dotnet#96306
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 3, 2024
jakobbotsch added a commit that referenced this issue Jan 5, 2024
Parameters that are going into float registers can come from integer
registers in the presence of struct promotion. We need to home those
before integer parameters or the source register could have been
overridden by the integer parameter homing logic.

Ideally it seems like the homing logic should be unified to handle all
parameters simultaneously, but this seems like a simple enough fix. I do
not think we have ABIs where we have the opposite kind constraint
(integer parameters coming from float registers).

Fix #96306
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 5, 2024
@jakobbotsch
Copy link
Member

@GeneralGDA #96439 has a fix for the problem in .NET 9. If this is blocking scenarios for you in .NET 8 and the workaround is not sufficient then I can also backport the fix to .NET 8. Please let me know if you'd like to request a backport.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2024
github-actions bot pushed a commit that referenced this issue Feb 21, 2024
Parameters that are going into float registers can come from integer
registers in the presence of struct promotion. We need to home those
before integer parameters or the source register could have been
overridden by the integer parameter homing logic.

Ideally it seems like the homing logic should be unified to handle all
parameters simultaneously, but this seems like a simple enough fix. I do
not think we have ABIs where we have the opposite kind constraint
(integer parameters coming from float registers).

Fix #96306
carlossanlop pushed a commit that referenced this issue Mar 11, 2024
…eters (#98749)

* JIT: Home float parameters before integer parameters

Parameters that are going into float registers can come from integer
registers in the presence of struct promotion. We need to home those
before integer parameters or the source register could have been
overridden by the integer parameter homing logic.

Ideally it seems like the homing logic should be unified to handle all
parameters simultaneously, but this seems like a simple enough fix. I do
not think we have ABIs where we have the opposite kind constraint
(integer parameters coming from float registers).

Fix #96306

* Add test

* Disable float -> int reg enregistration for some rare cases

---------

Co-authored-by: Jakob Botsch Nielsen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants