Skip to content

Add an implicit argument coercion check. #43386

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.

Sign up for GitHub

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 6 commits into from
Nov 6, 2020
Merged

Add an implicit argument coercion check. #43386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d54e56f
Add `impCheckImplicitArgumentCoercion`.
Oct 14, 2020
b7d7d77
Fix tests with type mismatch.
Oct 14, 2020
8158156
Try to fix VM signature.
Oct 14, 2020
07bb68d
Allow to pass byref as native int.
Oct 14, 2020
02452ba
another fix.
Oct 15, 2020
e63f15b
Fix another IL test.
Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/src/System.Private.CoreLib/src/System/StubHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ internal static class InterfaceMarshaler
internal static extern IntPtr ConvertToNative(object objSrc, IntPtr itfMT, IntPtr classMT, int flags);

[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern object ConvertToManaged(IntPtr ppUnk, IntPtr itfMT, IntPtr classMT, int flags);
internal static extern object ConvertToManaged(ref IntPtr ppUnk, IntPtr itfMT, IntPtr classMT, int flags);

[DllImport(RuntimeHelpers.QCall)]
internal static extern void ClearNative(IntPtr pUnk);
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4121,6 +4121,8 @@ class Compiler

GenTreeCall::Use* impPopCallArgs(unsigned count, CORINFO_SIG_INFO* sig, GenTreeCall::Use* prefixArgs = nullptr);

bool impCheckImplicitArgumentCoercion(var_types sigType, var_types nodeType) const;

GenTreeCall::Use* impPopReverseCallArgs(unsigned count, CORINFO_SIG_INFO* sig, unsigned skipReverseCount = 0);

/*
Expand Down
118 changes: 115 additions & 3 deletions src/coreclr/src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,20 +946,27 @@ GenTreeCall::Use* Compiler::impPopCallArgs(unsigned count, CORINFO_SIG_INFO* sig

CorInfoType corType = strip(info.compCompHnd->getArgType(sig, argLst, &argClass));

var_types jitSigType = JITtype2varType(corType);

if (!impCheckImplicitArgumentCoercion(jitSigType, arg->GetNode()->TypeGet()))
{
BADCODE("the call argument has a type that can't be implicitly converted to the signature type");
}

// insert implied casts (from float to double or double to float)

if ((corType == CORINFO_TYPE_DOUBLE) && (arg->GetNode()->TypeGet() == TYP_FLOAT))
if ((jitSigType == TYP_DOUBLE) && (arg->GetNode()->TypeGet() == TYP_FLOAT))
{
arg->SetNode(gtNewCastNode(TYP_DOUBLE, arg->GetNode(), false, TYP_DOUBLE));
}
else if ((corType == CORINFO_TYPE_FLOAT) && (arg->GetNode()->TypeGet() == TYP_DOUBLE))
else if ((jitSigType == TYP_FLOAT) && (arg->GetNode()->TypeGet() == TYP_DOUBLE))
{
arg->SetNode(gtNewCastNode(TYP_FLOAT, arg->GetNode(), false, TYP_FLOAT));
}

// insert any widening or narrowing casts for backwards compatibility

arg->SetNode(impImplicitIorI4Cast(arg->GetNode(), JITtype2varType(corType)));
arg->SetNode(impImplicitIorI4Cast(arg->GetNode(), jitSigType));

if (corType != CORINFO_TYPE_CLASS && corType != CORINFO_TYPE_BYREF && corType != CORINFO_TYPE_PTR &&
corType != CORINFO_TYPE_VAR && (argRealClass = info.compCompHnd->getArgClass(sig, argLst)) != nullptr)
Expand Down Expand Up @@ -993,6 +1000,111 @@ GenTreeCall::Use* Compiler::impPopCallArgs(unsigned count, CORINFO_SIG_INFO* sig
return argList;
}

static bool TypeIs(var_types type1, var_types type2)
{
return type1 == type2;
}

// Check if type1 matches any type from the list.
template <typename... T>
static bool TypeIs(var_types type1, var_types type2, T... rest)
{
return TypeIs(type1, type2) || TypeIs(type1, rest...);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like these are generally useful and should be in a header somewhere?

Copy link
Contributor Author

@sandreenko sandreenko Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have GenTree::TypeIs and it is usually sufficient, we don't have many places where we compare types not from tree nodes.
I did not want to make this function general available because it is a little confusing that the first argument is special there, but I am open to other opinions. I thought it would be nice to make it a member function, like var_types::TypeIs(this, types to compare this with) but var_types is not a class, it is an enum.

//------------------------------------------------------------------------
// impCheckImplicitArgumentCoercion: check that the node's type is compatible with
// the signature's type using ECMA implicit argument coercion table.
//
// Arguments:
// sigType - the type in the call signature;
// nodeType - the node type.
//
// Return Value:
// true if they are compatible, false otherwise.
//
// Notes:
// - it is currently allowing byref->long passing, should be fixed in VM;
// - it can't check long -> native int case on 64-bit platforms,
// so the behavior is different depending on the target bitness.
//
bool Compiler::impCheckImplicitArgumentCoercion(var_types sigType, var_types nodeType) const
{
if (sigType == nodeType)
{
return true;
}

if (TypeIs(sigType, TYP_BOOL, TYP_UBYTE, TYP_BYTE, TYP_USHORT, TYP_SHORT, TYP_UINT, TYP_INT))
{
if (TypeIs(nodeType, TYP_BOOL, TYP_UBYTE, TYP_BYTE, TYP_USHORT, TYP_SHORT, TYP_UINT, TYP_INT, TYP_I_IMPL))
{
return true;
}
}
else if (TypeIs(sigType, TYP_ULONG, TYP_LONG))
{
if (TypeIs(nodeType, TYP_LONG))
{
return true;
}
}
else if (TypeIs(sigType, TYP_FLOAT, TYP_DOUBLE))
{
if (TypeIs(nodeType, TYP_FLOAT, TYP_DOUBLE))
{
return true;
}
}
else if (TypeIs(sigType, TYP_BYREF))
{
if (TypeIs(nodeType, TYP_I_IMPL))
{
return true;
}

// This condition tolerates such IL:
// ; V00 this ref this class-hnd
// ldarg.0
// call(byref)
if (TypeIs(nodeType, TYP_REF))
{
return true;
}
}
else if (varTypeIsStruct(sigType))
{
if (varTypeIsStruct(nodeType))
{
return true;
}
}

// This condition should not be under `else` because `TYP_I_IMPL`
// intersects with `TYP_LONG` or `TYP_INT`.
if (TypeIs(sigType, TYP_I_IMPL, TYP_U_IMPL))
{
// Note that it allows `ldc.i8 1; call(nint)` on 64-bit platforms,
// but we can't distinguish `nint` from `long` there.
if (TypeIs(nodeType, TYP_I_IMPL, TYP_U_IMPL, TYP_INT, TYP_UINT))
{
return true;
}

// It tolerates IL that ECMA does not allow but that is commonly used.
// Example:
// V02 loc1 struct <RTL_OSVERSIONINFOEX, 32>
// ldloca.s 0x2
// call(native int)
if (TypeIs(nodeType, TYP_BYREF))
{
return true;
}
}

return false;
}

/*****************************************************************************
*
* Pop the given number of values from the stack in reverse order (STDCALL/CDECL etc.)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/vm/corelib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,7 @@ DEFINE_METHOD(OBJECTMARSHALER, CLEAR_NATIVE, ClearNative,

DEFINE_CLASS(INTERFACEMARSHALER, StubHelpers, InterfaceMarshaler)
DEFINE_METHOD(INTERFACEMARSHALER, CONVERT_TO_NATIVE, ConvertToNative, SM_Obj_IntPtr_IntPtr_Int_RetIntPtr)
DEFINE_METHOD(INTERFACEMARSHALER, CONVERT_TO_MANAGED, ConvertToManaged, SM_IntPtr_IntPtr_IntPtr_Int_RetObj)
DEFINE_METHOD(INTERFACEMARSHALER, CONVERT_TO_MANAGED, ConvertToManaged, SM_RefIntPtr_IntPtr_IntPtr_Int_RetObj)
DEFINE_METHOD(INTERFACEMARSHALER, CLEAR_NATIVE, ClearNative, SM_IntPtr_RetVoid)


Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/vm/metasig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@
DEFINE_METASIG_T(SM(Int_IntPtr_Obj_RetException, i I j, C(EXCEPTION)))
DEFINE_METASIG_T(SM(Type_ArrType_IntPtr_int_RetType, C(TYPE) a(C(TYPE)) I i, C(TYPE) ))
DEFINE_METASIG_T(SM(Type_RetIntPtr, C(TYPE), I))
DEFINE_METASIG(SM(IntPtr_IntPtr_IntPtr_Int_RetObj, I I I i, j))
DEFINE_METASIG(SM(RefIntPtr_IntPtr_IntPtr_Int_RetObj, r(I) I I i, j))
DEFINE_METASIG(SM(Obj_IntPtr_RetIntPtr, j I, I))
DEFINE_METASIG(SM(Obj_IntPtr_RetObj, j I, j))
DEFINE_METASIG(SM(Obj_RefIntPtr_RetVoid, j r(I), v))
Expand Down
32 changes: 16 additions & 16 deletions src/tests/JIT/Directed/coverage/importer/Desktop/byrefsubbyref1.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,21 @@
.class a extends [mscorlib]System.Object
{
.field static class ctest S_1
.method public static int32 byrefsubbyref(class ctest& V_1, class ctest& V_2)
.method public static native int byrefsubbyref(class ctest& V_1, class ctest& V_2)
{
ldarg 0
ldarg 1
sub
ret
}
.method public static int32 byrefsubi4(class ctest& V_1, int32 V_2)
.method public static native int byrefsubi4(class ctest& V_1, int32 V_2)
{
ldarg 0
ldarg 1
sub
ret
}
.method public static int32 i4subbyref(int32, class ctest& V_2)
.method public static native int i4subbyref(int32, class ctest& V_2)
{
ldarg 0
ldarg 1
Expand All @@ -53,54 +53,54 @@ ret
IL_0010:
ldloca V_2
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
dup
stloc.2
call void [System.Console]System.Console::WriteLine(int32)

ldloca V_2
ldc.i4 1
call int32 a::byrefsubi4(class ctest&, int32)
call native int a::byrefsubi4(class ctest&, int32)
call void [System.Console]System.Console::WriteLine(int32)

ldc.i4 1
ldloca V_1
call int32 a::i4subbyref(int32, class ctest&)
call native int a::i4subbyref(int32, class ctest&)
call void [System.Console]System.Console::WriteLine(int32)

newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stsfld class ctest a::S_1
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
call void [System.Console]System.Console::WriteLine(int32)

ldc.i4 100
Expand Down
34 changes: 17 additions & 17 deletions src/tests/JIT/Directed/coverage/importer/byrefsubbyref1.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,23 @@
.class a extends [mscorlib]System.Object
{
.field static class ctest S_1
.method public static int32 byrefsubbyref(class ctest& V_1, class ctest& V_2)
.method public static native int byrefsubbyref(class ctest& V_1, class ctest& V_2)
{
// op1: byref, op2: byref
ldarg 0
ldarg 1
sub
ret
}
.method public static int32 byrefsubi4(class ctest& V_1, int32 V_2)
.method public static native int byrefsubi4(class ctest& V_1, int32 V_2)
{
// op1: byref, op2: int32
ldarg 0
ldarg 1
sub
ret
}
.method public static int32 i4subbyref(int32, class ctest& V_2)
.method public static native int i4subbyref(int32, class ctest& V_2)
{
// op1: int32, op2: byref
ldarg 0
Expand All @@ -41,7 +41,7 @@ ret
.maxstack 2
.locals init (class ctest V_1,
class ctest V_2,
int32 V_3)
native int V_3)
IL_0004: newobj instance void ctest::.ctor()
IL_0009: stloc.0
IL_000a: newobj instance void ctest::.ctor()
Expand All @@ -53,57 +53,57 @@ ret
IL_0010:
ldloca V_2
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
dup
stloc.2
call void [System.Console]System.Console::WriteLine(int32)

// op1: byref, op2: int
ldloca V_2
ldc.i4 1
call int32 a::byrefsubi4(class ctest&, int32)
call native int a::byrefsubi4(class ctest&, int32)
call void [System.Console]System.Console::WriteLine(int32)

// op1: int, op2: byref
ldc.i4 1
ldloca V_1
call int32 a::i4subbyref(int32, class ctest&)
call native int a::i4subbyref(int32, class ctest&)
call void [System.Console]System.Console::WriteLine(int32)

// op1: byref, op2: byref
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stsfld class ctest a::S_1
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
ldsflda class ctest a::S_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
newobj instance void ctest::.ctor()
stloc.0
ldloca V_1
call int32 a::byrefsubbyref(class ctest&, class ctest&)
call native int a::byrefsubbyref(class ctest&, class ctest&)
call void [System.Console]System.Console::WriteLine(int32)

ldc.i4 100
Expand Down
2 changes: 1 addition & 1 deletion src/tests/JIT/Regression/JitBlue/GitHub_18295/GitHub_18295.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
stloc returnVal

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

ldc.i4.0
Expand Down
Loading