Skip to content

Commit

Permalink
[release/7.0-rc1] Disable Int128 use in by value ABI scenarios, and f…
Browse files Browse the repository at this point in the history
…ix field layout behavior (#74279)

* First stab at support for proper 128bit integer layout and abi

* Add ABI tests for Int128 covering interesting scenarios

* Fix bugs so that at least Windows Arm64 works

* Add more types to the ABI tester, so that we cover the Int128 scenarios

* Revert changes which attempted to enable by value passing for Int128

* Make Int128 have layout match the expected unmanaged field layout - On Unix platforms (64 bit) use 16 byte alignment - On Arm32 use 8 byte alignment matching the 128 byte vector type - On other Windows platforms the 128 bit integer type isn't defined by the C compiler, but match the behavior of other 128 bit types (16 byte alignment)

Add tests to call down to native that should pass with these rules

- Disable use of Int128 as p/invoke parameter passed by value

* Mark Int128 types as not having a stable abi - This disables use of these types for parameter passing in R2R images

* Address all known issues

* Try to fix PR job

* Should fix the test issues

* Apply suggestions from code review

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>

Co-authored-by: David Wrighton <davidwr@microsoft.com>
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
  • Loading branch information
3 people committed Aug 20, 2022
1 parent 1af9cfd commit d9c1700
Show file tree
Hide file tree
Showing 36 changed files with 758 additions and 51 deletions.
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/mscorrc.rc
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ BEGIN
IDS_EE_BADMARSHAL_ABSTRACTRETCRITICALHANDLE "Returned CriticalHandles cannot be abstract."
IDS_EE_BADMARSHAL_CUSTOMMARSHALER "Custom marshalers are only allowed on classes, strings, arrays, and boxed value types."
IDS_EE_BADMARSHAL_GENERICS_RESTRICTION "Non-blittable generic types cannot be marshaled."
IDS_EE_BADMARSHAL_INT128_RESTRICTION "System.Int128 and System.UInt128 cannot be passed by value to unmanaged."
IDS_EE_BADMARSHAL_AUTOLAYOUT "Structures marked with [StructLayout(LayoutKind.Auto)] cannot be marshaled."
IDS_EE_BADMARSHAL_STRING_OUT "Cannot marshal a string by-value with the [Out] attribute."
IDS_EE_BADMARSHAL_MARSHAL_DISABLED "Cannot marshal managed types when the runtime marshalling system is disabled."
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@
#define IDS_EE_BADMARSHAL_ABSTRACTOUTCRITICALHANDLE 0x1a63
#define IDS_EE_BADMARSHAL_RETURNCHCOMTONATIVE 0x1a64
#define IDS_EE_BADMARSHAL_CRITICALHANDLE 0x1a65
#define IDS_EE_BADMARSHAL_INT128_RESTRICTION 0x1a66

#define IDS_EE_BADMARSHAL_ABSTRACTRETCRITICALHANDLE 0x1a6a
#define IDS_EE_CH_IN_VARIANT_NOT_SUPPORTED 0x1a6b
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/inc/readytorun.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,18 @@
#define READYTORUN_SIGNATURE 0x00525452 // 'RTR'

// Keep these in sync with src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs
#define READYTORUN_MAJOR_VERSION 0x0007
#define READYTORUN_MINOR_VERSION 0x0001
#define READYTORUN_MAJOR_VERSION 0x0008
#define READYTORUN_MINOR_VERSION 0x0000

#define MINIMUM_READYTORUN_MAJOR_VERSION 0x006
#define MINIMUM_READYTORUN_MAJOR_VERSION 0x008

// R2R Version 2.1 adds the InliningInfo section
// R2R Version 2.2 adds the ProfileDataInfo section
// R2R Version 3.0 changes calling conventions to correctly handle explicit structures to spec.
// R2R 3.0 is not backward compatible with 2.x.
// R2R Version 6.0 changes managed layout for sequential types with any unmanaged non-blittable fields.
// R2R 6.0 is not backward compatible with 5.x or earlier.
// R2R Version 8.0 Changes the alignment of the Int128 type

struct READYTORUN_CORE_HEADER
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ struct ReadyToRunHeaderConstants
{
static const uint32_t Signature = 0x00525452; // 'RTR'

static const uint32_t CurrentMajorVersion = 7;
static const uint32_t CurrentMinorVersion = 1;
static const uint32_t CurrentMajorVersion = 8;
static const uint32_t CurrentMinorVersion = 0;
};

struct ReadyToRunHeader
Expand Down
10 changes: 7 additions & 3 deletions src/coreclr/tools/Common/Compiler/Int128FieldLayoutAlgorithm.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ public override ComputedInstanceFieldLayout ComputeInstanceLayout(DefType defTyp

ComputedInstanceFieldLayout layoutFromMetadata = _fallbackAlgorithm.ComputeInstanceLayout(defType, layoutKind);

if (defType.Context.Target.IsWindows || (defType.Context.Target.PointerSize == 4))
// 32bit platforms use standard metadata layout engine
if (defType.Context.Target.Architecture == TargetArchitecture.ARM)
{
layoutFromMetadata.LayoutAbiStable = false; // Int128 parameter passing ABI is unstable at this time
layoutFromMetadata.IsInt128OrHasInt128Fields = true;
return layoutFromMetadata;
}

Expand All @@ -42,7 +45,8 @@ public override ComputedInstanceFieldLayout ComputeInstanceLayout(DefType defTyp
FieldAlignment = new LayoutInt(16),
FieldSize = layoutFromMetadata.FieldSize,
Offsets = layoutFromMetadata.Offsets,
LayoutAbiStable = true
LayoutAbiStable = false, // Int128 parameter passing ABI is unstable at this time
IsInt128OrHasInt128Fields = true
};
}

Expand Down Expand Up @@ -72,7 +76,7 @@ public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristi
public static bool IsIntegerType(DefType type)
{
return type.IsIntrinsic
&& type.Namespace == "System."
&& type.Namespace == "System"
&& ((type.Name == "Int128") || (type.Name == "UInt128"));
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ internal struct ReadyToRunHeaderConstants
{
public const uint Signature = 0x00525452; // 'RTR'

public const ushort CurrentMajorVersion = 7;
public const ushort CurrentMinorVersion = 1;
public const ushort CurrentMajorVersion = 8;
public const ushort CurrentMinorVersion = 0;
}

#pragma warning disable 0169
Expand Down
23 changes: 23 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/DefType.FieldLayout.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ private static class FieldLayoutFlags
/// True if the type transitively has any types with LayoutKind.Auto in its layout.
/// </summary>
public const int IsAutoLayoutOrHasAutoLayoutFields = 0x400;

/// <summary>
/// True if the type transitively has an Int128 in it or is an Int128
/// </summary>
public const int IsInt128OrHasInt128Fields = 0x800;
}

private class StaticBlockInfo
Expand Down Expand Up @@ -135,6 +140,20 @@ public virtual bool IsAutoLayoutOrHasAutoLayoutFields
}
}

