Skip to content

Type verification in tests #40186

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
davidwrighton merged 8 commits into from
Aug 3, 2020
Merged

Type verification in tests #40186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f63fb2c
Add type verification to crossgen2 test pass
davidwrighton Jul 31, 2020
c0c6b75
Enable CG2 type layout verification in CG2 test passes
davidwrighton Jul 31, 2020
d52c7d1
Fix handling of fields with extremely large offsets and type layout v…
davidwrighton Jul 31, 2020
42dfdc2
Fix hfa encoding in check/verify type layout
davidwrighton Jul 31, 2020
5967917
Use R2R enum type instead of internal CLR type to describe HFA layouts
davidwrighton Jul 31, 2020
5e477d9
Tweak Blittability around generics, and add test that behavior is con…
davidwrighton Aug 3, 2020
10c02c2
Code review feedback and deletion of unnecessary comments
davidwrighton Aug 3, 2020
ec89e0d
Revert unintended change to crossgen2smoke
davidwrighton Aug 3, 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
9 changes: 9 additions & 0 deletions src/coreclr/src/inc/readytorun.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,4 +395,13 @@ enum ReadyToRunRuntimeConstants : DWORD
READYTORUN_ReversePInvokeTransitionFrameSizeInPointerUnits = 2
};

enum ReadyToRunHFAElemType : DWORD
{
READYTORUN_HFA_ELEMTYPE_None = 0,
READYTORUN_HFA_ELEMTYPE_Float32 = 1,
READYTORUN_HFA_ELEMTYPE_Float64 = 2,
READYTORUN_HFA_ELEMTYPE_Vector64 = 3,
READYTORUN_HFA_ELEMTYPE_Vector128 = 4,
};

#endif // __READYTORUN_H__
11 changes: 11 additions & 0 deletions src/coreclr/src/tools/Common/Internal/Runtime/ReadyToRunConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,17 @@ public enum ReadyToRunHelper
TypeHandleToRuntimeTypeHandle,
}

// Enum used for HFA type recognition.
// Supported across architectures, so that it can be used in altjits and cross-compilation.
public enum ReadyToRunHFAElemType
{
None = 0,
Float32 = 1,
Float64 = 2,
Vector64 = 3,
Vector128 = 4,
}

public static class ReadyToRunRuntimeConstants
{
public const int READYTORUN_PInvokeTransitionFrameSizeInPointerUnits = 11;
Expand Down
5 changes: 4 additions & 1 deletion ...s/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;

using Internal.JitInterface;
using Internal.Text;
Expand All @@ -13,17 +14,19 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun
{
public class FieldFixupSignature : Signature
{
public const int MaxCheckableOffset = 0x1FFFFFFF;
Copy link
Member

@AntonLapounov AntonLapounov Aug 1, 2020

Choose a reason for hiding this comment

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

What was the issue with larger offsets? Are they now unchecked?

Copy link
Member Author

@davidwrighton davidwrighton Aug 3, 2020

Choose a reason for hiding this comment

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

They are now unchecked. The problem is that the encoding uses the compressed uint encoding from metadata which cannot encode a number greater than 0x1FFFFFFF.

In practice, the need to do this is so rare, that it isn't worth changing the encoding.

Copy link
Member

@AntonLapounov AntonLapounov Aug 3, 2020

Choose a reason for hiding this comment

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

Shouldn't we fail compilation for large offsets then? I see throwing a RequiresRuntimeJitException in one if branch, but not in others.

private readonly ReadyToRunFixupKind _fixupKind;

private readonly FieldDesc _fieldDesc;

public FieldFixupSignature(ReadyToRunFixupKind fixupKind, FieldDesc fieldDesc)
public FieldFixupSignature(ReadyToRunFixupKind fixupKind, FieldDesc fieldDesc, NodeFactory factory)
{
_fixupKind = fixupKind;
_fieldDesc = fieldDesc;

// Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature
((CompilerTypeSystemContext)fieldDesc.Context).EnsureLoadableType(fieldDesc.OwningType);
Debug.Assert(factory.SignatureContext.GetTargetModule(_fieldDesc) != null);
}

public override int ClassCode => 271828182;
Expand Down
14 changes: 7 additions & 7 deletions ...ls/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,16 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ

if (defType.IsHomogeneousAggregate)
{
CorElementType elementType = (defType.ValueTypeShapeCharacteristics & ValueTypeShapeCharacteristics.AggregateMask) switch
ReadyToRunHFAElemType hfaElementType = (defType.ValueTypeShapeCharacteristics & ValueTypeShapeCharacteristics.AggregateMask) switch
{
ValueTypeShapeCharacteristics.Float32Aggregate => CorElementType.ELEMENT_TYPE_R4,
ValueTypeShapeCharacteristics.Float64Aggregate => CorElementType.ELEMENT_TYPE_R8,
ValueTypeShapeCharacteristics.Vector64Aggregate => CorElementType.ELEMENT_TYPE_R8,
ValueTypeShapeCharacteristics.Float32Aggregate => ReadyToRunHFAElemType.Float32,
ValueTypeShapeCharacteristics.Float64Aggregate => ReadyToRunHFAElemType.Float64,
ValueTypeShapeCharacteristics.Vector64Aggregate => ReadyToRunHFAElemType.Vector64,
// See MethodTable::GetHFAType
ValueTypeShapeCharacteristics.Vector128Aggregate => CorElementType.ELEMENT_TYPE_VALUETYPE,
_ => CorElementType.Invalid
ValueTypeShapeCharacteristics.Vector128Aggregate => ReadyToRunHFAElemType.Vector128,
_ => throw new NotSupportedException()
};
dataBuilder.EmitUInt((uint)elementType);
dataBuilder.EmitUInt((uint)hfaElementType);
}

if (alignment != pointerSize)
Expand Down
8 changes: 4 additions & 4 deletions ...ools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,15 @@ private void CreateNodeCaches()
_codegenNodeFactory,
_codegenNodeFactory.HelperImports,
ReadyToRunHelper.DelayLoad_Helper,
new FieldFixupSignature(ReadyToRunFixupKind.FieldAddress, key)
new FieldFixupSignature(ReadyToRunFixupKind.FieldAddress, key, _codegenNodeFactory)
);
});

_fieldOffsetCache = new NodeCache<FieldDesc, ISymbolNode>(key =>
{
return new PrecodeHelperImport(
_codegenNodeFactory,
new FieldFixupSignature(ReadyToRunFixupKind.FieldOffset, key)
new FieldFixupSignature(ReadyToRunFixupKind.FieldOffset, key, _codegenNodeFactory)
);
});

Expand All @@ -94,7 +94,7 @@ private void CreateNodeCaches()
{
return new PrecodeHelperImport(
_codegenNodeFactory,
new FieldFixupSignature(_verifyTypeAndFieldLayout ? ReadyToRunFixupKind.Verify_FieldOffset : ReadyToRunFixupKind.Check_FieldOffset, key)
new FieldFixupSignature(_verifyTypeAndFieldLayout ? ReadyToRunFixupKind.Verify_FieldOffset : ReadyToRunFixupKind.Check_FieldOffset, key, _codegenNodeFactory)
);
});

Expand Down Expand Up @@ -356,7 +356,7 @@ private ISymbolNode CreateFieldHandleHelper(FieldDesc field)
{
return new PrecodeHelperImport(
_codegenNodeFactory,
new FieldFixupSignature(ReadyToRunFixupKind.FieldHandle, field));
new FieldFixupSignature(ReadyToRunFixupKind.FieldHandle, field, _codegenNodeFactory));
}

private ISymbolNode CreateCctorTrigger(TypeDesc type)
Expand Down
15 changes: 9 additions & 6 deletions src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,7 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET
CorInfoHelpFunc.CORINFO_HELP_GETGENERICS_NONGCSTATIC_BASE);
}

if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout)
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout && (fieldOffset <= FieldFixupSignature.MaxCheckableOffset))
{
// ENCODE_CHECK_FIELD_OFFSET
_methodCodeNode.Fixups.Add(_compilation.SymbolNodeFactory.CheckFieldOffset(field));
Expand Down Expand Up @@ -1066,7 +1066,7 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET
else
if (helperId != ReadyToRunHelperId.Invalid)
{
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout)
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout && (fieldOffset <= FieldFixupSignature.MaxCheckableOffset))
{
// ENCODE_CHECK_FIELD_OFFSET
_methodCodeNode.Fixups.Add(_compilation.SymbolNodeFactory.CheckFieldOffset(field));
Expand Down Expand Up @@ -1922,7 +1922,7 @@ private bool NeedsTypeLayoutCheck(TypeDesc type)
if (!type.IsValueType)
return false;

return !_compilation.IsLayoutFixedInCurrentVersionBubble(type) || _compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout;
return !_compilation.IsLayoutFixedInCurrentVersionBubble(type) || (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout && !((MetadataType)type).IsNonVersionable());
}

