Skip to content

Commit 9833f60

Browse files
davidwrightondavidwrighton
authored
Handle Implicit arguments consistently between interpreter and JIT (#120339)
- The JIT did not match the behavior specified in the ECMA 335 standard. However, since its behavior is long-standing, instead of fixing the JIT, I'm proposing a new section in the ECMA augments document. - I've written tests for the scenarios where the conversion happens - And I've fixed the interpreter compiler to match the behavior the JIT has for these scenarios
1 parent f8796dc commit 9833f60

File tree

8 files changed

+214
-13
lines changed

8 files changed

+214
-13
lines changed

‎docs/design/specs/Ecma-335-Augments.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ This is a list of additions and edits to be made in ECMA-335 specifications. It
2525
- [API documentation](#api-documentation)
2626
- [Debug Interchange Format](#debug-interchange-format)
2727
- [Extended layout](#extended-layout)
28+
- [Implicit argument coercion rules](#implicit-argument-coercion-rules)
2829

2930
## Signatures
3031

@@ -1191,3 +1192,6 @@ In section II.23.1.15, the following row is added to the table:
11911192
|-----|------|------|
11921193
| `ExtendedLayout` | 0x00000018 | Layout is supplied by a `System.Runtime.InteropServices.ExtendedLayoutAttribute` custom attribute |
11931194

1195+
## Implict argument coercion rules
1196+
1197+
Implicit argument coercion as defined in section III.1.6 does not match with existing practice in CLR runtimes. Notably, implicit argument coercion of an `int32` on the IL evaluation stack to a `native unsigned int` is a sign extending operation, not a zero-extending operation.

‎src/coreclr/interpreter/compiler.cpp‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2405,7 +2405,7 @@ void InterpCompiler::EmitStoreVar(int32_t var)
24052405
AddIns(INTOP_LOAD_FRAMEVAR);
24062406
PushInterpType(InterpTypeI, NULL);
24072407
m_pLastNewIns->SetDVar(m_pStackPointer[-1].var);
2408-
EmitStind(interpType, m_pVars[var].clsHnd, m_pVars[var].offset, true /* reverseSVarOrder */);
2408+
EmitStind(interpType, m_pVars[var].clsHnd, m_pVars[var].offset, true /* reverseSVarOrder */, false /* enableImplicitArgConversionRules */);
24092409
return;
24102410
}
24112411

@@ -3885,10 +3885,8 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re
38853885
{
38863886
#ifdef TARGET_64BIT
38873887
case CORINFO_TYPE_NATIVEINT:
3888-
EmitConv(m_pStackPointer + iCurrentStackArg, StackTypeI8, INTOP_CONV_I8_I4);
3889-
break;
38903888
case CORINFO_TYPE_NATIVEUINT:
3891-
EmitConv(m_pStackPointer + iCurrentStackArg, StackTypeI8, INTOP_CONV_U8_U4);
3889+
EmitConv(m_pStackPointer + iCurrentStackArg, StackTypeI8, INTOP_CONV_I8_I4); // This subtly differs from the published ECMA spec, and is what is defined in the Ecma-335-Augments.md document
38923890
break;
38933891
#endif // TARGET_64BIT
38943892
case CORINFO_TYPE_BYTE:
@@ -4343,11 +4341,11 @@ void InterpCompiler::EmitLdind(InterpType interpType, CORINFO_CLASS_HANDLE clsHn
43434341
m_pLastNewIns->SetDVar(m_pStackPointer[-1].var);
43444342
}
43454343

4346-
void InterpCompiler::EmitStind(InterpType interpType, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder)
4344+
void InterpCompiler::EmitStind(InterpType interpType, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder, bool enableImplicitArgConversionRules)
43474345
{
43484346
int stackIndexValue;
43494347
int stackIndexAddress;
4350-
if (reverseSVarOrder)
4348+
if (!reverseSVarOrder)
43514349
{
43524350
stackIndexAddress = -2;
43534351
stackIndexValue = -1;
@@ -4366,6 +4364,10 @@ void InterpCompiler::EmitStind(InterpType interpType, CORINFO_CLASS_HANDLE clsHn
43664364
{
43674365
EmitConv(m_pStackPointer + stackIndexValue, StackTypeR8, INTOP_CONV_R8_R4);
43684366
}
4367+
else if (enableImplicitArgConversionRules && interpType == InterpTypeI8 && m_pStackPointer[stackIndexValue].type == StackTypeI4) // This subtly differs from the published ECMA spec for section III.1.6, and is what is defined in the Ecma-335-Augments.md document
4368+
{
4369+
EmitConv(m_pStackPointer + stackIndexValue, StackTypeI8, INTOP_CONV_I8_I4);
4370+
}
43694371

43704372
// stack contains address and then the value to be stored
43714373
// or in the reverse order if the flag is set
@@ -4389,7 +4391,7 @@ void InterpCompiler::EmitStind(InterpType interpType, CORINFO_CLASS_HANDLE clsHn
43894391

43904392
m_pLastNewIns->data[0] = offset;
43914393

4392-
m_pLastNewIns->SetSVars2(m_pStackPointer[stackIndexValue].var, m_pStackPointer[stackIndexAddress].var);
4394+
m_pLastNewIns->SetSVars2(m_pStackPointer[stackIndexAddress].var, m_pStackPointer[stackIndexValue].var);
43934395
m_pStackPointer -= 2;
43944396
}
43954397

@@ -4561,7 +4563,7 @@ void InterpCompiler::EmitStaticFieldAccess(InterpType interpFieldType, CORINFO_F
45614563
if (isLoad)
45624564
EmitLdind(interpFieldType, pFieldInfo->structType, 0);
45634565
else
4564-
EmitStind(interpFieldType, pFieldInfo->structType, 0, true);
4566+
EmitStind(interpFieldType, pFieldInfo->structType, 0, true, true /* enableImplicitArgConversionRules */);
45654567
break;
45664568
}
45674569
}
@@ -5101,7 +5103,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
51015103
}
51025104
else
51035105
{
5104-
EmitStind(interpType, resolvedToken.hClass, 0, false);
5106+
EmitStind(interpType, resolvedToken.hClass, 0, false, false /* enableImplicitArgConversionRules */);
51055107
}
51065108
m_ip += 5;
51075109
break;
@@ -5114,7 +5116,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
51145116
ResolveToken(getU4LittleEndian(m_ip + 1), CORINFO_TOKENKIND_Class, &resolvedToken);
51155117
InterpType interpType = GetInterpType(m_compHnd->asCorInfoType(resolvedToken.hClass));
51165118
EmitLdind(interpType, resolvedToken.hClass, 0);
5117-
EmitStind(interpType, resolvedToken.hClass, 0, false);
5119+
EmitStind(interpType, resolvedToken.hClass, 0, false, false /* enableImplicitArgConversionRules */);
51185120
m_ip += 5;
51195121
break;
51205122
}
@@ -6402,7 +6404,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
64026404
else
64036405
{
64046406
assert(fieldInfo.fieldAccessor == CORINFO_FIELD_INSTANCE);
6405-
EmitStind(interpFieldType, fieldInfo.structType, fieldInfo.offset, false);
6407+
EmitStind(interpFieldType, fieldInfo.structType, fieldInfo.offset, false, true /* enableImplicitArgConversionRules */);
64066408
}
64076409
m_ip += 5;
64086410

@@ -6570,7 +6572,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo)
65706572
AddIns(INTOP_MEMBAR);
65716573
volatile_ = false;
65726574
}
6573-
EmitStind(interpType, NULL, 0, false);
6575+
EmitStind(interpType, NULL, 0, false, false /* enableImplicitArgConversionRules */);
65746576
m_ip++;
65756577
break;
65766578
}

‎src/coreclr/interpreter/compiler.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -737,7 +737,7 @@ class InterpCompiler
737737
void EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CORINFO_SIG_INFO* callSiteSig);
738738
bool EmitNamedIntrinsicCall(NamedIntrinsic ni, bool nonVirtualCall, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig);
739739
void EmitLdind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset);
740-
void EmitStind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder);
740+
void EmitStind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder, bool enableImplicitArgConversionRules);
741741
void EmitLdelem(int32_t opcode, InterpType type);
742742
void EmitStelem(InterpType type);
743743
void EmitStaticFieldAddress(CORINFO_FIELD_INFO *pFieldInfo, CORINFO_RESOLVED_TOKEN *pResolvedToken);