/// <summary>
/// Is a type Int128 or transitively have any fields of a type Int128.
/// </summary>
public virtual bool IsInt128OrHasInt128Fields
{
get
{
if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedInstanceTypeLayout))
{
ComputeInstanceLayout(InstanceLayoutKind.TypeAndFields);
}
return _fieldLayoutFlags.HasFlags(FieldLayoutFlags.IsInt128OrHasInt128Fields);
}
}

/// <summary>
/// The number of bytes required to hold a field of this type
Expand Down Expand Up @@ -430,6 +449,10 @@ public void ComputeInstanceLayout(InstanceLayoutKind layoutKind)
{
_fieldLayoutFlags.AddFlags(FieldLayoutFlags.IsAutoLayoutOrHasAutoLayoutFields);
}
if (computedLayout.IsInt128OrHasInt128Fields)
{
_fieldLayoutFlags.AddFlags(FieldLayoutFlags.IsInt128OrHasInt128Fields);
}

if (computedLayout.Offsets != null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public struct ComputedInstanceFieldLayout
public LayoutInt ByteCountAlignment;
public bool LayoutAbiStable; // Is the layout stable such that it can safely be used in function calling conventions
public bool IsAutoLayoutOrHasAutoLayoutFields;
public bool IsInt128OrHasInt128Fields;

/// <summary>
/// If Offsets is non-null, then all field based layout is complete.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ out instanceByteSizeAndAlignment
FieldSize = sizeAndAlignment.Size,
LayoutAbiStable = true,
IsAutoLayoutOrHasAutoLayoutFields = false,
IsInt128OrHasInt128Fields = false,
};

