Integrate value-class GC protection into GCFrame across CoreCLR and NativeAOT - #133730
Integrate value-class GC protection into GCFrame across CoreCLR and NativeAOT#133730jkoritzinsky with Copilot wants to merge 13 commits into
Conversation
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
What is the idea? |
| [StructLayout(LayoutKind.Sequential)] | ||
| internal unsafe struct GCFrameRegistration | ||
| { | ||
| #if CORECLR || NATIVEAOT |
There was a problem hiding this comment.
@copilot this file is only included on CoreCLR and NativeAOT. Please remove these guards (keep the NativeAOT-only guard for the using alias above)
There was a problem hiding this comment.
Removed the redundant guards while retaining the NativeAOT-only alias guard in 6a3f653.
There was a problem hiding this comment.
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
Pull request overview
Adds StackWalk contract version c2 to support reporting off-heap value-class GC roots (including byref-like inline arrays) from the GCFrame chain, aligning runtime structures, managed reader logic, and documentation.
Changes:
- Introduces
StackWalk_2and bumps the StackWalk contract version fromc1toc2. - Extends
GCFrameto support value-class payloads and addsValueClassInfodata descriptors (CoreCLR + NativeAOT + managed contracts). - Updates docs and unit tests around inline-array/byref-like handling.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/StackWalkTests.cs | Adds coverage for inline-array/byref-like helper logic used by StackWalk v2. |
| src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.cs | Updates expected StackWalk contract version to c2. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs | Adds ValueClassInfo to contract data types. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ValueClassInfo.cs | Adds contract type for VM ValueClassInfo nodes. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GCFrame.cs | Adds ValueClassInfoList pointer for GCFrame union arm. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEClass.cs | Adds VMFLAG_INLINE_ARRAY support via IsInlineArray. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs | Registers IStackWalk version c2 implementation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs | Implements StackWalk v2 GCFrame walking for value-class frames, byref-like recursion, and inline arrays. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_1.cs | Makes ReportGCFrameRoots overridable to enable v2 specialization. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs | Adds global GCFrameValueClassFlag for runtime-managed sync. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/GCFrameRegistration.cs | Updates GCFrameRegistration layout/flags and adds managed ValueClassInfo for CORECLR/NATIVEAOT. |
| src/coreclr/vm/reflectioninvocation.cpp | Switches value-class protection from ProtectValueClassFrame to value-class GCFrame. |
| src/coreclr/vm/frames.h | Adds value-class GCFrame union arm + CDAC fields; removes ProtectValueClassFrame. |
| src/coreclr/vm/frames.cpp | Implements GCFrame value-class constructor + scanning path; removes ProtectValueClassFrame scanning. |
| src/coreclr/vm/exceptionhandling.cpp | Removes ProtectValueClassFrame special casing (frame type removed). |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds CDAC ValueClassInfo, GCFrame.ValueClassInfoList, and GCFrameValueClassFlag; bumps StackWalk contract to c2. |
| src/coreclr/vm/corelib.h | Binds CoreLib GCFrameRegistration and ValueClassInfo fields to VM structures. |
| src/coreclr/vm/callhelpers.h | Updates comment to reflect value-class GCFrame usage. |
| src/coreclr/vm/FrameTypes.h | Removes ProtectValueClassFrame type entry. |
| src/coreclr/nativeaot/Runtime/thread.h | Extends NativeAOT GCFrameRegistration for value-class payloads + flags. |
| src/coreclr/nativeaot/Runtime/thread.cpp | Adds NativeAOT scanning for value-class GCFrame payloads via GC desc. |
| src/coreclr/nativeaot/Runtime/inc/MethodTable.h | Adds MethodTable::IsByRefLike() helper for NativeAOT. |
| src/coreclr/debug/ee/funceval.cpp | Replaces ProtectValueClassFrame with value-class GCFrame in func-eval path. |
| docs/design/datacontracts/data-descriptor-meanings.json | Documents new fields/types/global for value-class GCFrames and inline-array flag. |
| docs/design/datacontracts/StackWalk.md | Documents StackWalk contract c2 and new GCFrame behavior/fields. |
| docs/design/datacontracts/RuntimeTypeSystem.md | Documents VMFLAG_INLINE_ARRAY in EEClass.VMFlags. |
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 6
- Review effort level: Lite
| foreach ((uint offset, uint size) in rts.GetGCDescSeries(typeHandle)) | ||
| { | ||
| ulong unboxedOffset = offset - (uint)_target.PointerSize; | ||
| for (ulong innerOffset = 0; innerOffset < size; innerOffset += (uint)_target.PointerSize) | ||
| { | ||
| scanContext.GCReportCallback(info.Data + unboxedOffset + innerOffset, GcScanFlags.None); | ||
| } | ||
| } |
| Data.MethodTable canonicalMethodTable = _target.ProcessedData.GetOrAdd<Data.MethodTable>(canonicalMethodTablePointer); | ||
| eeClassPointer = canonicalMethodTable.EEClassOrCanonMT; | ||
| } | ||
|
|
| Frame, | ||
| GCFrame, | ||
| ValueClassInfo, | ||
| InlinedCallFrame, | ||
| SoftwareExceptionFrame, |
| protected override void ReportGCFrameRoots(ThreadData threadData, GcScanContext scanContext) | ||
| { | ||
| ulong pointerSize = (ulong)_target.PointerSize; | ||
| uint valueClassFlag = _target.ReadGlobal<uint>(Constants.Globals.GCFrameValueClassFlag); | ||
| HashSet<TargetPointer> seen = []; | ||
| TargetPointer pGCFrame = threadData.GCFrame; | ||
| while (pGCFrame != TargetPointer.Null) | ||
| { |
| GcScanContext scanContext, | ||
| int depth) | ||
| { | ||
| if (depth > MaxValueClassRecursionDepth) |
| if (elementSize == 0) | ||
| throw new InvalidOperationException("Inline array element size must be non-zero."); | ||
|
|
||
| for (uint repeatedOffset = 0; repeatedOffset < totalSize; repeatedOffset = checked(repeatedOffset + elementSize)) |
| private uint GetInlineArrayElementSize( | ||
| IRuntimeTypeSystem rts, | ||
| CorElementType fieldType, | ||
| ITypeHandle? nestedType) | ||
| { | ||
| if (fieldType == CorElementType.Byref) | ||
| return (uint)_target.PointerSize; | ||
|
|
||
| if (nestedType is not null) | ||
| return rts.GetNumInstanceFieldBytes(nestedType); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| private bool IsInlineArray(ITypeHandle typeHandle) | ||
| { | ||
| Data.MethodTable methodTable = _target.ProcessedData.GetOrAdd<Data.MethodTable>(typeHandle.Address); | ||
| TargetPointer eeClassPointer = methodTable.EEClassOrCanonMT; | ||
| if (MethodTableFlags_1.GetEEClassOrCanonMTBits(eeClassPointer) == MethodTableFlags_1.EEClassOrCanonMTBits.CanonMT) | ||
| { | ||
| TargetPointer canonicalMethodTablePointer = MethodTableFlags_1.UntagEEClassOrCanonMT(eeClassPointer); | ||
| Data.MethodTable canonicalMethodTable = _target.ProcessedData.GetOrAdd<Data.MethodTable>(canonicalMethodTablePointer); | ||
| eeClassPointer = canonicalMethodTable.EEClassOrCanonMT; | ||
| } | ||
|
|
||
| Data.EEClass eeClass = _target.ProcessedData.GetOrAdd<Data.EEClass>(eeClassPointer); | ||
| return eeClass.IsInlineArray; |
There was a problem hiding this comment.
@copilot these methods should be part of the RuntimeTypeSystem contract. Please move them there and ensure there are implementations that work for both CoreCLR and NativeAOT (adding new contract versions if they're currently the same version as the release/11.0 branch)
There was a problem hiding this comment.
Implemented in 443262a. StackWalk now obtains the inline-array queries through IRuntimeTypeSystem; CoreCLR advertises RuntimeTypeSystem c2, with regenerated contract documentation and coverage for the c2 registration.
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
There was a problem hiding this comment.
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 4
- Review effort level: Lite
| #if CORECLR || NATIVEAOT | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| internal unsafe struct ValueClassInfo | ||
| { |
| eeClassPointer = canonicalMethodTable.EEClassOrCanonMT; | ||
| } | ||
|
|
||
| Data.EEClass eeClass = _target.ProcessedData.GetOrAdd<Data.EEClass>(eeClassPointer); |
| [Field] public partial TargetPointer Next { get; } | ||
| [Field] public partial TargetPointer ObjRefs { get; } | ||
| [Field] public partial uint NumObjRefs { get; } | ||
| [Field] public partial TargetPointer ValueClassInfoList { get; } |
| internal static IEnumerable<uint> GetByRefLikeFieldOffsets( | ||
| bool isInlineArray, | ||
| uint fieldOffset, | ||
| uint elementSize, | ||
| uint totalSize) | ||
| { | ||
| if (!isInlineArray) | ||
| { | ||
| yield return fieldOffset; | ||
| yield break; | ||
| } | ||
|
|
||
| if (elementSize == 0) | ||
| throw new InvalidOperationException("Inline array element size must be non-zero."); | ||
|
|
||
| for (uint repeatedOffset = 0; repeatedOffset < totalSize; repeatedOffset = checked(repeatedOffset + elementSize)) | ||
| { | ||
| yield return checked(fieldOffset + repeatedOffset); | ||
| } | ||
| } |
Reflection Invoke only ever works with boxed types that are protected by the virtue of being boxed. How is this going to help with enabling value-type-specialized thunks for Reflection Invoke? |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings remain for byref-only protection, malformed metadata validation, and integration coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs:77
offsetcomes from target GC metadata, but subtracting the pointer size asuintwraps when a malformed series reports an offset smaller than the object header. The resultinginfo.Data + unboxedOffsetcan point at an unrelated address, andGCReportCallbackthen reads it; because GCFrame reporting runs after the per-frame exception handler, this can also abort the whole stack walk. Reject or otherwise safely handle offsets below the pointer size before doing the subtraction (and validate the resulting run bounds).
ulong unboxedOffset = offset - (uint)_target.PointerSize;
for (ulong innerOffset = 0; innerOffset < size; innerOffset += (uint)_target.PointerSize)
{
scanContext.GCReportCallback(info.Data + unboxedOffset + innerOffset, GcScanFlags.None);
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs:124
- The native scanner requires an inline-array field descriptor to have offset zero, but this path accepts any
offsetand only boundsrepeatedOffsetagainsttotalSize. A malformed target with a nonzero field offset therefore reportsinfo.Data + fieldOffset + repeatedOffset, potentially outside the value-class payload, andGCReportCallbackwill try to read those addresses. Reject invalid inline-array offsets (and check the field range) before callingGetByRefLikeFieldOffsets.
uint elementSize = isInlineArray
? GetInlineArrayElementSize(rts, fieldType, nestedType)
: 0;
if (isInlineArray && elementSize == 0)
continue;
foreach (uint fieldOffset in GetByRefLikeFieldOffsets(
isInlineArray,
offset,
elementSize,
totalSize))
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs:39
- The added tests cover only the two pure helper methods; no test constructs a c2
GCFramewith aValueClassInfolist and verifies the reported GCDesc/byref roots. The new traversal, canonical method-table handling, malformed metadata, and cycle behavior can therefore regress without failing these tests. Add a mock-target or dump-level integration test for this path.
ReportValueClassFrameRoots(pGCFrame, scanContext);
src/native/managed/cdac/tests/UnitTests/StackWalkTests.cs:23
- The added test only exercises the two pure helpers; the repository has no test that constructs a c2 target with the value-class GCFrame flag, a
ValueClassInfochain, and real method-table/field data, then verifies object and interior roots. As a result, the new discriminator/list traversal, GC descriptor offset adjustment, canonical method-table lookup, and nested byref-like path can regress while this test still passes. Add an integration-style unit or dump test covering those paths (including a malformed descriptor case).
[Fact]
public void ByRefLikeFieldHelpersHandleInlineArrays()
{
Assert.Equal([8u], StackWalk_2.GetByRefLikeFieldOffsets(isInlineArray: false, fieldOffset: 8, elementSize: 0, totalSize: 24));
Assert.Equal([0u, 8u, 16u], StackWalk_2.GetByRefLikeFieldOffsets(isInlineArray: true, fieldOffset: 0, elementSize: 8, totalSize: 24));
Assert.Throws<InvalidOperationException>(() => StackWalk_2.GetByRefLikeFieldOffsets(isInlineArray: true, fieldOffset: 0, elementSize: 0, totalSize: 24).ToArray());
- Files reviewed: 26/26 changed files
- Comments generated: 1
- Review effort level: Lite
| ValueClassInfo *pValueClasses = NULL; | ||
| GCFrame valueClassGCFrame(GetThread(), &pValueClasses); | ||
|
|
|
We bounced the idea of making TypedReferences actually useful that would allow things like Reflection Invoke to work with ref-like types and eliminate boxing. I doubt this refactoring would be useful for that feature. Also, it is a very complicated feature with unclear return. Source generators eliminated a lot of the scenarios where it would be valuable. |
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Two critical findings and three moderate StackWalk findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs:49
- The new value-class branch is not exercised by the added tests: coverage only checks inline-array offset helpers and RuntimeTypeSystem metadata queries, while the existing GCFrame dump test covers ordinary object-reference slots. Add a StackWalk integration test that builds a value-class GCFrame/list and verifies both embedded references and byref-like fields are reported, so regressions in discriminator/list decoding or unboxed offsets are caught.
if ((gcFrame.GCFlags & valueClassFlag) != 0)
{
ReportValueClassFrameRoots(pGCFrame, scanContext);
}
else
{
GcScanFlags flags = (GcScanFlags)gcFrame.GCFlags;
for (uint i = 0; i < gcFrame.NumObjRefs; i++)
{
TargetPointer slot = new(gcFrame.ObjRefs.Value + (ulong)i * pointerSize);
scanContext.GCReportCallback(slot, flags);
}
}
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs:74
GetGCDescSeriesreads the series offset from target metadata without guaranteeing that it is at least the target pointer size. If a malformed value-type descriptor returns a smaller offset, this subtraction wraps and the subsequent pointer arithmetic targets an unrelated address outside the value-class payload, causing the stack-reference walk to fail or report invalid data. Validate and skip an offset below the pointer size before normalizing it.
[!NOTE]
This review comment was generated by GitHub Copilot.
ulong unboxedOffset = offset - (uint)_target.PointerSize;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_2.cs:157
- When malformed metadata makes
totalSizenot an exact multiple ofelementSize, the< totalSizecondition still emits a final offset whose full element extends past the value-class payload (for example, a 10-byte payload with an 8-byte element emits offset 8). That can make cDAC report a root outside the value. Enumerate only offsets for which a complete element fits, and return no offsets when the element is larger than the payload.
[!NOTE]
This review comment was generated by GitHub Copilot.
for (uint repeatedOffset = 0; repeatedOffset < totalSize; repeatedOffset = checked(repeatedOffset + elementSize))
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Lite
|
|
||
| if (pProtectValueClassFrame != NULL) | ||
| pProtectValueClassFrame->Pop(pThread); | ||
| if (pValueClassGCFrame != NULL) |
| #if NATIVEAOT | ||
| using MethodTable = Internal.Runtime.MethodTable; | ||
| #endif |
|
I was thinking about if there was a case with byreflike, but you're right and there's not (since everything needs to be boxed anyway in this path). This is really for just enabling us to move more of the funceval logic to managed I guess. |
For non-byref-like value types, it is fine for funceval to box. There is so much overhead in funceval that a few boxes are nothing. For byref-like types, we do not have the right GC protect in place in funceval as far as I tell from the discussion on #126809 . To make it correct, I think we would need a GC handle like thing that can protect byref-like type stored in arbitrary memory (ie not on the stack). I think we are just getting lucky that there is not much opportunities for GC to run during funceval and so the crashes do not show up on the radar. If we were to switch more of funceval to managed, it will likely increase likelihood of GC running and our luck may run out. The pre-requisite for switching more of funceval to managed is to understand and fix lurking reliability problems like this. |
|
My understanding is the ProtectValueClassFrame already handles this today. This PR doing 2 things:
If we have no desire to move the portion of funceval that uses ProtectValueClassFrame up to managed ever, then the second part of this change can be reverted. |
I am asking about FuncEval end-to-end.
I suspect that once we get rid of the reflection invoke interpreter and figure out how the FuncEval needs to work to be reliable, |
|
I'm fine holding off on this if we can eventually just delete ProtectValueClassFrame instead. |
Summary
Move off-heap value-class GC protection from the explicit
Framehierarchy to the per-threadGCFramechain in CoreCLR and extend NativeAOT's correspondingGCFrameRegistrationmechanism. The registration uses a size-neutral discriminated pointer union for either ordinary object-reference slots or aValueClassInfo**list head, avoiding stack-walk-only frame handling while preserving the existing frame footprint.This work enables value-type-specialized thunks in #133662 and allows more func-eval logic to move to managed code than #133659 does today.
Changes
ProtectValueClassFrameand its explicit-frame stack-walking and exception-unwind handling.GCFrame::GcScanRoots, including embedded object references and byref-like fields.GCFrame.GCFrameRegistrationandValueClassInfolayouts, including typedMethodTable*metadata and NativeAOT availability.DEFINE_CLASS_UandDEFINE_FIELD_Uchecks to keep CoreCLR's managed/native layouts synchronized.GCFrameRegistrationlayout with the same discriminated value-class list representation.CGCDescmetadata for unboxed value types containing managed object references, while retaining existing object/byref registration behavior.ValueClassInfolist.Validation
./build.sh clr+libs+host— succeeded../build.sh clr.aot+libs -rc Debug -lc Release— succeeded with 0 warnings and 0 errors.Note
This pull request title and description were generated by GitHub Copilot.