Repository navigation
Incorrect IL is accepted by the Jit and leading to incorrect execution. #43342
Description
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
runtime/src/tests/JIT/Regression/JitBlue/GitHub_18295/GitHub_18295.il
Lines 49 to 52 in 1d9b5e0
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
Metadata
Metadata
Assignees
Labels
Type
Projects
- Status
Activity
CarolEidt commentedon Oct 14, 2020
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 commentedon Oct 14, 2020
@sandreenko making this as .NET 6.