if (numInstanceFields > 0)
Expand Down Expand Up @@ -211,7 +212,7 @@ public override ComputedStaticFieldLayout ComputeStaticFieldLayout(DefType defTy
}

ref StaticsBlock block = ref GetStaticsBlockForField(ref result, field);
SizeAndAlignment sizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout: false, context.Target.DefaultPackingSize, out bool _, out bool _);
SizeAndAlignment sizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout: false, context.Target.DefaultPackingSize, out bool _, out bool _, out bool _);

block.Size = LayoutInt.AlignUp(block.Size, sizeAndAlignment.Alignment, context.Target);
result.Offsets[index] = new FieldAndOffset(field, block.Size);
Expand Down Expand Up @@ -303,15 +304,18 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty
int fieldOrdinal = 0;
bool layoutAbiStable = true;
bool hasAutoLayoutField = false;
bool hasInt128Field = type.BaseType == null ? false : type.BaseType.IsInt128OrHasInt128Fields;

foreach (var fieldAndOffset in layoutMetadata.Offsets)
{
TypeDesc fieldType = fieldAndOffset.Field.FieldType;
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType.UnderlyingType, hasLayout: true, packingSize, out bool fieldLayoutAbiStable, out bool fieldHasAutoLayout);
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType.UnderlyingType, hasLayout: true, packingSize, out bool fieldLayoutAbiStable, out bool fieldHasAutoLayout, out bool fieldHasInt128Field);
if (!fieldLayoutAbiStable)
layoutAbiStable = false;
if (fieldHasAutoLayout)
hasAutoLayoutField = true;
if (fieldHasInt128Field)
hasInt128Field = true;

largestAlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequired);

Expand Down Expand Up @@ -357,6 +361,7 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty
ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout
{
IsAutoLayoutOrHasAutoLayoutFields = hasAutoLayoutField,
IsInt128OrHasInt128Fields = hasInt128Field,
};
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
computedLayout.FieldSize = instanceSizeAndAlignment.Size;
Expand Down Expand Up @@ -392,17 +397,20 @@ protected ComputedInstanceFieldLayout ComputeSequentialFieldLayout(MetadataType
int packingSize = ComputePackingSize(type, layoutMetadata);
bool layoutAbiStable = true;
bool hasAutoLayoutField = false;
bool hasInt128Field = type.BaseType == null ? false : type.BaseType.IsInt128OrHasInt128Fields;

foreach (var field in type.GetFields())
{
if (field.IsStatic)
continue;

var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType.UnderlyingType, hasLayout: true, packingSize, out bool fieldLayoutAbiStable, out bool fieldHasAutoLayout);
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType.UnderlyingType, hasLayout: true, packingSize, out bool fieldLayoutAbiStable, out bool fieldHasAutoLayout, out bool fieldHasInt128Field);
if (!fieldLayoutAbiStable)
layoutAbiStable = false;
if (fieldHasAutoLayout)
hasAutoLayoutField = true;
if (fieldHasInt128Field)
hasInt128Field = true;

largestAlignmentRequirement = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequirement);

Expand All @@ -424,6 +432,7 @@ protected ComputedInstanceFieldLayout ComputeSequentialFieldLayout(MetadataType
ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout
{
IsAutoLayoutOrHasAutoLayoutFields = hasAutoLayoutField,
IsInt128OrHasInt128Fields = hasInt128Field,
};
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
computedLayout.FieldSize = instanceSizeAndAlignment.Size;
Expand Down Expand Up @@ -459,6 +468,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
int instanceValueClassFieldCount = 0;
int instanceGCPointerFieldsCount = 0;
int[] instanceNonGCPointerFieldsCount = new int[maxLog2Size + 1];
bool hasInt128Field = false;

foreach (var field in type.GetFields())
{
Expand All @@ -471,6 +481,8 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
{
// Valuetypes which are not primitives or enums
instanceValueClassFieldCount++;
if (((DefType)fieldType).IsInt128OrHasInt128Fields)
hasInt128Field = true;
}
else if (fieldType.IsGCPointer)
{
Expand All @@ -480,7 +492,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
{
Debug.Assert(fieldType.IsPrimitive || fieldType.IsPointer || fieldType.IsFunctionPointer || fieldType.IsEnum || fieldType.IsByRef);

var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out bool _, out bool _);
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out bool _, out bool _, out bool _);
instanceNonGCPointerFieldsCount[CalculateLog2(fieldSizeAndAlignment.Size.AsInt)]++;
}
}
Expand Down Expand Up @@ -517,7 +529,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,

