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

Incorrect IL is accepted by the Jit and leading to incorrect execution. #43342

Closed
sandreenko opened this issue Oct 13, 2020 · 2 comments · Fixed by #43386
Closed

Incorrect IL is accepted by the Jit and leading to incorrect execution. #43342

sandreenko opened this issue Oct 13, 2020 · 2 comments · Fixed by #43386
Assignees
Labels
arch-x86 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug JitUntriaged CLR JIT issues needing additional triage
Milestone

Comments

@sandreenko
Copy link
Contributor

sandreenko commented Oct 13, 2020

On x86 such IL is not rejected by our Jit:

        ldc.i4     1 // it goes to int64 arg
        ldc.i8     2 // it goes to int32 arg
        call       int32 GitHub_18295::Test(int64 a, int32 b)

but we actually compile it and end up mixing them and pass 1 in a register, 2 on the stack (because we don't track precise types nowadays).

so if inside Test we check the values we will see 1 in b and 2 in a.
This is not the correct behavior.

Full test
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

.assembly extern mscorlib { auto }
.assembly extern System.Console {auto}


.class private auto ansi beforefieldinit GitHub_18295
       extends [mscorlib]System.Object
{
    .method private hidebysig static int32 Test(int64 l, int32 i) cil managed
    {
        // Code size       36 (0x24)
        .maxstack  2
        .locals init (int32 V_0)
        IL_0000:  nop
        IL_0001:  ldarg.0
        IL_0002:  ldc.i8     1
        IL_000b:  ceq
        IL_000d:  call       void [System.Diagnostics.Debug]System.Diagnostics.Debug::Assert(bool)
        IL_0012:  nop
        IL_0013:  ldarg.1
        IL_0014:  ldc.i4.2
        IL_0015:  ceq
        IL_0017:  call       void [System.Diagnostics.Debug]System.Diagnostics.Debug::Assert(bool)
        IL_001c:  nop
        IL_001d:  ldc.i4.s   100
        IL_001f:  stloc.0
        IL_0020:  br.s       IL_0022
        IL_0022:  ldloc.0
        IL_0023:  ret

    }

    .method private hidebysig static int32 Main() cil managed
    {
        .entrypoint
        .vtentry 11 : 1
        // Code size       131 (0x83)
        .maxstack  4

        ldc.i4     1
        ldc.i8     2
        call       int32 GitHub_18295::Test(int64, int32)

        ldc.i4     100
        ret
    }
}

We have an example of such IL in

// if (Test(1,1) != 1) goto F1
ldc.i4 1
ldc.i8 1
call int32 GitHub_18295::Test(int64, int32)

that failed during my arm64 OSX work #43130.
In this test, we probably should just swap int8 and int4, but Jit should be changed to reject such IL as BADCODE?

Ecma III.1.6 Implicit argument coercion (page 305).

@dotnet/jit-contrib do you think it should be BADCODE or should we support it?

category:correctness
theme:importer
skill-level:intermediate
cost:medium

@sandreenko sandreenko added bug arch-x86 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Oct 13, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 13, 2020
@sandreenko sandreenko self-assigned this Oct 13, 2020
@CarolEidt
Copy link
Contributor

My opinion is that this is clearly illegal IL, and the only reason we'd consider accepting it would be for backward compatibility, but since it appears that the existing code generation isn't a reasonable interpretation of how to "correctly" handle the case, I think we should reject it.

@JulieLeeMSFT
Copy link
Member

@sandreenko making this as .NET 6.

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Oct 14, 2020
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Oct 14, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x86 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug JitUntriaged CLR JIT issues needing additional triage
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants