Skip to content

Implement convertPInvokeCalliToCall for CoreCLR - #131654

Merged
jkoritzinsky merged 19 commits into
mainfrom
dev/jkoritzinsky/calli-pinvoke
Aug 24, 2026
Merged

Implement convertPInvokeCalliToCall for CoreCLR#131654
jkoritzinsky merged 19 commits into
mainfrom
dev/jkoritzinsky/calli-pinvoke

Conversation

@jkoritzinsky

@jkoritzinskyjkoritzinsky commented Jul 31, 2026

Copy link
Copy Markdown
Member

Replaces CoreCLR's lazy, call-time marshalling mechanism for unmanaged calli with the
convertPInvokeCalliToCall JIT-EE path that NativeAOT already uses.

Background

An unmanaged calli that needed marshalling used to compile into a call to
CORINFO_HELP_PINVOKE_CALLI. At run time GenericPInvokeCalliHelper — hand-written assembly on
every architecture — pushed a PInvokeCalliFrame, called GenericPInvokeCalliStubWorker, and
generated the marshalling IL stub from a VASigCookie on first call. The unmanaged target was
smuggled through a secret register, shifted <<1|1 on 64-bit.

Now the stub MethodDesc is created while the caller is being JIT-compiled, cached by signature
in the IL stub cache exactly as before, and the stub is JIT-compiled on first call. The unmanaged
target is passed as an ordinary trailing native int parameter.

Failures are reported when the stub is called

Because the stub is built while its caller is jitted, a failure detected while building it would
otherwise fail the compilation of a method that may never execute the call site.

Marshalling failures were already reported per parameter through
StubHelpers.ThrowInteropParamException. That is extended to the failures which are not tied to a
parameter, so the whole set — a signature that needs marshalling, a non-blittable generic
instantiation, an unsupported calling convention, and an instance this the runtime cannot
express — becomes a stub body that throws when it is called. Classification records the failure and
returns a usable calling convention so stub creation runs to completion, and FinishEmit discards
the marshalling it produced in favour of the throw.

Only a signature that is not a standalone method signature at all — one carrying the generic bit or
an unknown calling convention nibble, from which no stub signature can be built — still fails
eagerly with COR_E_BADIMAGEFORMAT.

One case is a genuine behavior change: an unmanaged calli whose signature carries an instance
this is not expressible as a static stub and the JIT will not expand it inline. Previously the
runtime declined to build a stub, which — once the helper fallback was gone — would have let the
JIT emit a plain managed indirect call to an unmanaged target. It now reports
InvalidProgramException / VLDTR_E_FMD_PINVOKENOTSTATIC, matching a P/Invoke declared on an
instance method. Native member functions are unaffected: they are static signatures whose first
parameter is the this pointer.

Removed

GenericPInvokeCalliHelper on amd64/arm/arm64/i386/loongarch64/riscv64/wasm,
GenericPInvokeCalliStubWorker, PInvokeCalliFrame with its stack-walking and data-descriptor
support, and the PInvoke-calli cookie mechanism in the JIT: GenTreeCall::gtCallCookie,
WellKnownArg::PInvokeCookie/PInvokeTarget with their argument registers on all seven
architectures, GTF_ICON_PINVKI_HDL, the fgMorphArgs rewrite into the helper call,
LowerIndirectNonvirtCall, and eeConvertToLookup.

Nothing then referenced GetCookieForPInvokeCalliSig or CORINFO_HELP_PINVOKE_CALLI, so both are
dropped from the JIT-EE interface and the JIT-EE version GUID is bumped. The generated files
were regenerated with ThunkGenerator/gen.bat. CORINFO_HELP_PINVOKE_CALLI is not a ReadyToRun
helper, so removing it does not version the ReadyToRun format — R2R images encode
READYTORUN_HELPER_* ids, which are unaffected.

ReadyToRun

ReadyToRun never fell back to the runtime helper: GetCookieForPInvokeCalliSig threw
RequiresRuntimeJitException, so the cookie call was only ever an abort that left the whole
method to the runtime JIT. crossgen2 now implements convertPInvokeCalliToCall and throws for
exactly the call sites the JIT cannot expand inline — mustConvert, a fastcall convention, an
unmanaged signature carrying an instance this, or a signature that needs marshalling.

Verified equivalent: an assembly exercising blittable stdcall / platform-default /
SuppressGCTransition / managed / marshalling / fastcall calli was crossgen2'd before and after.
The set of precompiled methods and their exact code sizes are byte-identical, and
MarshallingCalli / FastcallCalli are left to the runtime JIT in both.

Debugger

ILStubManager::TraceManager can no longer read the target from the removed hidden argument. It
locates the trailing target parameter with MetaSig/ArgIterator and reads it from the context
captured at the stub's entry point — from the argument register the calling convention assigned, or
from the incoming stack arguments. The register mapping is implemented for x86/amd64/arm/arm64 and
returns FALSE (no trace) elsewhere, matching the existing StubManagerHelpers::GetFirstArg
precedent.

Two latent fixes worth reviewing independently

In CallConv::TryGetUnmanagedCallingConventionFromModOptSigStartingAtRetType, both pre-existing and
both required for generic calli stubs whose signatures are converted to module-independent form:

  • It skipped optionalELEMENT_TYPE_CMOD_INTERNAL modifiers, inverted relative to the token
    path. ConvertToInternalExactlyOne encodes CMOD_OPT as required = 0 and CallConv* modopts
    are always optional, so the branch was dead.
  • It computed tokenLookupModule but passed pModule to GetNameOfTypeRefOrDef, producing a
    spurious InvalidProgramException for delegate* unmanaged<U,T>.

These change behavior for any module-independent signature carrying calling-convention modopts.

Validation

windows-x64 Checked:

  • Interop tree: 333 total, 321 passed, 10 failed — the 10 are pre-existing out-of-proc COM and
    DisabledRuntimeMarshalling environment failures, unchanged from baseline.
  • JIT/Directed tree: 686 total, 679 passed, 0 failed, 7 skipped.
  • All 13 JIT/Directed/pinvoke and JIT/Directed/callconv.cmd tests pass.
  • Ad hoc coverage: marshalling / blittable / tiered calli, nested layout classes, fastcall,
    collectible ALC unload (5/5 assemblies collected), and a hand-written IL matrix of
    calli instance unmanaged variants.
  • The ArgIterator target recovery was checked against a synthetic entry-point context for targets
    landing in RDX, R8, R9 and the first three stack slots.

Not built or executed locally: every non-windows-x64 target. The assembly deletions, the
non-x64 argument-register mappings in the stub manager, and the non-x64 target-header edits are
covered by inspection only.

Fixes#127473
Fixes#131606 in main (though this fix is probably not one we want to backport)

Note

This pull request description was generated by GitHub Copilot.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates CoreCLR to use the JIT-EE convertPInvokeCalliToCall path for unmanaged calli, aligning behavior with NativeAOT by creating/caching an IL stub at JIT-time (instead of the legacy runtime helper + per-arch assembly mechanism).

Changes:

  • Implement CEEInfo::convertPInvokeCalliToCall and plumb VM support to create CALLI IL stubs and defer/report stub-generation failures at stub-execution time.
  • Remove the legacy GenericPInvokeCalliHelper / cookie-based JIT path and associated helper/assembly plumbing across architectures, plus related frame/cDAC entries.
  • Update debugger stub tracing to recover the unmanaged target from the new trailing native int argument.

Reviewed changes

Copilot reviewed 71 out of 71 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppRemoves wasm placeholder for the deleted GenericPInvokeCalliHelper.
src/coreclr/vm/stubmgr.hRemoves mention of GenericPInvokeCalliHelper from comments.
src/coreclr/vm/stubmgr.cppUpdates IL stub tracing to locate CALLI target from the last argument (not hidden arg); removes helper tracing paths.
src/coreclr/vm/stubhelpers.hAdds QCall entrypoint declaration for throwing deferred interop exceptions.
src/coreclr/vm/stubhelpers.cppAdds QCall implementation for deferred interop exception throwing; removes old CALLI target mangling.
src/coreclr/vm/riscv64/pinvokestubs.SDeletes CALLI helper stub macro usage and associated “secret” mangling logic.
src/coreclr/vm/qcallentrypoints.cppRegisters new StubHelpers_ThrowInteropException QCall entrypoint.
src/coreclr/vm/mlinfo.cppDefers CALLI-stub marshal failures to runtime (records exception info instead of throwing during stub generation).
src/coreclr/vm/loongarch64/pinvokestubs.SDeletes CALLI helper stub macro usage and associated “secret” mangling logic.
src/coreclr/vm/jitinterface.cppImplements convertPInvokeCalliToCall; marks IL stubs as non-inlineable.
src/coreclr/vm/i386/cgenx86.cppRemoves x86 PInvokeCalliFrame regdisplay support (frame removed).
src/coreclr/vm/i386/asmhelpers.SRemoves x86 GenericPInvokeCalliHelper assembly helper.
src/coreclr/vm/i386/asmhelpers.asmRemoves x86 GenericPInvokeCalliHelper assembly helper and worker import.
src/coreclr/vm/FrameTypes.hRemoves PInvokeCalliFrame from frame type list.
src/coreclr/vm/frames.hRemoves PInvokeCalliFrame definition and updates CALLI documentation comments.
src/coreclr/vm/frames.cppRemoves PInvokeCalliFrame promotion/logging and constructor.
src/coreclr/vm/dllimport.hAdds VM API for creating CALLI IL stubs; adds stub linker state for deferred exceptions and target-arg index.
src/coreclr/vm/dllimport.cppAdds CALLI IL stub creation pipeline and deferred-throw stub generation; refactors vararg stub worker path.
src/coreclr/vm/datadescriptor/datadescriptor.incRemoves cDAC descriptor for the deleted PInvokeCalliFrame.
src/coreclr/vm/corelib.hAdds binder mapping for StubHelpers.ThrowInteropException.
src/coreclr/vm/cgensys.hRemoves GenericPInvokeCalli* extern declarations.
src/coreclr/vm/ceeload.hAdds Module::GetLoaderModuleForSignature for standalone-signature-owned artifacts.
src/coreclr/vm/ceeload.cppImplements GetLoaderModuleForSignature and reuses it from GetVASigCookie.
src/coreclr/vm/callconvbuilder.cppFixes module-independent signature modopt parsing for unmanaged calling conventions.
src/coreclr/vm/arm64/pinvokestubs.SDeletes CALLI helper stub macro usage and associated “secret” mangling logic.
src/coreclr/vm/arm64/PInvokeStubs.asmDeletes CALLI helper stub macro usage and associated “secret” mangling logic.
src/coreclr/vm/arm/pinvokestubs.SDeletes CALLI helper stub macro usage.
src/coreclr/vm/amd64/pinvokestubs.SDeletes GenericPInvokeCalliHelper implementation (Unix asm).
src/coreclr/vm/amd64/PInvokeStubs.asmDeletes GenericPInvokeCalliHelper implementation (Windows asm).
src/coreclr/vm/amd64/asmconstants.hRemoves now-unused pinvoke-calli target register constants.
src/coreclr/tools/superpmi/superpmi/icorjitinfo.cppRemoves SuperPMI interception for deleted JIT-EE method GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cppRemoves shim forwarding for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cppRemoves shim forwarding/counter for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cppRemoves recording for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.hRemoves recording/replay surface for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cppRemoves recording/replay implementation for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shared/lwmlist.hRemoves LWM map entry for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/superpmi/superpmi-shared/agnostic.hRemoves serialized key struct for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txtRemoves deleted JIT-EE interface method from thunk generator input.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csRemoves managed-side implementation stub for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.csRemoves generated callback plumbing for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.csRemoves helper id CORINFO_HELP_PINVOKE_CALLI.
src/coreclr/tools/aot/jitinterface/jitinterface_generated.hRemoves AOT wrapper method for deleted GetCookieForPInvokeCalliSig.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csRemoves setting hClass for converted calli stub (RyuJit AOT side).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.csImplements ReadyToRun behavior for convertPInvokeCalliToCall (throws to force runtime JIT when needed).
src/coreclr/System.Private.CoreLib/src/System/StubHelpers.csAdds managed QCall declaration for ThrowInteropException.
src/coreclr/jit/wellknownargs.hRemoves well-known args for the deleted cookie/target register convention.
src/coreclr/jit/targetx86.hRemoves x86 pinvoke-calli cookie/target register definitions.
src/coreclr/jit/targetwasm.hRemoves wasm pinvoke-calli cookie/target register definitions.
src/coreclr/jit/targetriscv64.hRemoves riscv64 pinvoke-calli cookie/target register definitions.
src/coreclr/jit/targetloongarch64.hRemoves loongarch64 pinvoke-calli cookie/target register definitions.
src/coreclr/jit/targetarm64.hRemoves arm64 pinvoke-calli cookie/target register definitions.
src/coreclr/jit/targetarm.hRemoves arm pinvoke-calli cookie/target register definitions.
src/coreclr/jit/targetamd64.hRemoves amd64 pinvoke-calli cookie/target register definitions.
src/coreclr/jit/morph.cppRemoves cookie-based indirect-call morphing to CORINFO_HELP_PINVOKE_CALLI.
src/coreclr/jit/lower.hRemoves lowering hook that asserted cookie calls should not reach lowering.
src/coreclr/jit/lower.cppRemoves lowering helper that asserted cookie calls do not exist.
src/coreclr/jit/importercalls.cppAlways attempts calli conversion via convertPInvokeCalliToCall; removes old cookie-building calli path.
src/coreclr/jit/ICorJitInfo_wrapper_generated.hppRemoves wrapper for deleted GetCookieForPInvokeCalliSig.
src/coreclr/jit/ICorJitInfo_names_generated.hRemoves API name entry for deleted GetCookieForPInvokeCalliSig.
src/coreclr/jit/handlekinds.hRemoves handle kind for the deleted pinvoke-calli cookie handle.
src/coreclr/jit/gentree.hRemoves GenTreeCall::gtCallCookie storage.
src/coreclr/jit/gentree.cppRemoves cookie argument/register plumbing and updates call cloning/init.
src/coreclr/jit/ee_il_dll.cppRemoves now-unused eeConvertToLookup helper (used by cookie path).
src/coreclr/jit/compiler.hRemoves eeConvertToLookup declaration.
src/coreclr/inc/jithelpers.hRemoves CORINFO_HELP_PINVOKE_CALLI helper mapping.
src/coreclr/inc/jiteeversionguid.hBumps JIT-EE interface version GUID (interface change).
src/coreclr/inc/icorjitinfoimpl_generated.hRemoves override for deleted GetCookieForPInvokeCalliSig.
src/coreclr/inc/corinfo.hRemoves CORINFO_HELP_PINVOKE_CALLI and deletes GetCookieForPInvokeCalliSig; clarifies convertPInvokeCalliToCall contract.
docs/design/coreclr/botr/guide-for-porting.mdUpdates BOTR documentation to remove mention of the deleted CALLI helper.
docs/design/coreclr/botr/clr-abi.mdRemoves documentation for the deleted CALLI pinvoke cookie/target register convention.

Comment threadsrc/coreclr/vm/stubhelpers.cpp Outdated
Comment threadsrc/coreclr/vm/stubmgr.cpp Outdated
Comment threadsrc/coreclr/vm/dllimport.cpp Outdated
@jkoritzinsky
jkoritzinsky marked this pull request as draft July 31, 2026 17:18
CopilotAI review requested due to automatic review settings July 31, 2026 17:31
@jkoritzinsky
jkoritzinskyforce-pushed the dev/jkoritzinsky/calli-pinvoke branch from 9fd25b3 to 2d846b4CompareJuly 31, 2026 17:31

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 71 out of 71 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/coreclr/vm/stubhelpers.cpp:515

  • The local target in StubHelpers::GetDelegateTarget is now unused on 64-bit builds, which is likely to trigger an unreferenced-local warning (often treated as error in CoreCLR builds). It looks like the old target-mangling logic was removed but the temporary wasn’t cleaned up.
    src/coreclr/vm/stubmgr.cpp:1733
  • *pValue = ((TADDR*)&regs)[index]; type-puns an ArgumentRegisters struct as a TADDR[], which is undefined behavior under C++ strict-aliasing rules (and is a new pattern in this file). This can be made aliasing-safe by copying bytes instead of reinterpreting the pointer.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 71 out of 71 changed files in this pull request and generated no new comments.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 71 out of 71 changed files in this pull request and generated no new comments.

@jkoritzinsky
jkoritzinsky marked this pull request as ready for review July 31, 2026 23:34
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 8 pipeline(s).
8 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@MichalPetryka

Copy link
Copy Markdown
Contributor

x86 failure seems related?

CopilotAI review requested due to automatic review settings August 1, 2026 04:39
jkoritzinskyand others added 15 commits August 24, 2026 10:58
Nothing calls GetCookieForPInvokeCalliSig or CORINFO_HELP_PINVOKE_CALLI now that
an unmanaged calli is either expanded inline or converted to a call to a
marshalling stub, so drop both and bump the JIT-EE version GUID. The generated
files were regenerated with ThunkGenerator/gen.bat.
CORINFO_HELP_PINVOKE_CALLI is not a ReadyToRun helper, so removing it does not
version the ReadyToRun format - R2R images encode READYTORUN_HELPER_* ids, which
are unaffected.
Also removes the SuperPMI recording for the cookie. Its two historical packet
ids are left in place, matching Packet_CanGetCookieForPInvokeCalliSig which has
been unused for some time.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Remove unused #if HOST_64BIT 'target' variable in StubHelpers::GetDelegateTarget
that was left over after removing the old GenericPInvokeCalliHelper mangling.
- Fix BuildCalliILStubSignature to only prepend an extra leading native int for
implicit HASTHIS (HASTHIS && !EXPLICITTHIS). For EXPLICITTHIS signatures the
explicit this parameter is already counted in numArgs, so adding a second
native int would double-count it and produce an incorrect stub signature.
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
TryGetUnmanagedCallingConventionFromModOpt reports its failures as an HRESULT -
COR_E_INVALIDPROGRAM for conflicting calling convention modopts, and
COR_E_BADIMAGEFORMAT for a malformed one - and the original code threw them with
COMPlusThrowHR, which derives the exception kind from that HRESULT. When the
throw became a deferred error carrying a RuntimeExceptionKind, the HRESULT was
dropped and the kind hardcoded to TypeLoadException, so a calli with two
calling convention modopts reported TypeLoadException instead of
InvalidProgramException. Derive the kind from the HRESULT with
EEException::GetKindFromHR, which is what COMPlusThrowHR does internally, so the
deferred throw is identical to the original one.
Fixes baseservices/callconvs/TestCallingConventions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
LowerSpecialCopyArgs implements the x86 IJW copy-constructor semantics by
mapping each argument of the unmanaged call onto the IL stub argument at the
same index, and asserted that the two counts are equal. An unmanaged CALLI stub
takes the call target as an extra trailing argument that is not passed on to the
unmanaged call, so its argument count is one higher and the assert fired for a
calli with IsCopyConstructed modreqs.
The index mapping itself is unaffected - the extra argument is last, and the
loop only walks indices below the unmanaged call's argument count - so only the
assert needs to allow the stub to have more arguments than the call.
Fixes Interop/PInvoke/Miscellaneous/CopyCtor on windows-x86.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Have convertPInvokeCalliToCall fill both hClass and hMethod in CoreCLR and NativeAOT. Record both handles in SuperPMI now that the JIT consumes the complete resolved token directly.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… dead code. Also move calli stubs back to using a stack-local signature (as we always copy in this case).
CopilotAI review requested due to automatic review settings August 24, 2026 17:59
@jkoritzinsky
jkoritzinskyforce-pushed the dev/jkoritzinsky/calli-pinvoke branch from bebabda to 9a716b5CompareAugust 24, 2026 17:59
@jkoritzinsky

Copy link
Copy Markdown
MemberAuthor

Fixed the bad codegen by taking a fix from #132534 and applying it here.

