Skip to content

Commit

Permalink
Disable Int128 use in by value ABI scenarios, and fix field layout be…
Browse files Browse the repository at this point in the history
…havior (#74123)

Unfortunately, attempting to actually fix all the ABI issues is probably too complex for this time in the release cycle.

- Mark `Int128` as being ABI unstable in Crossgen2. This will prevent it from appearing in function signatures
- Adjust layout so that crossgen2 and the runtime agree about layout
  - Arm32 - Alignment of 8 bytes
  - Everywhere else - Alignment of 16 bytes
- Disable use of `Int128` in pinvokes as a by value parameter or return value. (Match behavior of `Vector128<T>`) (Unlike Vector128<T> handle scenarios such as having fields of `Int128` types.)
- Disable tests that will fail now that pinvokes are disabled
- Build a test that will succeed now that pinvokes are disabled
- Add test that alignment matches OS behavior
- Add unit tests for alignment behavior
- Update R2R version

Fixes #72206
  • Loading branch information
davidwrighton authored Aug 19, 2022
1 parent 33164ff commit e6b5e67
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 e6b5e67

Please sign in to comment.