private bool HasLayoutMetadata(TypeDesc type)
Expand Down Expand Up @@ -1963,6 +1963,9 @@ private void EncodeFieldBaseOffset(FieldDesc field, CORINFO_FIELD_INFO* pResult,
if (pMT.IsValueType)
{
// ENCODE_CHECK_FIELD_OFFSET
if (pResult->offset > FieldFixupSignature.MaxCheckableOffset)
throw new RequiresRuntimeJitException(callerMethod.ToString() + " -> " + field.ToString());

_methodCodeNode.Fixups.Add(_compilation.SymbolNodeFactory.CheckFieldOffset(field));
// No-op other than generating the check field offset fixup
}
Expand All @@ -1978,7 +1981,7 @@ private void EncodeFieldBaseOffset(FieldDesc field, CORINFO_FIELD_INFO* pResult,
}
else if (pMT.IsValueType)
{
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout)
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout && !callerMethod.IsNonVersionable() && (pResult->offset <= FieldFixupSignature.MaxCheckableOffset))
{
// ENCODE_CHECK_FIELD_OFFSET
_methodCodeNode.Fixups.Add(_compilation.SymbolNodeFactory.CheckFieldOffset(field));
Expand All @@ -1987,7 +1990,7 @@ private void EncodeFieldBaseOffset(FieldDesc field, CORINFO_FIELD_INFO* pResult,
}
else if (_compilation.IsInheritanceChainLayoutFixedInCurrentVersionBubble(pMT.BaseType))
{
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout)
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout && !callerMethod.IsNonVersionable() && (pResult->offset <= FieldFixupSignature.MaxCheckableOffset))
{
// ENCODE_CHECK_FIELD_OFFSET
_methodCodeNode.Fixups.Add(_compilation.SymbolNodeFactory.CheckFieldOffset(field));
Expand All @@ -2009,7 +2012,7 @@ private void EncodeFieldBaseOffset(FieldDesc field, CORINFO_FIELD_INFO* pResult,
{
PreventRecursiveFieldInlinesOutsideVersionBubble(field, callerMethod);

if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout)
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout && !callerMethod.IsNonVersionable() && (pResult->offset <= FieldFixupSignature.MaxCheckableOffset))
{
// ENCODE_CHECK_FIELD_OFFSET
_methodCodeNode.Fixups.Add(_compilation.SymbolNodeFactory.CheckFieldOffset(field));
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/vm/fieldmarshaler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ bool IsFieldBlittable(
}
else
{
isBlittable = !valueTypeHandle.GetMethodTable()->HasInstantiation() && valueTypeHandle.GetMethodTable()->IsBlittable();
isBlittable = valueTypeHandle.GetMethodTable()->IsBlittable();
}
break;
default:
Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/src/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13840,7 +13840,8 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,
DWORD baseOffset = CorSigUncompressData(pBlob);
DWORD fieldOffset = CorSigUncompressData(pBlob);
FieldDesc* pField = ZapSig::DecodeField(currentModule, pInfoModule, pBlob);
pField->GetEnclosingMethodTable()->CheckRestore();
MethodTable *pEnclosingMT = pField->GetApproxEnclosingMethodTable();
pEnclosingMT->CheckRestore();
DWORD actualFieldOffset = pField->GetOffset();
if (!pField->IsStatic() && !pField->IsFieldOfValueType())
{
Expand All @@ -13849,10 +13850,10 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,

DWORD actualBaseOffset = 0;
if (!pField->IsStatic() &&
pField->GetEnclosingMethodTable()->GetParentMethodTable() != NULL &&
!pField->GetEnclosingMethodTable()->IsValueType())
pEnclosingMT->GetParentMethodTable() != NULL &&
!pEnclosingMT->IsValueType())
{
actualBaseOffset = ReadyToRunInfo::GetFieldBaseOffset(pField->GetEnclosingMethodTable());
actualBaseOffset = ReadyToRunInfo::GetFieldBaseOffset(pEnclosingMT);
}

if ((fieldOffset != actualFieldOffset) || (baseOffset != actualBaseOffset))
Expand All @@ -13863,7 +13864,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,

SString fatalErrorString;
fatalErrorString.Printf(W("Verify_FieldOffset '%s.%s' %d!=%d || %d!=%d"),
GetFullyQualifiedNameForClassW(pField->GetEnclosingMethodTable()),
GetFullyQualifiedNameForClassW(pEnclosingMT),
ssFieldName.GetUnicode(),
fieldOffset,
actualFieldOffset,
Expand Down
10 changes: 10 additions & 0 deletions src/coreclr/src/zap/zapreadytorun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,3 +851,13 @@ static_assert_no_msg((int)READYTORUN_FIXUP_Verify_TypeLayout == (int)EN
//
static_assert_no_msg(sizeof(READYTORUN_EXCEPTION_LOOKUP_TABLE_ENTRY) == sizeof(CORCOMPILE_EXCEPTION_LOOKUP_TABLE_ENTRY));
static_assert_no_msg(sizeof(READYTORUN_EXCEPTION_CLAUSE) == sizeof(CORCOMPILE_EXCEPTION_CLAUSE));

//
// ReadyToRunHFAElemType
//
static_assert_no_msg((int)READYTORUN_HFA_ELEMTYPE_None == (int)CORINFO_HFA_ELEM_NONE);
static_assert_no_msg((int)READYTORUN_HFA_ELEMTYPE_Float32 == (int)CORINFO_HFA_ELEM_FLOAT);
static_assert_no_msg((int)READYTORUN_HFA_ELEMTYPE_Float64 == (int)CORINFO_HFA_ELEM_DOUBLE);
static_assert_no_msg((int)READYTORUN_HFA_ELEMTYPE_Vector64 == (int)CORINFO_HFA_ELEM_VECTOR64);
static_assert_no_msg((int)READYTORUN_HFA_ELEMTYPE_Vector128 == (int)CORINFO_HFA_ELEM_VECTOR128);

2 changes: 2 additions & 0 deletions src/coreclr/tests/src/CLRTest.CrossGen.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ if [ ! -z ${RunCrossGen2+x} ]%3B then
echo -r:$CORE_ROOT/System.*.dll>>$__ResponseFile
echo -r:$CORE_ROOT/Microsoft.*.dll>>$__ResponseFile
echo -r:$CORE_ROOT/mscorlib.dll>>$__ResponseFile
echo --verify-type-and-field-layout>>$__ResponseFile
echo --targetarch:$(TargetArchitecture)>>$__ResponseFile
echo -O>>$__ResponseFile

Expand Down Expand Up @@ -249,6 +250,7 @@ if defined RunCrossGen2 (
echo !__InputFile!>>!__ResponseFile!
echo -o:!__OutputFile!>>!__ResponseFile!
echo --targetarch:$(TargetArchitecture)>>!__ResponseFile!
echo --verify-type-and-field-layout>>!__ResponseFile!
echo -O>>!__ResponseFile!
echo -r:!CORE_ROOT!\System.*.dll>>!__ResponseFile!
echo -r:!CORE_ROOT!\Microsoft.*.dll>>!__ResponseFile!
Expand Down
56 changes: 56 additions & 0 deletions src/tests/readytorun/crossgen2/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1765,6 +1765,61 @@ private static bool ArrayLdtokenTests()
return true;
}

[StructLayout(LayoutKind.Explicit)]
struct ExplicitLayoutStruct16
{
[FieldOffset(0)]
public int x;
[FieldOffset(4)]
public int y;
[FieldOffset(8)]
public int z;
[FieldOffset(12)]
public int w;
public override string ToString() { return $"{x}{y}{z}{w}"; }
}
struct BlittableStruct<T>
{
public ExplicitLayoutStruct16 _explict;
public override string ToString() { return $"{_explict}"; }
}

struct StructWithGenericBlittableStruct
{
public BlittableStruct<short> _blittableGeneric;
public int _int;
public override string ToString() { return $"{_blittableGeneric}{_int}"; }
}

[MethodImpl(MethodImplOptions.AggressiveOptimization)]
private static bool TestWithStructureNonBlittableFieldDueToGenerics_StringCompare(ref StructWithGenericBlittableStruct input)
{
StructWithGenericBlittableStruct s = new StructWithGenericBlittableStruct();
s._blittableGeneric._explict.x = 1;
s._blittableGeneric._explict.y = 2;
s._blittableGeneric._explict.z = 3;
s._blittableGeneric._explict.w = 4;
s._int = 5;

Console.WriteLine(input);
Console.WriteLine(s);

return s.Equals(input) && input.ToString() == "12345";
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static bool TestWithStructureNonBlittableFieldDueToGenerics()
{
StructWithGenericBlittableStruct s = new StructWithGenericBlittableStruct();
s._blittableGeneric._explict.x = 1;
s._blittableGeneric._explict.y = 2;
s._blittableGeneric._explict.z = 3;
s._blittableGeneric._explict.w = 4;
s._int = 5;

return TestWithStructureNonBlittableFieldDueToGenerics_StringCompare(ref s);
}

public static int Main(string[] args)
{
_passedTests = new List<string>();
Expand Down Expand Up @@ -1832,6 +1887,7 @@ public static int Main(string[] args)
RunTest("GenericLdtokenTest", GenericLdtokenTest());
RunTest("ArrayLdtokenTests", ArrayLdtokenTests());
RunTest("TestGenericMDArrayBehavior", TestGenericMDArrayBehavior());
RunTest("TestWithStructureNonBlittableFieldDueToGenerics", TestWithStructureNonBlittableFieldDueToGenerics());

File.Delete(TextFileName);

Expand Down