JulieLeeMSFT pushed a commit that referenced this pull request Aug 24, 2026
…orms in DAC stack walk (#131961)
Fixes Issue #131606
main PR N/A (targeted servicing fix; the full fix for main is in #131642
/ #131654, which are too invasive for a servicing branch)
# Description
On 64-bit platforms, an unmanaged `calli` target can be encoded into
`InlinedCallFrame::m_Datum` as `(target << 1) | 1`. This left-shift
moves bit 0 of the target address into bit 1 of `m_Datum`. Two sites in
`src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp`
(`GetCountOfInternalFrames` and `EnumerateInternalFrames`) test bit 1 of
`m_Datum` against `InlinedCallFrameMarker::ExceptionHandlingHelper`
without first checking bit 0 to confirm the field actually holds a
`MethodDesc*` (with the marker bit set) rather than a shifted-and-tagged
raw `calli` target. As a result, when the linker happens to place the
native `calli` target at an odd address, the shifted-in low bit is
misread as the EH-helper marker, and the frame is incorrectly skipped
during stack walking.
This PR adds a `TARGET_64BIT`-guarded check for bit 0 before
interpreting bit 1 as the
`InlinedCallFrameMarker::ExceptionHandlingHelper` marker at both
affected sites, matching the precedent already established in
`InlinedCallFrame::GetFunction_Impl` (`src/coreclr/vm/frames.h`), which
masks out the marker bits before treating `m_Datum` as a pointer.
# Customer Impact
Visual Studio's mixed-mode debugger fails to show native C++ call stack
frames when a C++/CLI layer calls native code through an unmanaged
`calli` whose target address happens to be odd (a common occurrence with
delay-loaded imports and incrementally-linked thunks). This makes
debugging native code called from managed C++/CLI unreliable and appears
intermittent/random to customers, since it depends on a single bit of a
linker-chosen address.
# Regression
Yes. This is a regression from .NET 8, introduced by the exception
handling rewrite.
# Testing
Verified the corrected bit-check logic in isolation (bit 0 check first,
then bit 1), confirming that on 64-bit an odd-tagged `calli` target
(bit0=1, bit1=1) is no longer misidentified as the EH-helper marker,
while behavior for actual EH-helper-marked frames (bit0=0, bit1=1) and
for 32-bit platforms (unaffected, no shift-tagging) is unchanged.
# Risk
Low. The change is narrowly scoped to two conditional checks in the DAC
stack-walk code, guarded by `TARGET_64BIT`, and does not alter behavior
for 32-bit platforms or for frames that are genuinely marked as EH
helpers.
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 86 out of 86 changed files in this pull request and generated no new comments.

@jkoritzinsky
jkoritzinsky merged commit 095c001 into mainAug 24, 2026
167 of 169 checks passed
@jkoritzinsky
jkoritzinsky deleted the dev/jkoritzinsky/calli-pinvoke branch August 24, 2026 21:17
jkoritzinsky added a commit that referenced this pull request Aug 24, 2026
…t-il-com-stubs
Conflict resolved in stubmgr.cpp: the new CLRToCOMStubManager block abuts
the InteropDispatchStubManager comment, which main updated when
GenericPInvokeCalliHelper was removed (#131654). Kept the new block and
took main's comment text.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jkoritzinsky added a commit that referenced this pull request Aug 28, 2026
# Build vararg support only where it is reachable
Stacked on #131654 — targets `dev/jkoritzinsky/calli-pinvoke`, not
`main`.
## What and why
The vararg calling convention is only supported on Windows:
`ConvToJitSig` rejects both `IMAGE_CEE_CS_CALLCONV_VARARG` and
`IMAGE_CEE_CS_CALLCONV_NATIVEVARARG` everywhere else. The supporting
infrastructure was nonetheless compiled on every platform — assembly
stubs, cookie allocation, GC reporting, marshalling helpers and their
managed surface.
This introduces `FEATURE_VARARGS` and builds that machinery only where
it can be reached. **No behavior change on Windows.**
`ConvToJitSig` now keys off the same feature instead of spelling out
`TARGET_UNIX || TARGET_ARM`, so there is a single definition of "varargs
are supported" rather than two that have to be kept in agreement. That
is not a behavior change: `browser` and `wasi` both set
`CLR_CMAKE_TARGET_UNIX` (`configureplatform.cmake` lines 434 and 439),
so every target that threw before still throws.
## The feature is declared twice, deliberately
```cmake
# clrdefinitions.cmake — native
if (CLR_CMAKE_TARGET_WIN32)
```
```xml
<!-- clr.featuredefines.props — managed, alongside FeatureIjw / FeatureCominterop -->
<PropertyGroup Condition="'$(TargetsWindows)' == 'true'">
```
Both are required, and the reason is worth writing down because the
failure mode is silent.
`vm/corelib.h` is not only a C++ header. It is also parsed by the
`CreateRuntimeRootILLinkDescriptorFile` MSBuild task
(`System.Private.CoreLib/CreateRuntimeRootILLinkDescriptorFile.targets`)
to generate the **ILLink root descriptor** for System.Private.CoreLib —
and that task receives the **managed `$(DefineConstants)`**, not the
native CMake defines.
So a native-only `#ifdef` around a `DEFINE_METHOD` there still compiles,
but the ILLink root quietly disappears, the trimmer removes a method the
runtime resolves by name, and you get:
```
Assert failure: pMD != NULL && ("EE expects method to exist")
CoreLibBinder::LookupMethodLocal
ILCodeStream::EmitCALL
ILArgIteratorMarshaler::EmitConvertSpaceAndContentsCLRToNative
```
which points at the marshaller and gives no hint that a *build-system*
conditional is the cause. The existing `#ifdef FEATURE_COMINTEROP` in
that header is not a counter-example: `FEATURE_COMINTEROP` is emitted on
**both** sides by `clr.featuredefines.props`, so its root is dropped
exactly when the managed code is also absent. Declaring `FeatureVarargs`
in the same place gives `FEATURE_VARARGS` that same symmetry, which is
what makes gating `corelib.h` safe here.
## Deleted outright
Unix `VarargPInvokeStub` implementations in `amd64`, `arm`, `arm64`,
`loongarch64` and `riscv64` `pinvokestubs.S` and in `i386/asmhelpers.S`;
the wasm `PORTABILITY_ASSERT` placeholders in `vm/wasm/helpers.cpp`; and
the `VASigCookie` offsets in the `arm`, `loongarch64` and `riscv64`
`asmconstants.h`. None of those files is deletable whole — each also
holds `JIT_PInvokeBegin`/`End`/`InitPInvokeFrame`.
## Gated — native
The cookie machinery (`struct VASigCookie` and its cDAC data descriptor,
`VASigCookieBlock`, `Module::GetVASigCookie`/`GetVASigCookieWorker`,
`VASigCookieEx`), `VarargPInvokeStubWorker`,
`GetILStubForVarargPInvoke`, `TheVarargPInvokeStub`,
`InteropDispatchStubManager` (plus its `vptr_list.h` entry and
`Init()`), `clrvarargs.cpp` and `varargsnative.cpp`, the `ArgIterator` →
`va_list` marshaller and the `StubHelpers` entrypoints behind it,
`TransitionFrame::GetVASigCookie`, and the vararg GC reporting in
`eetwain.cpp` and `gc_unwind_x86.inl`.
`Module::GetLoaderModuleForSignature` is **not** gated — the
unmanaged-`calli` stub path calls it on every platform.
The Windows `.asm` files are guarded rather than excluded. Strictly
unnecessary, since every Windows arch enables the feature — but it buys
a compile-time proxy: forcing `FEATURE_VARARGS` off on windows-x64
compiles the whole VM **and DAC** as a stand-in for the six targets that
can't be built locally. That proxy earned its keep immediately by
catching `promoteVarArgs` in `eetwain.cpp`, which takes
`PTR_VASigCookie` and is compiled on every target — a real
cross-platform break that inspection alone had missed.
## Gated — managed
NativeAOT's throwing `ArgIterator` moves to the shared partition as
`System/ArgIterator.PlatformNotSupported.cs` and is now used by CoreCLR
too whenever the feature is off, so the throwing copy exists once
instead of twice:
| Build | `ArgIterator` source |
|---|---|
| CoreCLR, varargs supported | its own `ArgIterator.cs` (real
implementation) |
| CoreCLR, varargs unsupported | shared
`ArgIterator.PlatformNotSupported.cs` |
| NativeAOT | shared `ArgIterator.PlatformNotSupported.cs` |
| Mono | its own real implementation, untouched |
The `Shared.projitems` condition is `'$(FeatureVarargs)' != 'true' and
'$(FeatureMono)' != 'true'`. The Mono clause matters: Mono's CoreLib
also imports `Shared.projitems`, has its own real `ArgIterator`, and
does not import `clr.featuredefines.props` — without it, Mono would pick
up a duplicate `System.ArgIterator`.
Because CoreCLR's `ArgIterator.cs` is now included conditionally, its
`#if TARGET_WINDOWS` split and the ~50-line duplicated throwing branch
both go away; the file has no preprocessor directives left. The three
`StubHelpers` `va_list` members that only the marshaller calls are gated
the same way.
## Deliberately unchanged
- **`GCREFMAP_VASIG_COOKIE` (= 5)** — part of the ReadyToRun GC ref map
format. Value kept; only its handling is gated.
- **`DynamicMethodDesc::StubPInvokeVarArg` (= 4)** — the cDAC contract
depends on the value.
- **x86 GC info `varargs` bit / `FLIP_VARARGS`** — serialized format,
and x86 *is* a supported vararg target. There is no unreachable x86
GC/unwind vararg code.
- **`tools/Common/CallingConvention/ArgIterator.cs`** — crossgen2
cross-targets, so it cannot be gated at compile time.
- **`inc/dacdbi.idl` / `debug/inc/dacdbiinterface.h`** — versioned
interface shape untouched; only the `GetVarArgSig` implementation is
gated.
- **`getVarArgsHandle`** keeps its `ICorJitInfo` vtable slot and asserts
instead of being removed. `ICorJitInfo` is not per-platform and varargs
do work on Windows, so the JIT still needs the method there; this change
does not reshape the interface.
`GetStubForILStub` now throws `IDS_EE_VARARG_NOT_SUPPORTED` for every
target without the feature rather than only under
`FEATURE_PORTABLE_ENTRYPOINTS` — a strict generalization, since that
feature is wasm-only and already outside `FEATURE_VARARGS`.
## cDAC
`struct VASigCookie` and its `CDAC_TYPE` descriptor are gated, so a
target without the feature stops advertising a type it can never
allocate. The managed cDAC is unchanged and reads the **target's own**
descriptor, so older runtimes are unaffected; the lookup is lazy and
only reachable from `DacDbiImpl.GetVarArgSig` on a vararg frame.
Verified by inspecting the emitted contract descriptor in both
configurations.
## Renamed
`PINVOKE_CALLI_SIGTOKEN_REGNUM`/`REGISTER` on amd64 →
`PINVOKE_VARARG_SIGTOKEN_REGNUM`/`REGISTER`. That register (r11) carries
the `VASigCookie*` into `VarargPInvokeStub`; it outlived the
unmanaged-`calli` helpers removed in the parent layer, so the old name
referred to a caller that no longer exists.
## Note for reviewers touching `asmconstants.h`
`h2inc` runs at CMake **configure** time and `asmconstants.h` is not
registered as a configure dependency, so editing a constant's name or
value does not regenerate `AsmConstants.inc` on an incremental build —
the assembler keeps consuming the stale copy. The rename above surfaced
this as `error A2006: undefined symbol`. Pre-existing infrastructure
behavior, not changed here; a reconfigure picks it up. Worth knowing
because a silently stale *value* would be far less obvious than a stale
name.
## Validation
| Target | Result |
|---|---|
| windows-x64 `clr+libs -rc Checked` | builds clean |
| windows-x86 | builds clean (exercises `i386/asmhelpers.asm` +
asmconstants) |
| windows-arm64 | builds clean (exercises `arm64/PInvokeStubs.asm` +
asmconstants) |
| windows-x64, `FEATURE_VARARGS` forced **off** on both sides | builds
clean, **including the DAC** |
| NativeAOT (`clr.nativeaotlibs`) | builds clean, picks up the shared
file |
| linux/osx x64, linux arm32/arm64, loongarch64, riscv64, browser-wasm |
**not built locally** — covered by inspection plus the forced-off proxy
|
Tests (windows-x64 Checked):
- `JIT/Directed/arglist/vararg_TargetWindows` — 241/241 passed
- Interop — 333 total, 321 passed, 10 failed; the 10 are the known
pre-existing out-of-proc COM and DisabledRuntimeMarshalling environment
failures, unchanged from baseline. `VarargsTest`,
`CrossAssemblyVarargsTest` and IJW `NativeVarargsTest` all pass.
- JIT/Directed — 686 total, 679 passed, **0 failed**, 7 skipped
Feature-off behavior was verified by inspecting build output rather than
assuming: the contract descriptor drops `VASigCookie` (while
`MethodTable` remains), and CoreLib drops
`ArgIterator_Init`/`CalcVaListSize` while gaining
`PlatformNotSupported_ArgIterator`.
> [!NOTE]
> This pull request description was generated by GitHub Copilot.
---------
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 544b2da7-b36a-4a7c-a2df-20e1487849af
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

6 participants

@jkoritzinsky@MichalPetryka@jkotas@jakobbotsch