From 880430a50be01155d3b3b752343ef45aae7b726a Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 8 Dec 2021 13:35:20 -0800 Subject: [PATCH] Update the auto-layout algorithm to better track alignment requirements. Fixes #12977 --- src/coreclr/debug/ee/debugger.cpp | 2 +- .../Common/MetadataFieldLayoutAlgorithm.cs | 32 ++++++-------- src/coreclr/vm/array.cpp | 2 +- src/coreclr/vm/managedmdimport.hpp | 3 -- src/coreclr/vm/methodtable.h | 4 -- src/coreclr/vm/methodtable.inl | 7 --- src/coreclr/vm/methodtablebuilder.cpp | 43 ++++++++++++++----- src/coreclr/vm/mlinfo.cpp | 2 +- src/coreclr/vm/object.cpp | 2 +- 9 files changed, 49 insertions(+), 48 deletions(-) diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index dde66e58d2eb1d..8e5200d37d5097 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -4360,7 +4360,7 @@ SIZE_T GetSetFrameHelper::GetValueClassSize(MetaSig* pSig) // - but we don't care if it's shared (since it will be the same size either way) _ASSERTE(!vcType.IsNull() && vcType.IsValueType()); - return (vcType.GetMethodTable()->GetAlignedNumInstanceFieldBytes()); + return (vcType.GetMethodTable()->GetNumInstanceFieldBytes()); } // diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 04c0aa883d8a38..24c949939e1d91 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -645,29 +645,14 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, // Place value class fields last for (int i = 0; i < instanceValueClassFieldsArr.Length; i++) { - // If the field has an indeterminate alignment, align the cumulative field offset to the indeterminate value - // Otherwise, align the cumulative field offset to the PointerSize - // This avoids issues with Universal Generic Field layouts whose fields may have Indeterminate sizes or alignments + // Align the cumulative field offset to the indeterminate value var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(instanceValueClassFieldsArr[i].FieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable); if (!fieldLayoutAbiStable) layoutAbiStable = false; - if (fieldSizeAndAlignment.Alignment.IsIndeterminate) - { - cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(type, cumulativeInstanceFieldPos, fieldSizeAndAlignment.Alignment, context.Target); - } - else - { - LayoutInt AlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, context.Target.LayoutPointerSize); - cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(type, cumulativeInstanceFieldPos, AlignmentRequired, context.Target); - } + cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(type, cumulativeInstanceFieldPos, fieldSizeAndAlignment.Alignment, context.Target); offsets[fieldOrdinal] = new FieldAndOffset(instanceValueClassFieldsArr[i], cumulativeInstanceFieldPos + offsetBias); - - // If the field has an indeterminate size, align the cumulative field offset to the indeterminate value - // Otherwise, align the cumulative field offset to the aligned-instance field size - // This avoids issues with Universal Generic Field layouts whose fields may have Indeterminate sizes or alignments - LayoutInt alignedInstanceFieldBytes = fieldSizeAndAlignment.Size.IsIndeterminate ? fieldSizeAndAlignment.Size : GetAlignedNumInstanceFieldBytes(fieldSizeAndAlignment.Size); - cumulativeInstanceFieldPos = checked(cumulativeInstanceFieldPos + alignedInstanceFieldBytes); + cumulativeInstanceFieldPos = checked(cumulativeInstanceFieldPos + fieldSizeAndAlignment.Size); fieldOrdinal++; } @@ -682,11 +667,18 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, } else if (cumulativeInstanceFieldPos.AsInt > context.Target.PointerSize) { - minAlign = context.Target.LayoutPointerSize; - if (requiresAlign8 && minAlign.AsInt == 4) + if (requiresAlign8) { minAlign = new LayoutInt(8); } + else if (type.ContainsGCPointers) + { + minAlign = context.Target.LayoutPointerSize; + } + else + { + minAlign = largestAlignmentRequired; + } } else { diff --git a/src/coreclr/vm/array.cpp b/src/coreclr/vm/array.cpp index a83c577b72c860..f08c256dadb515 100644 --- a/src/coreclr/vm/array.cpp +++ b/src/coreclr/vm/array.cpp @@ -660,7 +660,7 @@ MethodTable* Module::CreateArrayMethodTable(TypeHandle elemTypeHnd, CorElementTy } else if (index == 0) { - skip = pElemMT->GetAlignedNumInstanceFieldBytes() - numPtrsInBytes; + skip = pElemMT->GetNumInstanceFieldBytes() - numPtrsInBytes; } else { diff --git a/src/coreclr/vm/managedmdimport.hpp b/src/coreclr/vm/managedmdimport.hpp index ffc1efb2b7a64c..72e3703c376d46 100644 --- a/src/coreclr/vm/managedmdimport.hpp +++ b/src/coreclr/vm/managedmdimport.hpp @@ -28,9 +28,6 @@ typedef struct { I4Array * largeResult; int length; -#ifdef HOST_64BIT - int padding; -#endif int smallResult[16]; } MetadataEnumResult; diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index a56c575636318d..6e81b502e3df0f 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -1603,10 +1603,6 @@ class MethodTable inline WORD GetNumIntroducedInstanceFields(); - // Does this always return the same (or related) size as GetBaseSize()? - inline DWORD GetAlignedNumInstanceFieldBytes(); - - // Note: This flag MUST be available even from an unrestored MethodTable - see GcScanRoots in siginfo.cpp. DWORD ContainsPointers() { diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index 1ff15d02975e6a..d14a73532b6e12 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -181,13 +181,6 @@ inline WORD MethodTable::GetNumIntroducedInstanceFields() return(wNumFields); } -//========================================================================================== -inline DWORD MethodTable::GetAlignedNumInstanceFieldBytes() -{ - WRAPPER_NO_CONTRACT; - return((GetNumInstanceFieldBytes() + 3) & (~3)); -} - //========================================================================================== inline PTR_FieldDesc MethodTable::GetApproxFieldDescListRaw() { diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 615372a60ef3d6..52b41eceee6461 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8169,36 +8169,59 @@ VOID MethodTableBuilder::PlaceInstanceFields(MethodTable ** pByValueClassCach else dwNumGCPointerSeries = bmtParent->NumParentPointerSeries; + bool containsGCPointers = bmtFP->NumInstanceGCPointerFields > 0; // Place by value class fields last // Update the number of GC pointer series + // Calculate largest alignment requirement + int largestAlignmentRequirement = 1; for (i = 0; i < bmtEnumFields->dwNumInstanceFields; i++) { if (pFieldDescList[i].IsByValue()) { MethodTable * pByValueMT = pByValueClassCache[i]; - // value classes could have GC pointers in them, which need to be pointer-size aligned - // so do this if it has not been done already - #if !defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4) - dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, - (pByValueMT->GetNumInstanceFieldBytes() >= DATA_ALIGNMENT) ? DATA_ALIGNMENT : TARGET_POINTER_SIZE); -#else // !(!defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4)) -#ifdef FEATURE_64BIT_ALIGNMENT + if (pByValueMT->GetNumInstanceFieldBytes() >= DATA_ALIGNMENT) + { + dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, DATA_ALIGNMENT); + largestAlignmentRequirement = max(largestAlignmentRequirement, DATA_ALIGNMENT); + } + else +#elif defined(FEATURE_64BIT_ALIGNMENT) if (pByValueMT->RequiresAlign8()) + { dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, 8); + largestAlignmentRequirement = max(largestAlignmentRequirement, 8); + } else #endif // FEATURE_64BIT_ALIGNMENT + if (pByValueMT->ContainsPointers()) + { + // this field type has GC pointers in it, which need to be pointer-size aligned + // so do this if it has not been done already dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, TARGET_POINTER_SIZE); -#endif // !(!defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4)) + largestAlignmentRequirement = max(largestAlignmentRequirement, TARGET_POINTER_SIZE); + containsGCPointers = true; + } + else if (pByValueMT->HasLayout()) + { + int fieldAlignmentRequirement = pByValueMT->GetLayoutInfo()->m_ManagedLargestAlignmentRequirementOfAllMembers; + largestAlignmentRequirement = max(largestAlignmentRequirement, fieldAlignmentRequirement); + dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, fieldAlignmentRequirement); + } pFieldDescList[i].SetOffset(dwCumulativeInstanceFieldPos - dwOffsetBias); - dwCumulativeInstanceFieldPos += pByValueMT->GetAlignedNumInstanceFieldBytes(); + dwCumulativeInstanceFieldPos += pByValueMT->GetNumInstanceFieldBytes(); // Add pointer series for by-value classes dwNumGCPointerSeries += pByValueMT->ContainsPointers() ? (DWORD)CGCDesc::GetCGCDescFromMT(pByValueMT)->GetNumSeries() : 0; } + else + { + // non-value-type fields always require pointer alignment + largestAlignmentRequirement = max(largestAlignmentRequirement, TARGET_POINTER_SIZE); + } } // Can be unaligned @@ -8223,7 +8246,7 @@ VOID MethodTableBuilder::PlaceInstanceFields(MethodTable ** pByValueClassCach else #endif // FEATURE_64BIT_ALIGNMENT if (dwNumInstanceFieldBytes > TARGET_POINTER_SIZE) { - minAlign = TARGET_POINTER_SIZE; + minAlign = containsGCPointers ? TARGET_POINTER_SIZE : (unsigned)largestAlignmentRequirement; } else { minAlign = 1; diff --git a/src/coreclr/vm/mlinfo.cpp b/src/coreclr/vm/mlinfo.cpp index 69cba82dcf6f41..1dcc726213640f 100644 --- a/src/coreclr/vm/mlinfo.cpp +++ b/src/coreclr/vm/mlinfo.cpp @@ -2180,7 +2180,7 @@ MarshalInfo::MarshalInfo(Module* pModule, IfFailGoto(E_FAIL, lFail); } - UINT managedSize = m_pMT->GetAlignedNumInstanceFieldBytes(); + UINT managedSize = m_pMT->GetNumInstanceFieldBytes(); UINT nativeSize = 0; if ( nativeSize > 0xfff0 || diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index af2201cbf4dce6..6b64f72e5e2805 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -390,7 +390,7 @@ void STDCALL CopyValueClassArgUnchecked(ArgDestination *argDest, void* src, Meth if (argDest->IsHFA()) { - argDest->CopyHFAStructToRegister(src, pMT->GetAlignedNumInstanceFieldBytes()); + argDest->CopyHFAStructToRegister(src, pMT->GetNumInstanceFieldBytes()); return; }