‎src/tests/JIT/Methodical/casts/coverage/implicit_arg_coercion.il‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
.assembly extern mscorlib { }
5+
.assembly extern System.Console
6+
{
7+
.publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A )
8+
.ver 4:0:0:0
9+
}
10+
.assembly ASSEMBLY_NAME
11+
{
12+
}
13+
.assembly extern xunit.core {}
14+
.assembly extern Microsoft.DotNet.XUnitExtensions { .publickeytoken = (31 BF 38 56 AD 36 4E 35 ) }
15+
.namespace JitTest_implicit_arg_coercion
16+
{
17+
.class public auto ansi beforefieldinit TestClass
18+
extends [mscorlib]System.Object
19+
{
20+
.field private static native int s_data
21+
.field private static native uint s_udata
22+
23+
.method public hidebysig static native int TakeNativeIntArg(native int) cil managed
24+
{
25+
ldarg.0
26+
ret
27+
}
28+
29+
.method public hidebysig static native uint TakeNativeUIntArg(native uint) cil managed
30+
{
31+
ldarg.0
32+
ret
33+
}
34+
35+
.method public hidebysig static int32 TakeU2Arg(uint16) cil managed
36+
{
37+
ldarg.0
38+
ret
39+
}
40+
41+
.method public hidebysig static int32 TakeU1Arg(uint8) cil managed
42+
{
43+
ldarg.0
44+
ret
45+
}
46+
47+
.method public hidebysig static int32
48+
Main() cil managed
49+
{
50+
.custom instance void [xunit.core]Xunit.FactAttribute::.ctor() = (
51+
01 00 00 00
52+
)
53+
.custom instance void [Microsoft.DotNet.XUnitExtensions]Xunit.SkipOnMonoAttribute::.ctor(string, valuetype [Microsoft.DotNet.XUnitExtensions]Xunit.TestPlatforms) = {
54+
string('Handling of implicit arg coercion rules III.1.6 differs between Mono and CoreCLR, and this tests the CoreCLR behavior, and what we have put in the augments')
55+
int32(0xFFFFFFFF) // Any
56+
}
57+
58+
.entrypoint
59+
// Code size 36 (0x24)
60+
.maxstack 2
61+
ldc.i4.m1
62+
stsfld native int JitTest_implicit_arg_coercion.TestClass::s_data
63+
ldc.i4.m1
64+
stsfld native uint JitTest_implicit_arg_coercion.TestClass::s_udata
65+
66+
ldsfld native int JitTest_implicit_arg_coercion.TestClass::s_data
67+
conv.u8
68+
box [mscorlib]System.UInt64
69+
call void [System.Console]System.Console::WriteLine(object)
70+
71+
ldsfld native uint JitTest_implicit_arg_coercion.TestClass::s_udata
72+
conv.u8
73+
box [mscorlib]System.UInt64
74+
call void [System.Console]System.Console::WriteLine(object)
75+
76+
ldc.i4.m1
77+
conv.i
78+
box [mscorlib]System.UInt64
79+
call void [System.Console]System.Console::WriteLine(object)
80+
81+
ldc.i4.m1
82+
conv.u
83+
box [mscorlib]System.UInt64
84+
call void [System.Console]System.Console::WriteLine(object)
85+
86+
ldc.i4.m1
87+
conv.i
88+
ldsfld native int JitTest_implicit_arg_coercion.TestClass::s_data
89+
beq IL_PassNativeInt
90+
ldstr "Failed conversion of i4 into i for stsfld"
91+
call void [System.Console]System.Console::WriteLine(string)
92+
ldc.i4.s 101
93+
ret
94+
95+
IL_PassNativeInt:
96+
ldc.i4.m1
97+
conv.i // The published ECMA 335 spec specifies that we should use the implicit argument coercion rules in section III.
98+
ldsfld native uint JitTest_implicit_arg_coercion.TestClass::s_udata
99+
beq IL_PassNativeUInt
100+
ldstr "Failed conversion of i4 into u for stsfld"
101+
call void [System.Console]System.Console::WriteLine(string)
102+
ldc.i4.s 102
103+
ret
104+
IL_PassNativeUInt:
105+
106+
ldc.i4.m1
107+
call native int [ASSEMBLY_NAME]JitTest_implicit_arg_coercion.TestClass::TakeNativeIntArg(native int)
108+
ldc.i4.m1
109+
conv.i
110+
beq IL_PassNativeIntCall
111+
ldstr "Failed conversion of i4 into i for argument call"
112+
call void [System.Console]System.Console::WriteLine(string)
113+
ldc.i4.s 103
114+
ret
115+
IL_PassNativeIntCall:
116+
117+
ldc.i4.m1
118+
call native uint [ASSEMBLY_NAME]JitTest_implicit_arg_coercion.TestClass::TakeNativeUIntArg(native uint)
119+
ldc.i4.m1
120+
conv.i
121+
beq IL_PassNativeUIntCall
122+
ldstr "Failed conversion of i4 into u for argument call"
123+
call void [System.Console]System.Console::WriteLine(string)
124+
ldc.i4.s 104
125+
ret
126+
IL_PassNativeUIntCall:
127+
128+
ldc.i4.m1
129+
call int32 [ASSEMBLY_NAME]JitTest_implicit_arg_coercion.TestClass::TakeU2Arg(uint16)
130+
ldc.i4 0xFFFF
131+
beq IL_PassU2Call
132+
ldstr "Failed conversion of i4 into u2 for argument call"
133+
call void [System.Console]System.Console::WriteLine(string)
134+
ldc.i4.s 105
135+
ret
136+
IL_PassU2Call:
137+
138+
ldc.i4.m1
139+
call int32 [ASSEMBLY_NAME]JitTest_implicit_arg_coercion.TestClass::TakeU1Arg(uint8)
140+
ldc.i4 0xFF
141+
beq IL_PassU1Call
142+
ldstr "Failed conversion of i4 into u1 for argument call"
143+
call void [System.Console]System.Console::WriteLine(string)
144+
ldc.i4.s 105
145+
ret
146+
IL_PassU1Call:
147+
148+
ldstr "Passed => 100"
149+
IL_001c: call void [System.Console]System.Console::WriteLine(string)
150+
IL_0021: ldc.i4.s 100
151+
IL_0023: ret
152+
} // end of method TestClass::Main
153+
154+
.method public hidebysig specialname rtspecialname
155+
instance void .ctor() cil managed
156+
{
157+
// Code size 7 (0x7)
158+
.maxstack 8
159+
IL_0000: ldarg.0
160+
IL_0001: call instance void [mscorlib]System.Object::.ctor()
161+
IL_0006: ret
162+
} // end of method TestClass::.ctor
163+
164+
} // end of class TestClass
165+
166+
} // end of namespace JitTest_implicit_arg_coercion
167+
168+
//*********** DISASSEMBLY COMPLETE ***********************
169+
// WARNING: Created Win32 resource file ldnull.res

