[cDAC] Implement RequiresAlign8 for cDAC

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷

[barosiak]

Summary

Implement RequiresAlign8 on DacDbiImpl in the cDAC.

Changes

• MethodTableFlags_1.cs - added RequiresAlign8 flag and property
• IRuntimeTypeSystem.cs / RuntimeTypeSystem_1.cs - added RequiresAlign8 contract method
• DacDbiImpl.cs - RequiresAlign8 implementation with #if DEBUG legacy validation
• methodtable.h - added [cDAC] contract annotation on native flag
• RuntimeTypeSystem.md - updated contract design doc
• Tests - new tests covering flag-set/flag-unset across architecture

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Implements RequiresAlign8 support end-to-end for the cDAC RuntimeTypeSystem contract and exposes it through the legacy DacDbiImpl surface area.

Changes:

• Added a RequiresAlign8 flag/property to the MethodTableFlags_1 helper and exposed it via IRuntimeTypeSystem + RuntimeTypeSystem_1.
• Implemented DacDbiImpl.RequiresAlign8 using the cDAC RuntimeTypeSystem contract, with #if DEBUG validation against the legacy DAC.
• Updated native flag annotations and the RuntimeTypeSystem contract design doc; added tests for flag set/unset across standard architectures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MethodTableTests.cs	Adds coverage for IRuntimeTypeSystem.RequiresAlign8 behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements RequiresAlign8 via cDAC with DEBUG cross-check against legacy DAC.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs	Introduces the RequiresAlign8 flag and accessor on method table flags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs	Exposes RequiresAlign8 on the v1 RuntimeTypeSystem contract implementation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs	Adds the RequiresAlign8 contract API.
src/coreclr/vm/methodtable.h	Annotates the native flag as contract-dependent for cDAC.
docs/design/datacontracts/RuntimeTypeSystem.md	Documents the new contract API/flag and updates the sample implementation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[github-actions]

🤖 Copilot Code Review — PR #126928

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified — the cDAC needs to expose the RequiresAlign8 flag from MethodTable to support the debugger's alignment queries on ARM and WASM targets. This is a straightforward gap in the existing cDAC contract surface.

Approach: Correct — the implementation follows the established pattern used by ContainsGCPointers, IsString, IsCollectible, and other boolean flag readers. The layering (flag enum → flags helper → contract implementation → DacDbiImpl bridge → design doc) is thorough and consistent.

Summary: ⚠️ Needs Human Review. The implementation is correct in the core contract layer and well-tested. One minor inconsistency in DacDbiImpl error handling (flagged by multiple models) should be evaluated by a reviewer.

---

Detailed Findings

⚠️GetTargetArchitecture() Outside try/catch — DacDbiImpl.cs:644

_target.Contracts.RuntimeInfo.GetTargetArchitecture() is called at line 644 before the try block. If this call throws (e.g., the target's architecture global string is missing or malformed), the exception will propagate uncaught instead of being converted to an HRESULT return value. This is inconsistent with sibling methods like IsValueType (line 688-714) where all contract resolution is inside the try/catch.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

Suggested fix — move the call inside the try block:

publicintRequiresAlign8(ulongthExact,Interop.BOOL*pResult){*pResult=Interop.BOOL.FALSE;inthr=HResults.S_OK;try{RuntimeInfoArchitecturearch=_target.Contracts.RuntimeInfo.GetTargetArchitecture();// Some 32-bit platform ABIs require 64-bit alignment (FEATURE_64BIT_ALIGNMENT).if(arch==RuntimeInfoArchitecture.Arm||arch==RuntimeInfoArchitecture.Wasm){
...

The native DAC's equivalent function uses DD_ENTER_MAY_THROW which wraps the entire body in exception handling (dacdbiimpl.cpp:5780), so moving this inside try would be more consistent.

Severity: Advisory — GetTargetArchitecture() is unlikely to throw in practice since architecture is fundamental infrastructure, but the pattern inconsistency is worth fixing.

✅ Flag Value Correctness — Verified

The flag value 0x00800000 in MethodTableFlags_1.WFLAGS_HIGH.RequiresAlign8 matches the native enum_flag_RequiresAlign8 in src/coreclr/vm/methodtable.h:3822. The native flag is conditionally compiled under #ifdef FEATURE_64BIT_ALIGNMENT, but the cDAC correctly defines it unconditionally since it's a cross-platform reader — the bit is simply never set on non-ARM/WASM targets.

Flagged by: all three models

✅ Architecture Guard in DacDbiImpl — Correct

The check arch == RuntimeInfoArchitecture.Arm || arch == RuntimeInfoArchitecture.Wasm precisely mirrors the native compile-time guard in src/coreclr/inc/switches.h:153-154:

#if defined(TARGET_ARM) || defined(TARGET_WASM)
#defineFEATURE_64BIT_ALIGNMENT
#endif

On unsupported architectures, throw new NotImplementedException() is caught and its HResult (E_NOTIMPL = 0x80004001, verified at src/libraries/System.Private.CoreLib/src/System/NotImplementedException.cs:20) is returned, matching the native DAC behavior at src/coreclr/debug/daccess/dacdbiimpl.cpp:5792 (ThrowHR(E_NOTIMPL)).

Note on the existing Copilot AI review comment on this PR: That comment claims NotImplementedException.HResult is 0x80131515. This is incorrect — 0x80131515 is COR_E_NOTSUPPORTED (NotSupportedException). NotImplementedException.HResult is 0x80004001 (E_NOTIMPL), so the current code is correct and the DEBUG validation will pass. That comment also suggests adding Armv6 to the check, but src/coreclr/vm/datadescriptor/datadescriptor.inc:1314 always reports ARM targets as "arm" (not "armv6"), so the current check is correct.

Flagged by: Claude Opus 4.6, GPT-5.3-codex

✅ Pattern Consistency — Matches Codebase

The implementation mirrors ContainsGCPointers and IsString exactly:

• Interface default: throw new NotImplementedException() ✅
• Contract: !typeHandle.IsMethodTable() ? false : _methodTables[...].Flags.RequiresAlign8 ✅
• DacDbiImpl: try/catch with #if DEBUG legacy validation ✅
• Non-MethodTable TypeHandles return false (consistent with all similar methods) ✅
• x86 FEATURE_DOUBLE_ALIGNMENT_HINT is a separate mechanism and does not define/use RequiresAlign8 ✅

Flagged by: all three models

✅ Test Quality — Adequate

The test uses [Theory] with [MemberData(nameof(StdArchBool))] generating 8 test cases (4 architectures × 2 boolean states). The flagSet=true case correctly combines Category_ValueType | RequiresAlign8, which is a realistic scenario since RequiresAlign8 is typically set on value types containing 64-bit primitive fields. The flagSet=false case reuses SystemObjectMethodTable which naturally doesn't have the flag.

✅ Design Documentation — Accurate

The RuntimeTypeSystem.md updates are consistent with both the interface definition and the implementation.

💡 DacDbiImpl Integration Tests — Follow-up

The DacDbiImpl bridge layer's architecture-specific logic (ARM/WASM → read flag, other → E_NOTIMPL) is not directly tested by the unit test, which tests only the contract layer. Integration-level tests could be a useful follow-up but this is consistent with existing coverage for other DacDbiImpl methods.

Flagged by: Claude Sonnet 4.5, GPT-5.3-codex

---

Models contributing to this review: Claude Opus 4.6 (primary), Claude Sonnet 4.5 (sub-agent), GPT-5.3-codex (sub-agent).

Generated by Code Review for issue #126928 · ◷