Remove target ifdefs from dbi#130935
Conversation
Introduces IDacDbiInterface::GetTargetInfo, which reports the target's processor architecture and OS family, along with the TargetInfo/TargetArchitecture/TargetOperatingSystem types. Includes the native DAC implementation and the managed cDAC implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
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 fewTARGET_*→HOST_*fixes). - Remove the DAC-side platform mismatch check that compared
ICorDebugDataTarget::GetPlatformto 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
There was a problem hiding this comment.
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
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/debug/di/shimremotedatatarget.cpp:250
- ShimRemoteDataTarget::GetPlatform now unconditionally returns E_NOTIMPL. This breaks callers that require a CorDebugPlatform to compute target pointer size (e.g. DataTargetReader::GetRemotePointerSize calls ICorDebugDataTarget::GetPlatform and fails fast on error), so remote/transport debugging paths will regress. If cross-platform/arch remote debugging still isn't supported here, the previous host-platform mapping can be retained but switched to HOST_* defines to avoid TARGET_* usage.
ShimRemoteDataTarget::GetPlatform(
CorDebugPlatform *pPlatform)
{
return E_NOTIMPL;
}
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
| if (pThread != NULL) | ||
| { | ||
| // Remember that we've hijacked the thread. | ||
| SetState(CUTS_GenericHijacked); | ||
| HRESULT hr = S_OK; | ||
| EX_TRY |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "05147ef8784bb01ebfffcd948df6561daa8d06c8",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "34d8d6692375b818c6f6da9915c8507516bd175f",
"last_reviewed_commit": "05147ef8784bb01ebfffcd948df6561daa8d06c8",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "34d8d6692375b818c6f6da9915c8507516bd175f",
"last_recorded_worker_run_id": "29688117571",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "05147ef8784bb01ebfffcd948df6561daa8d06c8",
"review_id": 4730816575
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. DBI (mscordbi) is a host-side component that should not bake target architecture/OS assumptions into TARGET_* #ifdefs, since a single DBI build services multiple targets via DAC. Converting genuine host-only decisions to HOST_* and moving genuine target decisions to a runtime DAC query is the correct direction and complements the register/context work in #130367.
Approach: Sound. A new IDacDbiInterface::GetTargetInfo (arch/OS/pointer-size) is added on both the native DAC and the cDAC managed legacy impl (with a DEBUG cross-check against the legacy DAC), and the RS caches the result per-process. The large rsthread.cpp rewrites faithfully translate the previous per-arch #ifdef ladders into switch (targetInfo.arch) logic, and dead code (SaveRaiseExceptionEntryContext, cross-platform platform-mismatch check) is removed.
Summary: ShimRemoteDataTarget::GetPlatform change to E_NOTIMPL (inline), which may regress remote metadata pointer-size resolution; a maintainer familiar with the pipe-transport remote debugging path should confirm it is dead. Everything else looks correct and consistent.
Detailed Findings
⚠️ Behavioral change — ShimRemoteDataTarget::GetPlatform returns E_NOTIMPL
See the inline comment on src/coreclr/debug/di/shimremotedatatarget.cpp. The previous implementation returned a valid CorDebugPlatform; it now fails unconditionally. DataTargetReader::GetRemotePointerSize IfFailRets on GetPlatform, and that reader is reachable from CordbModule remote in-memory metadata reads via CreateRemoteMDInternalRWSource(GetProcess()->GetDataTarget(), ...). If that path is live for pipe-transport remote targets, this is a regression; if it is dead, the change is fine. Please confirm.
✅ GetTargetInfo implementations — consistent and defensive
The native DAC impl (dacdbiimpl.cpp) and the cDAC managed impl (DacDbiImpl.cs) both null-check the out pointer and map every supported arch/OS. The cDAC side folds Apple into Unix, matching the native side where Apple targets build with TARGET_UNIX, and gates a full field-by-field equality assertion against the legacy DAC under #if DEBUG. Pointer size derives from _target.PointerSize rather than a hardcoded constant. This is a good pattern and the mappings line up with the native enum ordering.
✅ rsthread.cpp arch-switch translations — faithful
Spot-checked the non-trivial rewrites: GetLocalRegisterValue now computes a 0-based floatingPointIndex before calling GetLocalFloatingPointValue (whose internal per-arch index subtraction was correspondingly removed), and the VLT_REG_FP/VLT_FPSTK call sites were updated to pass already-normalized indices (e.g. AMD64 subtracts REGISTER_AMD64_XMM0, Arm64/LoongArch64/RiscV64 pass vlrReg directly, x86 FPSTK passes vlfReg). The x87 R4/R8 workaround and m_floatStackTop adjustment remain correctly gated to x86. FabricateNativeInfo replaces cbArchitectureMin with targetInfo.pointerSize, preserving the x86 stack-growth-down vs. up split.
💡 Minor — AlignAddressForType now unconditional on FEATURE_64BIT_ALIGNMENT
In rspriv.h, AlignAddressForType previously required TARGET_ARM in addition to FEATURE_64BIT_ALIGNMENT and swallowed failures with an assert; it now runs for any FEATURE_64BIT_ALIGNMENT build and IfFailThrows. This looks intentional and more correct (host DBI should not assume the ARM target), and RequiresAlign8 is now always compiled. No action needed, just flagging the widened scope for confirmation.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 235.8 AIC · ⌖ 11.4 AIC · ⊞ 10K
| #endif // FEATURE_DBGIPC_TRANSPORT_DI | ||
|
|
||
| #if defined(TARGET_UNIX) || defined(__ANDROID__) | ||
| #if defined(HOST_UNIX) || defined(__ANDROID__) |
There was a problem hiding this comment.
Previously a cross targetted windows -> unix build wouldn't have been marked as supporting local debugging but now it is. That doesn't seem right.
| #if defined(TARGET_X86) || defined(TARGET_AMD64) | ||
| #endif // FEATURE_INTEROP_DEBUGGING | ||
| #if defined(HOST_X86) || defined(HOST_AMD64) | ||
| const ULONG_PTR breakpointOpcodeSize = 1; |
There was a problem hiding this comment.
Defining a breakpoint opcode size based on host architecture and then relying on code elsewhere to only use it in situations where target==host seems error prone. How about we define a helper function that maps target_arch -> breakpoint size to ensure code doesn't inadvertently use the wrong breakpoint size in a cross-architecture debugging scenario.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/debug/di/process.cpp:12765
- RestoreLeafSeh uses REMOTE_PTR (host pointer width) to write back the SEH chain head. Since this is now guarded by targetInfo.arch at runtime, it can be reached for x86 targets where the target pointer size may not match the host. Add a pointer-size guard to avoid writing an incorrectly sized pointer into the target TEB.
IDacDbiInterface::TargetInfo targetInfo;
IfFailThrow(GetTargetInfo(&targetInfo));
if (targetInfo.arch == IDacDbiInterface::kArchX86)
{
hr = pUnmanagedThread->RestoreLeafSeh();
_ASSERTE(SUCCEEDED(hr));
}
- Files reviewed: 18/18 changed files
- Comments generated: 2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5764
- Consider initializing the output struct to a known default value before populating individual fields. As written, if an exception occurs after setting
Archbut beforeOS/PointerSize, callers could observe partially-updated data even when a failure HRESULT is returned.
if (pTargetInfo is null)
throw new ArgumentException("Output pointer cannot be null.", nameof(pTargetInfo));
Contracts.IRuntimeInfo runtimeInfo = _target.Contracts.RuntimeInfo;
pTargetInfo->Arch = runtimeInfo.GetTargetArchitecture() switch
- Files reviewed: 18/18 changed files
- Comments generated: 3
| default: _ASSERTE(!"bad size"); | ||
| } |
| default: _ASSERTE(!"bad size"); | ||
| } |
| HRESULT hr = S_OK; | ||
| EX_TRY | ||
| { | ||
| if (!m_fHasCachedTargetInfo) | ||
| { | ||
| IfFailThrow(GetDAC()->GetTargetInfo(&m_cachedTargetInfo)); | ||
| m_fHasCachedTargetInfo = true; | ||
| } | ||
|
|
||
| *pTargetInfo = m_cachedTargetInfo; | ||
| } | ||
| EX_CATCH_HRESULT(hr); |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/debug/di/rspriv.h:11400
- AlignAddressForType now unconditionally calls CordbType::RequiresAlign8. On targets where FEATURE_64BIT_ALIGNMENT is not enabled, the DAC side (DacDbiInterfaceImpl::RequiresAlign8) throws E_NOTIMPL, causing IfFailThrow here to throw unexpectedly during varargs argument walking. This helper should be a no-op unless the target architecture actually uses FEATURE_64BIT_ALIGNMENT (currently ARM and WASM).
inline void AlignAddressForType(CordbType* pArgType, CORDB_ADDRESS& argBase)
{
BOOL align = FALSE;
IfFailThrow(pArgType->RequiresAlign8(&align));
- Files reviewed: 18/18 changed files
- Comments generated: 2
| case IDacDbiInterface::kArchX86: | ||
| case IDacDbiInterface::kArchAMD64: | ||
| *pcbSize = 1; | ||
| break; |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/debug/di/process.cpp:8174
GetTargetOpcodeSizedoesn’t handlekArchWasm, even thoughIDacDbiInterface::TargetArchitectureincludes it anddebug/inc/wasm/primitives.hdefines a 1-byte breakpoint instruction. As-is, any path that needs opcode sizing on Wasm will hit the default case and returnE_NOTIMPL.
switch (targetInfo.arch)
{
case IDacDbiInterface::kArchX86:
case IDacDbiInterface::kArchAMD64:
*pcbSize = 1;
break;
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
Target ifdefs in DBI generally fall into four categories: