Build vararg support only where it is reachable - #131655
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
This looks good to me. I do not understand the point of it being part of a stacked PR. Can this change be merged once the CI is greeen? |
|
Tagging subscribers to this area: @agocke |
|
This is stacked because it depends on unmanaged calli moving away from using VASigCookie on all platforms. Right now VASigCookie is used by varargs and calli. |
|
Ah ok, part of it depends on the change. |
de96c3f to
118c9a9
Compare
118c9a9 to
4b5c744
Compare
|
Does this mean that #48796 won't happen? |
There was a problem hiding this comment.
Pull request overview
This PR introduces a FEATURE_VARARGS build feature and uses it to compile vararg-related infrastructure only on platforms where vararg calling convention support is reachable (primarily Windows). The change centralizes “varargs supported” logic behind a single feature define and removes or gates otherwise-dead VM/JIT interop stubs, cookie machinery, marshalling helpers, and managed surface area on unsupported targets.
Changes:
- Add
FEATURE_VARARGS(native CMake + managed MSBuild) and use it to gate VM/JIT vararg support, includingConvToJitSigrejection logic. - Rework CoreLib’s
ArgIteratorinclusion: real implementation only when varargs are supported; otherwise use a shared PlatformNotSupported implementation (also used by NativeAOT). - Remove unreachable non-Windows vararg P/Invoke stub implementations / placeholders and gate related assembly constants, stub managers, QCalls, and cDAC descriptors.
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/ArgIterator.PlatformNotSupported.cs | Remove unused using; keep PNSE ArgIterator implementation. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Include PNSE ArgIterator only when varargs are off and not Mono. |
| src/coreclr/vm/wasm/helpers.cpp | Remove wasm VarargPInvokeStub* PORTABILITY_ASSERT placeholders. |
| src/coreclr/vm/stubmgr.h | Gate InteropDispatchStubManager declaration behind FEATURE_VARARGS. |
| src/coreclr/vm/stubmgr.cpp | Gate vararg stub recognition/tracing + DAC enumeration behind FEATURE_VARARGS. |
| src/coreclr/vm/stubhelpers.h | Gate vararg-related FCALL/QCALL declarations behind FEATURE_VARARGS. |
| src/coreclr/vm/stubhelpers.cpp | Gate vararg-related FCALL/QCALL implementations behind FEATURE_VARARGS. |
| src/coreclr/vm/riscv64/pinvokestubs.S | Remove unreachable vararg P/Invoke stub generator macro and stubs. |
| src/coreclr/vm/riscv64/asmconstants.h | Remove VASigCookie IL stub offset constant on unsupported target. |
| src/coreclr/vm/qcallentrypoints.cpp | Gate ArgIterator_* and StubHelpers_*VaList QCall entries behind FEATURE_VARARGS. |
| src/coreclr/vm/prestub.cpp | Gate TheVarargPInvokeStub behind FEATURE_VARARGS; simplify ret-buff arch conditions. |
| src/coreclr/vm/mtypes.h | Gate MARSHAL_TYPE_ARGITERATOR behind FEATURE_VARARGS. |
| src/coreclr/vm/mlinfo.cpp | Gate ArgIterator marshaler detection behind FEATURE_VARARGS. |
| src/coreclr/vm/metasig.h | Gate vararg metasig entry behind FEATURE_VARARGS. |
| src/coreclr/vm/loongarch64/pinvokestubs.S | Remove unreachable vararg P/Invoke stub generator macro and stubs. |
| src/coreclr/vm/loongarch64/asmconstants.h | Remove VASigCookie IL stub offset constant on unsupported target. |
| src/coreclr/vm/jitinterface.cpp | Use FEATURE_VARARGS for vararg rejection; gate vararg class/cookie paths with asserts when off. |
| src/coreclr/vm/ilmarshalers.h | Gate ILArgIteratorMarshaler definition behind FEATURE_VARARGS. |
| src/coreclr/vm/ilmarshalers.cpp | Gate ILArgIteratorMarshaler implementation behind FEATURE_VARARGS. |
| src/coreclr/vm/i386/asmhelpers.S | Remove unreachable non-Windows x86 vararg stub implementation. |
| src/coreclr/vm/i386/asmhelpers.asm | Gate Windows x86 vararg stub + worker extern behind FEATURE_VARARGS. |
| src/coreclr/vm/i386/asmconstants.h | Gate VASigCookie stub offset constant behind FEATURE_VARARGS. |
| src/coreclr/vm/gc_unwind_x86.inl | Assert/PORTABILITY_ASSERT varargs GC unwind when varargs feature is off. |
| src/coreclr/vm/frames.h | Gate TransitionFrame::GetVASigCookie declaration behind FEATURE_VARARGS. |
| src/coreclr/vm/frames.cpp | Gate GetVASigCookie; restructure caller stack promotion and GC refmap handling for varargs. |
| src/coreclr/vm/eetwain.cpp | Gate promoteVarArgs + varargs GC reporting; assert unreachable when feature is off. |
| src/coreclr/vm/ecalllist.h | Gate StubHelpers::CalcVaListSize FCALL exposure behind FEATURE_VARARGS. |
| src/coreclr/vm/dllimport.cpp | Gate vararg P/Invoke stub building and VarargPInvokeStubWorker; throw IDS_EE_VARARG_NOT_SUPPORTED when feature off. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Gate cDAC VASigCookie type descriptor behind FEATURE_VARARGS. |
| src/coreclr/vm/corelib.h | Gate CoreLib binder entries for ArgIterator/RuntimeArgumentHandle and va_list StubHelpers behind FEATURE_VARARGS. |
| src/coreclr/vm/CMakeLists.txt | Only compile clrvarargs.* / varargsnative.* sources when FEATURE_VARARGS is enabled. |
| src/coreclr/vm/class.h | Gate TheVarargPInvokeStub declaration behind FEATURE_VARARGS. |
| src/coreclr/vm/cgensys.h | Gate vararg stub worker/stub exports behind FEATURE_VARARGS; simplify ret-buff arch conditions. |
| src/coreclr/vm/ceeload.h | Gate VASigCookie / cookie block storage and APIs behind FEATURE_VARARGS. |
| src/coreclr/vm/ceeload.cpp | Gate Module::GetVASigCookie*; gate clrvarargs.h include for asmconstant asserts. |
| src/coreclr/vm/arm64/pinvokestubs.S | Remove unreachable unix arm64 vararg stub generator/stub. |
| src/coreclr/vm/arm64/PInvokeStubs.asm | Gate Windows arm64 vararg stub machinery behind FEATURE_VARARGS. |
| src/coreclr/vm/arm64/asmconstants.h | Gate VASigCookie IL stub offset constant behind FEATURE_VARARGS. |
| src/coreclr/vm/arm/pinvokestubs.S | Remove unreachable unix arm32 vararg stubs and generator macro. |
| src/coreclr/vm/arm/asmconstants.h | Remove VASigCookie IL stub offset constant on unsupported target. |
| src/coreclr/vm/appdomain.cpp | Only initialize InteropDispatchStubManager when FEATURE_VARARGS is enabled. |
| src/coreclr/vm/amd64/pinvokestubs.S | Remove unreachable unix amd64 vararg stubs. |
| src/coreclr/vm/amd64/PInvokeStubs.asm | Gate Windows amd64 vararg stubs behind FEATURE_VARARGS; rename sigtoken register macro. |
| src/coreclr/vm/amd64/asmconstants.h | Gate vararg sigtoken register and VASigCookie stub offset constants behind FEATURE_VARARGS. |
| src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj | Compile ArgIterator.cs only when FeatureVarargs is true. |
| src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs | Gate va_list marshalling imports and CalcVaListSize behind FEATURE_VARARGS. |
| src/coreclr/System.Private.CoreLib/src/System/ArgIterator.cs | Remove platform preprocessor split; keep only the real implementation (now conditionally compiled by csproj). |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj | Stop compiling NativeAOT’s old System\\ArgIterator.cs (use shared PNSE implementation instead). |
| src/coreclr/inc/vptr_list.h | Gate InteropDispatchStubManager VPTR entry behind FEATURE_VARARGS. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Return E_NOTIMPL for GetVarArgSig when varargs are off (implementation gated). |
| src/coreclr/clrdefinitions.cmake | Define FEATURE_VARARGS for Windows builds (native define). |
| src/coreclr/clr.featuredefines.props | Define FeatureVarargs for Windows builds and propagate FEATURE_VARARGS to managed DefineConstants. |
| #ifndef FEATURE_VARARGS | ||
| // Varargs are not supported on this target, so no vararg frame can exist for the | ||
| // right side to have found a VASigCookie on. | ||
| return E_NOTIMPL; | ||
| #else // FEATURE_VARARGS |
That will likely use a different mechanism than CLR-native varargs. Either that or we'd enable FEATURE_VARARGS and do the work, but I don't think we'd implement it as such as managed varargs is not something we want to do. |
Yeah I agree that those are not needed. |
The vararg calling convention is only supported on Windows, but the supporting infrastructure was compiled on every platform. Introduce FEATURE_VARARGS and build the machinery only where it is reachable. ConvToJitSig, which rejects IMAGE_CEE_CS_CALLCONV_VARARG and IMAGE_CEE_CS_CALLCONV_NATIVEVARARG, now keys off the same feature instead of spelling out TARGET_UNIX || TARGET_ARM, so there is a single definition of where varargs are supported. That is not a behaviour change: browser and wasi both set CLR_CMAKE_TARGET_UNIX, so every target that threw before still throws. The feature is declared twice, deliberately. clrdefinitions.cmake defines it for the native build, and clr.featuredefines.props defines it for the managed CoreLib build alongside FeatureIjw and FeatureCominterop, which share the same Windows-only condition. Both are needed because vm/corelib.h is not only a C++ header: CreateRuntimeRootILLinkDescriptorFile parses it with the managed DefineConstants to generate the ILLink root descriptor. A native-only conditional there would still compile, but would silently drop the root and let the trimmer remove a method the runtime resolves by name. The Unix VarargPInvokeStub implementations are deleted outright from amd64, arm, arm64, loongarch64 and riscv64 pinvokestubs.S and from i386/asmhelpers.S, along with the wasm PORTABILITY_ASSERT placeholders and the VASigCookie offsets in the arm, loongarch64 and riscv64 asmconstants.h. The Windows stubs are guarded in place because the .asm files also hold JIT_PInvokeBegin/End. The VASigCookie offsets shared between a Windows and a Unix build - amd64, arm64 and i386 - are guarded rather than removed. PINVOKE_CALLI_SIGTOKEN_REGNUM/REGISTER on amd64 is renamed to PINVOKE_VARARG_SIGTOKEN_REGNUM/REGISTER. It survived the removal of the unmanaged calli helpers as the register carrying the VASigCookie* into VarargPInvokeStub, so the old name described a caller that no longer exists. Gated on the native side: the VASigCookie allocation and lookup machinery, VarargPInvokeStubWorker, GetILStubForVarargPInvoke, TheVarargPInvokeStub, InteropDispatchStubManager, clrvarargs.cpp and varargsnative.cpp, the ArgIterator marshaller and the StubHelpers va_list entrypoints behind it, TransitionFrame::GetVASigCookie, and the vararg GC reporting in eetwain.cpp and gc_unwind_x86.inl. Module::GetLoaderModuleForSignature stays ungated - the unmanaged calli stub path calls it on every platform. On the managed side NativeAOT's throwing ArgIterator moves to the shared partition as System/ArgIterator.PlatformNotSupported.cs and is now used by CoreCLR too whenever FEATURE_VARARGS is off, so the throwing copy is written once instead of twice. Mono keeps its own implementation. CoreCLR's real ArgIterator is now included only when the feature is on, which lets its TARGET_WINDOWS conditional - a near miss for the actual condition - go away entirely. The three StubHelpers va_list members that only the ArgIterator marshaller calls are gated the same way. 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 and 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. GCREFMAP_VASIG_COOKIE keeps its value because it is part of the ReadyToRun GC ref map format; only its handling is gated. struct VASigCookie and its cDAC data descriptor are gated as well, so a target without the feature no longer advertises a type it can never allocate. The managed cDAC reads the target's own descriptor, so older runtimes are unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4b5c744 to
ce5ca3d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/coreclr/vm/jitinterface.cpp:6268
- Similar to the CLASSID_ARGUMENT_HANDLE case: in retail builds this will just return NULL (and leave *ppIndirection NULL) if called, which risks later crashes rather than a clear "varargs not supported" failure.
Since this is a THROWS path already, it’s safer to throw IDS_EE_VARARG_NOT_SUPPORTED here when FEATURE_VARARGS is off.
src/coreclr/clrdefinitions.cmake:126
- FEATURE_VARARGS is defined for all Win32 targets here. That’s broader than the actual varargs implementation (e.g., the old ConvToJitSig guard also excluded TARGET_ARM), and can accidentally enable varargs on Windows targets/arches that don’t have the supporting stubs/GC/reporting.
Consider scoping FEATURE_VARARGS to the same set of Windows architectures that actually support varargs (x86/x64/arm64), similar to FEATURE_INTEROP_DEBUGGING just above.
if (CLR_CMAKE_TARGET_WIN32)
set(FEATURE_VARARGS 1)
add_definitions(-DFEATURE_VARARGS)
endif (CLR_CMAKE_TARGET_WIN32)
src/coreclr/clr.featuredefines.props:54
- FeatureVarargs is enabled for all Windows builds, but the implementation is only expected to exist on specific Windows architectures (x86/x64/arm64). Keeping this scoped avoids accidentally defining FEATURE_VARARGS in managed builds where the native support isn’t present, and keeps the managed/native definitions truly “in sync” as the comment suggests.
<!-- The vararg calling convention is only supported on Windows.
Must stay in sync with FEATURE_VARARGS in clrdefinitions.cmake. -->
<FeatureVarargs>true</FeatureVarargs>
</PropertyGroup>
src/coreclr/vm/jitinterface.cpp:4098
- In retail builds (where _ASSERTE is compiled out), this path returns a null CORINFO_CLASS_HANDLE for CLASSID_ARGUMENT_HANDLE. If the JIT ever does request it (e.g., due to unexpected IL), that would likely turn into a harder-to-diagnose null deref later.
Since ConvToJitSig uses IDS_EE_VARARG_NOT_SUPPORTED for unsupported varargs, consider throwing the same exception here as a defensive fail-fast.
This issue also appears on line 6264 of the same file.
Build vararg support only where it is reachable
Stacked on #131654 — targets
dev/jkoritzinsky/calli-pinvoke, notmain.What and why
The vararg calling convention is only supported on Windows:
ConvToJitSigrejects bothIMAGE_CEE_CS_CALLCONV_VARARGandIMAGE_CEE_CS_CALLCONV_NATIVEVARARGeverywhere 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_VARARGSand builds that machinery only where it can be reached. No behavior change on Windows.ConvToJitSignow keys off the same feature instead of spelling outTARGET_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:browserandwasiboth setCLR_CMAKE_TARGET_UNIX(configureplatform.cmakelines 434 and 439), so every target that threw before still throws.The feature is declared twice, deliberately
Both are required, and the reason is worth writing down because the failure mode is silent.
vm/corelib.his not only a C++ header. It is also parsed by theCreateRuntimeRootILLinkDescriptorFileMSBuild 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
#ifdefaround aDEFINE_METHODthere still compiles, but the ILLink root quietly disappears, the trimmer removes a method the runtime resolves by name, and you get:which points at the marshaller and gives no hint that a build-system conditional is the cause. The existing
#ifdef FEATURE_COMINTEROPin that header is not a counter-example:FEATURE_COMINTEROPis emitted on both sides byclr.featuredefines.props, so its root is dropped exactly when the managed code is also absent. DeclaringFeatureVarargsin the same place givesFEATURE_VARARGSthat same symmetry, which is what makes gatingcorelib.hsafe here.Deleted outright
Unix
VarargPInvokeStubimplementations inamd64,arm,arm64,loongarch64andriscv64pinvokestubs.Sand ini386/asmhelpers.S; the wasmPORTABILITY_ASSERTplaceholders invm/wasm/helpers.cpp; and theVASigCookieoffsets in thearm,loongarch64andriscv64asmconstants.h. None of those files is deletable whole — each also holdsJIT_PInvokeBegin/End/InitPInvokeFrame.Gated — native
The cookie machinery (
struct VASigCookieand its cDAC data descriptor,VASigCookieBlock,Module::GetVASigCookie/GetVASigCookieWorker,VASigCookieEx),VarargPInvokeStubWorker,GetILStubForVarargPInvoke,TheVarargPInvokeStub,InteropDispatchStubManager(plus itsvptr_list.hentry andInit()),clrvarargs.cppandvarargsnative.cpp, theArgIterator→va_listmarshaller and theStubHelpersentrypoints behind it,TransitionFrame::GetVASigCookie, and the vararg GC reporting ineetwain.cppandgc_unwind_x86.inl.Module::GetLoaderModuleForSignatureis not gated — the unmanaged-callistub path calls it on every platform.The Windows
.asmfiles are guarded rather than excluded. Strictly unnecessary, since every Windows arch enables the feature — but it buys a compile-time proxy: forcingFEATURE_VARARGSoff 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 catchingpromoteVarArgsineetwain.cpp, which takesPTR_VASigCookieand is compiled on every target — a real cross-platform break that inspection alone had missed.Gated — managed
NativeAOT's throwing
ArgIteratormoves to the shared partition asSystem/ArgIterator.PlatformNotSupported.csand is now used by CoreCLR too whenever the feature is off, so the throwing copy exists once instead of twice:ArgIteratorsourceArgIterator.cs(real implementation)ArgIterator.PlatformNotSupported.csArgIterator.PlatformNotSupported.csThe
Shared.projitemscondition is'$(FeatureVarargs)' != 'true' and '$(FeatureMono)' != 'true'. The Mono clause matters: Mono's CoreLib also importsShared.projitems, has its own realArgIterator, and does not importclr.featuredefines.props— without it, Mono would pick up a duplicateSystem.ArgIterator.Because CoreCLR's
ArgIterator.csis now included conditionally, its#if TARGET_WINDOWSsplit and the ~50-line duplicated throwing branch both go away; the file has no preprocessor directives left. The threeStubHelpersva_listmembers 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.varargsbit /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 theGetVarArgSigimplementation is gated.getVarArgsHandlekeeps itsICorJitInfovtable slot and asserts instead of being removed.ICorJitInfois not per-platform and varargs do work on Windows, so the JIT still needs the method there; this change does not reshape the interface.GetStubForILStubnow throwsIDS_EE_VARARG_NOT_SUPPORTEDfor every target without the feature rather than only underFEATURE_PORTABLE_ENTRYPOINTS— a strict generalization, since that feature is wasm-only and already outsideFEATURE_VARARGS.cDAC
struct VASigCookieand itsCDAC_TYPEdescriptor 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 fromDacDbiImpl.GetVarArgSigon a vararg frame. Verified by inspecting the emitted contract descriptor in both configurations.Renamed
PINVOKE_CALLI_SIGTOKEN_REGNUM/REGISTERon amd64 →PINVOKE_VARARG_SIGTOKEN_REGNUM/REGISTER. That register (r11) carries theVASigCookie*intoVarargPInvokeStub; it outlived the unmanaged-callihelpers removed in the parent layer, so the old name referred to a caller that no longer exists.Note for reviewers touching
asmconstants.hh2incruns at CMake configure time andasmconstants.his not registered as a configure dependency, so editing a constant's name or value does not regenerateAsmConstants.incon an incremental build — the assembler keeps consuming the stale copy. The rename above surfaced this aserror 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
clr+libs -rc Checkedi386/asmhelpers.asm+ asmconstants)arm64/PInvokeStubs.asm+ asmconstants)FEATURE_VARARGSforced off on both sidesclr.nativeaotlibs)Tests (windows-x64 Checked):
JIT/Directed/arglist/vararg_TargetWindows— 241/241 passedVarargsTest,CrossAssemblyVarargsTestand IJWNativeVarargsTestall pass.Feature-off behavior was verified by inspecting build output rather than assuming: the contract descriptor drops
VASigCookie(whileMethodTableremains), and CoreLib dropsArgIterator_Init/CalcVaListSizewhile gainingPlatformNotSupported_ArgIterator.Note
This pull request description was generated by GitHub Copilot.