Remove target ifdefs from dbi

[rcj1]

Target ifdefs in DBI generally fall into four categories:

• Should actually be a host ifdef - changed to host ifdef
• Genuinely represents target information - used DacDbi API GetTargetInfo instead of ifdef
• Interop debugging - host and target are the same, also changed to host ifdef
• Anything register/context-related - dealt with in [cDAC] Remove DT_CONTEXT from DBI layer #130367

[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

This PR introduces a new DBI API (GetTargetInfo) to query target architecture / OS (and pointer size on the native side) and replaces many TARGET_* conditionals in the RS/shim code with runtime target queries (or HOST_* where appropriate).

Changes:

• Add IDacDbiInterface::GetTargetInfo (IDL + headers + DAC implementation) and a managed implementation in cDAC Legacy.
• Replace target-ifdef-based logic in RS/shim with GetTargetInfo-based branching (plus a few TARGET_* → HOST_* fixes).
• Remove the DAC-side platform mismatch check that compared ICorDebugDataTarget::GetPlatform to the DAC build platform.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs	Adds managed GetTargetInfo and managed TargetInfo/enums.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements managed GetTargetInfo via IRuntimeInfo.
src/coreclr/inc/dacdbi.idl	Adds native TargetInfo struct and GetTargetInfo method to the IDL.
src/coreclr/debug/inc/dacdbiinterface.h	Adds TargetInfo (arch/os/pointerSize) and GetTargetInfo to the native interface definition.
src/coreclr/debug/di/valuehome.cpp	Uses target info to replace TARGET_64BIT conditionals for 8-byte register values; removes some target-ifdef blocks.
src/coreclr/debug/di/shimstackwalk.cpp	Uses target info instead of TARGET_ARM* ifdefs for SP adjustment.
src/coreclr/debug/di/shimremotedatatarget.cpp	Implements GetPlatform based on queried target info; switches an ARM64 proc-mem workaround to HOST_ARM64.
src/coreclr/debug/di/shimprocess.cpp	Wires the ShimProcess pointer into the data target instance.
src/coreclr/debug/di/shimpriv.h	Adds a forward declaration needed by new target-info usage.
src/coreclr/debug/di/shimlocaldatatarget.cpp	Converts several TARGET_* checks to HOST_* for local data target behavior.
src/coreclr/debug/di/shimdatatarget.h	Adds ShimProcess backpointer plumbing for shim data targets.
src/coreclr/debug/di/rstype.cpp	Makes CordbType::RequiresAlign8 unconditional (was FEATURE_64BIT_ALIGNMENT-guarded).
src/coreclr/debug/di/rsthread.cpp	Replaces many TARGET_* ifdefs with target-info-driven logic for frame matching, hijacks, FP regs, arg walking, etc.
src/coreclr/debug/di/rspriv.h	Adds declarations/fields for target info caching; adjusts some host/target ifdefs; tweaks alignment helper.
src/coreclr/debug/di/rsmain.cpp	Converts TARGET_UNIX to HOST_UNIX in privilege code.
src/coreclr/debug/di/process.cpp	Adds cached CordbProcess::GetTargetInfo that calls through to the DAC; converts several TARGET_* to HOST_*.
src/coreclr/debug/di/platformspecific.cpp	Switches platform-specific includes to HOST_*.
src/coreclr/debug/di/module.cpp	Converts a couple TARGET_UNIX checks to HOST_UNIX.
src/coreclr/debug/di/cordb.cpp	Converts TARGET_UNIX to HOST_UNIX for local-debugging support gating.
src/coreclr/debug/daccess/dacdbiimpl.h	Declares DacDbiInterfaceImpl::GetTargetInfo.
src/coreclr/debug/daccess/dacdbiimpl.cpp	Implements DacDbiInterfaceImpl::GetTargetInfo using target build macros and pointer-size macro.
src/coreclr/debug/daccess/daccess.cpp	Removes the DAC-side host/target platform mismatch verification block.

Copilot's findings

• Files reviewed: 22/22 changed files
• Comments generated: 5

[Copilot]

Copilot's findings

• Files reviewed: 22/22 changed files
• Comments generated: 8

[Copilot]

Copilot's findings

• Files reviewed: 22/22 changed files
• Comments generated: 6

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/debug/daccess/daccess.cpp:5124

• ClrDataAccess::Initialize removed the platform-mismatch validation (GetPlatform + CORDBG_E_INCOMPATIBLE_PLATFORMS). There doesn’t appear to be a replacement check in daccess/, so a mismatched DAC/target combination could proceed further and fail later with harder-to-diagnose memory read errors/crashes instead of a clear incompatibility HRESULT.

HRESULT
ClrDataAccess::Initialize(void)
{
    HRESULT hr;
    CLRDATA_ADDRESS base = { 0 };

    // Get the current DLL base for mscorwks globals.
    // In case of multiple-CLRs, there may be multiple dlls named "mscorwks".
    // code:OpenVirtualProcess can take the base address (clrInstanceId) to select exactly
    // which CLR to is being target. If so, m_globalBase will already be set.
    //

    if (m_globalBase == 0)
    {
        // Caller didn't specify which CLR to debug, we should be using a legacy data target.
        if (m_pLegacyTarget == NULL)
        {
            DacError(E_INVALIDARG);
            UNREACHABLE();
        }

        ReleaseHolder<ICLRRuntimeLocator> pRuntimeLocator(NULL);
        if (m_pLegacyTarget->QueryInterface(__uuidof(ICLRRuntimeLocator), (void**)&pRuntimeLocator) != S_OK || pRuntimeLocator->GetRuntimeBase(&base) != S_OK)
        {
            IfFailRet(m_pLegacyTarget->GetImageBase(TARGET_MAIN_CLR_DLL_NAME_W, &base));
        }

        m_globalBase = TO_TADDR(base);
    }

• Files reviewed: 22/22 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 22/22 changed files
• Comments generated: 5

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8174

• GetTargetOpcodeSize doesn’t handle kArchWasm, even though IDacDbiInterface::TargetArchitecture includes it and debug/inc/wasm/primitives.h defines a 1-byte breakpoint instruction. As-is, any path that needs opcode sizing on Wasm will hit the default case and return E_NOTIMPL.

    switch (targetInfo.arch)
    {
        case IDacDbiInterface::kArchX86:
        case IDacDbiInterface::kArchAMD64:
            *pcbSize = 1;
            break;

• Files reviewed: 18/18 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (2)

src/coreclr/debug/di/cordb.cpp:474

• Same issue as the corresponding DbiGetThreadContext guard above: Unix builds should keep the local-debugging exclusion (previously via SUPPORT_LOCAL_DEBUGGING) so this doesn't try to use PAL SetThreadContext for remote scenarios.

{
#if !defined(HOST_ARM) || defined(FEATURE_DBGIPC_TRANSPORT_DI) || defined(__ANDROID__)
    _ASSERTE(!"Can't use local GetThreadContext remotely, this needed to go to datatarget");
    return FALSE;

src/coreclr/debug/di/rspriv.h:11400

• AlignAddressForType used to be a true no-op except on platforms that need 8-byte argument alignment (historically ARM32 via FEATURE_64BIT_ALIGNMENT). After this change it calls into DAC for every non-x86 target, which adds avoidable overhead (and can now throw) on architectures where this alignment adjustment is unnecessary. Consider gating the alignment computation on the target architecture so the helper stays a no-op for the common case.

    BOOL align = FALSE;
    IfFailThrow(pArgType->RequiresAlign8(&align));

    if (align)
        argBase = ALIGN_ADDRESS(argBase, 8);

• Files reviewed: 18/18 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (3)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5799

• GetTargetInfo writes to an output pointer but does not initialize the output struct on failure paths (exceptions are caught and converted to HRESULT). This can leave the caller with partially-initialized data if an exception occurs mid-way; initialize the output to default once the pointer is validated.

            if (pTargetInfo is null)
                throw new ArgumentException("Output pointer cannot be null.", nameof(pTargetInfo));
            Contracts.IRuntimeInfo runtimeInfo = _target.Contracts.RuntimeInfo;

src/coreclr/debug/di/valuehome.cpp:212

• In release builds, the pointer-size invariant in the 8-byte case is only asserted, so if this path is ever hit on a non-64-bit target it will silently proceed and truncate/produce incorrect results. Consider turning the pointer-size check into a real failure (and keep the assert as a debug aid).

                IfFailThrow(m_pFrame->GetProcess()->GetTargetInfo(&targetInfo));
                _ASSERTE(targetInfo.pointerSize == 8);
                extendedVal = (SSIZE_T) *(INT64*)newValue.StartAddress();
                break;

src/coreclr/debug/di/valuehome.cpp:235

• In release builds, the pointer-size invariant in the 8-byte case is only asserted, so if this path is ever hit on a non-64-bit target it will silently proceed and truncate/produce incorrect results. Consider turning the pointer-size check into a real failure (and keep the assert as a debug aid).

                IfFailThrow(m_pFrame->GetProcess()->GetTargetInfo(&targetInfo));
                _ASSERTE(targetInfo.pointerSize == 8);
                extendedVal = (SIZE_T) *(UINT64*)newValue.StartAddress();
                break;

• Files reviewed: 18/18 changed files
• Comments generated: 0 new

[noahfalk]

The change looks reasonable to me - I think my broader concern now is timing and the risk to the .NET 11 release making changes of this scope that could impact non-cDAC scenarios. We are now less than 2 weeks from the RC1 snap. If there were bugs in here that our internal testing didn't catch we might not hear about it until the bits are shipped in mid-September at which point the bug fix bar + schedule constraints probably mean it stays broken at release and servicing fixes go out early 2027.

I'm going to propose that we should start holding these larger DBI refactors until we've set up a place where we can build and ship cDAC/DBI out-of-band from the runtime. This avoids the risk that we accidentally break our non-cDAC debugging scenarios and uncouples the non-contract portion of the work from the .NET 11 runtime schedule. We should probably discuss this a bit more as a team to decide how we proceed.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5832

• In the DEBUG parity check, the legacy call always uses a non-null local buffer. If this method returns E_INVALIDARG due to a null pTargetInfo, hrLocal will be S_OK (because &targetInfoLocal is non-null) and Debug.ValidateHResult will assert even though both implementations would return E_INVALIDARG for a null output pointer.

Consider passing a null pointer through to the legacy implementation when pTargetInfo is null, while still using a local buffer for the successful case, so the parity check compares like-for-like HRESULTs.

        if (_legacy is not null)
        {
            TargetInfo targetInfoLocal;
            int hrLocal = _legacy.GetTargetInfo(&targetInfoLocal);
            Debug.ValidateHResult(hr, hrLocal);
            if (hr == HResults.S_OK)

• Files reviewed: 18/18 changed files
• Comments generated: 0 new

[Copilot]

Review details

Suppressed comments (4)

src/coreclr/debug/di/cordb.cpp:507

• Same issue as the DbiGetThreadContext guard above: this local SetThreadContext path should remain disabled on Unix to avoid calling unsupported local thread-context APIs.

#if !defined(HOST_ARM) || defined(FEATURE_DBGIPC_TRANSPORT_DI) || defined(__ANDROID__)
    _ASSERTE(!"Can't use local GetThreadContext remotely, this needed to go to datatarget");
    return FALSE;

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs:974

• This change adds new public API surface (GetTargetInfo plus TargetArchitecture/TargetOperatingSystem/TargetInfo). The PR metadata provided doesn’t reference an api-approved issue. If this API is intended to be publicly consumable, it should go through the API approval process; otherwise consider making the new API internal to avoid expanding the public surface area.

    [PreserveSig]
    int GetTargetInfo(TargetInfo* pTargetInfo);
}

public enum TargetArchitecture
{
    Unknown = 0,
    X86,
    AMD64,
    Arm,
    Arm64,
    LoongArch64,
    RiscV64,
    Wasm,
}

public enum TargetOperatingSystem
{
    Unknown = 0,
    Windows,
    Unix,
}

[StructLayout(LayoutKind.Sequential)]
public struct TargetInfo
{
    public TargetArchitecture Arch;
    public TargetOperatingSystem OS;
    public uint PointerSize;

src/coreclr/debug/di/cordb.cpp:468

• The local-debugging guard for ARM thread context helpers no longer blocks Unix targets; this used to be disabled for Unix because local GetThreadContext/SetThreadContext isn’t supported there. With the current condition, a Unix build without FEATURE_DBGIPC_TRANSPORT_DI could try to use the local path. Consider reintroducing a Unix host/target check here (and in the matching DbiSetThreadContext block below).

    // if we aren't local debugging this isn't going to work
#if !defined(HOST_ARM) || defined(FEATURE_DBGIPC_TRANSPORT_DI) || defined(__ANDROID__)
    _ASSERTE(!"Can't use local GetThreadContext remotely, this needed to go to datatarget");
    return FALSE;

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:6434

• This new HRESULT-returning method writes to an output pointer but doesn’t initialize it before doing work. If an exception is thrown after partially writing fields, the caller may observe partially-initialized data. Consider zero-initializing *pTargetInfo after the null check so it’s defined on all paths.

            if (pTargetInfo is null)
                throw new ArgumentException("Output pointer cannot be null.", nameof(pTargetInfo));
            Contracts.IRuntimeInfo runtimeInfo = _target.Contracts.RuntimeInfo;

• Files reviewed: 30/30 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5799

• GetTargetInfo writes into an output pointer but doesn’t initialize the TargetInfo struct before populating fields. If an exception occurs after partially writing (e.g., contract access throws), callers can observe partially-initialized output despite the failure HRESULT. Initialize the output struct to default immediately after validating the pointer.

            if (pTargetInfo is null)
                throw new ArgumentException("Output pointer cannot be null.", nameof(pTargetInfo));
            Contracts.IRuntimeInfo runtimeInfo = _target.Contracts.RuntimeInfo;

• Files reviewed: 18/18 changed files
• Comments generated: 0 new
• Review effort level: Lite