‎src/tests/JIT/Methodical/casts/coverage/implicit_arg_coercion_d.il‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
#define ASSEMBLY_NAME "implicit_arg_coercion_d"

‎src/tests/JIT/Methodical/casts/coverage/implicit_arg_coercion_d.ilproj‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<Project Sdk="Microsoft.NET.Sdk.IL">
2+
<PropertyGroup>
3+
<CLRTestPriority>0</CLRTestPriority>
4+
</PropertyGroup>
5+
<PropertyGroup>
6+
<DebugType>Full</DebugType>
7+
</PropertyGroup>
8+
<ItemGroup>
9+
<Compile Include="implicit_arg_coercion_d.il" />
10+
<Compile Include="implicit_arg_coercion.il" />
11+
</ItemGroup>
12+
</Project>

‎src/tests/JIT/Methodical/casts/coverage/implicit_arg_coercion_r.il‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
#define ASSEMBLY_NAME "implicit_arg_coercion_r"

‎src/tests/JIT/Methodical/casts/coverage/implicit_arg_coercion_r.ilproj‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<Project Sdk="Microsoft.NET.Sdk.IL">
2+
<PropertyGroup>
3+
<CLRTestPriority>0</CLRTestPriority>
4+
</PropertyGroup>
5+
<PropertyGroup>
6+
<DebugType>PdbOnly</DebugType>
7+
</PropertyGroup>
8+
<ItemGroup>
9+
<Compile Include="implicit_arg_coercion_r.il" />
10+
<Compile Include="implicit_arg_coercion.il" />
11+
</ItemGroup>
12+
</Project>

0 commit comments

Comments
 (0)