TypeDesc fieldType = field.FieldType;

var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable, out bool _);
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable, out bool _, out bool _);
if (!fieldLayoutAbiStable)
layoutAbiStable = false;

Expand Down Expand Up @@ -678,7 +690,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
for (int i = 0; i < instanceValueClassFieldsArr.Length; i++)
{
// Align the cumulative field offset to the indeterminate value
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(instanceValueClassFieldsArr[i].FieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable, out bool _);
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(instanceValueClassFieldsArr[i].FieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable, out bool _, out bool _);
if (!fieldLayoutAbiStable)
layoutAbiStable = false;

Expand Down Expand Up @@ -729,6 +741,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout
{
IsAutoLayoutOrHasAutoLayoutFields = true,
IsInt128OrHasInt128Fields = hasInt128Field,
};
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
computedLayout.FieldSize = instanceSizeAndAlignment.Size;
Expand All @@ -742,7 +755,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,

private static void PlaceInstanceField(FieldDesc field, bool hasLayout, int packingSize, FieldAndOffset[] offsets, ref LayoutInt instanceFieldPos, ref int fieldOrdinal, LayoutInt offsetBias)
{
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType, hasLayout, packingSize, out bool _, out bool _);
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType, hasLayout, packingSize, out bool _, out bool _, out bool _);

instanceFieldPos = AlignUpInstanceFieldOffset(field.OwningType, instanceFieldPos, fieldSizeAndAlignment.Alignment, field.Context.Target);
offsets[fieldOrdinal] = new FieldAndOffset(field, instanceFieldPos + offsetBias);
Expand Down Expand Up @@ -802,11 +815,12 @@ public LayoutInt CalculateFieldBaseOffset(MetadataType type, bool requiresAlign8
return cumulativeInstanceFieldPos;
}

private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType, bool hasLayout, int packingSize, out bool layoutAbiStable, out bool fieldTypeHasAutoLayout)
private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType, bool hasLayout, int packingSize, out bool layoutAbiStable, out bool fieldTypeHasAutoLayout, out bool fieldTypeHasInt128Field)
{
SizeAndAlignment result;
layoutAbiStable = true;
fieldTypeHasAutoLayout = true;
fieldTypeHasInt128Field = false;

if (fieldType.IsDefType)
{
Expand All @@ -817,6 +831,7 @@ private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType,
result.Alignment = defType.InstanceFieldAlignment;
layoutAbiStable = defType.LayoutAbiStable;
fieldTypeHasAutoLayout = defType.IsAutoLayoutOrHasAutoLayoutFields;
fieldTypeHasInt128Field = defType.IsInt128OrHasInt128Fields;
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,12 @@ internal static MarshallerKind GetMarshallerKind(
return MarshallerKind.Invalid;
}

if (!isField && InteropTypes.IsInt128Type(context, type))
{
// Int128 types cannot be passed by value
return MarshallerKind.Invalid;
}

if (isBlittable)
{
if (nativeType != NativeTypeKind.Default && nativeType != NativeTypeKind.Struct)
Expand Down Expand Up @@ -887,7 +893,7 @@ internal static MarshallerKind GetDisabledMarshallerKind(
else if (underlyingType.IsValueType)
{
var defType = (DefType)underlyingType;
if (!defType.ContainsGCPointers && !defType.IsAutoLayoutOrHasAutoLayoutFields)
if (!defType.ContainsGCPointers && !defType.IsAutoLayoutOrHasAutoLayoutFields && !defType.IsInt128OrHasInt128Fields)
{
return MarshallerKind.BlittableValue;
}
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Interop/InteropTypes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ public static bool IsSystemRuntimeIntrinsicsVector64T(TypeSystemContext context,
return IsCoreNamedType(context, type, "System.Runtime.Intrinsics", "Vector64`1");
}

public static bool IsInt128Type(TypeSystemContext context, TypeDesc type)
{
return IsCoreNamedType(context, type, "System", "Int128") || IsCoreNamedType(context, type, "System", "UInt128");
}

public static bool IsSystemRuntimeIntrinsicsVector128T(TypeSystemContext context, TypeDesc type)
{
return IsCoreNamedType(context, type, "System.Runtime.Intrinsics", "Vector128`1");
Expand Down
Loading

0 comments on commit d9c1700

Please sign in to comment.