Uh oh!
There was an error while loading. Please reload this page.
Route QCall exceptions through marshalling - #132420
Open
davidwrighton wants to merge 29 commits into
Open
Conversation
Contributor
Tagging subscribers to this area: @agocke |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes CoreCLR QCall exception propagation by routing exceptions thrown inside BEGIN_QCALL/END_QCALL through an out QCallException status value that is custom-marshalled back to managed code. The native side captures throwables onto the managed Thread, and the managed marshaller rethrows during unmarshalling, while leaving non-BEGIN_QCALL QCalls on the prior behavior.
Changes:
- Introduce
QCallException+QCallExceptionMarshallerand update managed QCall signatures to include anout QCallExceptionforBEGIN_QCALL-based QCalls. - Update native QCall entrypoints and exception macros to capture exceptions instead of dispatching/rethrowing directly across the boundary.
- Add the per-thread storage needed to temporarily root the captured throwable and rethrow it from managed unmarshalling.
Reviewed changes
Copilot reviewed 170 out of 170 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Threading/Wasi/WasiFinalizerScheduler.cs | Add conditional out QCallException to WASI finalizer QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs | Pass conditional out _ to updated reentrant wait QCall. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs | Update CurrentOSThreadId QCall invocation to include conditional out _. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs | Use updated Environment.GetProcessorCount signature under CORECLR. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs | Thread out QCallException through multiple tracing/ALC QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TypeMapLazyDictionary.cs | Add out QCallException to QCalls used by type map cache population/lookups. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveC/ObjectiveCMarshal.cs | Pass out _ to updated ObjC marshal QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.NoCom.cs | Extend internal signature with conditional out _ under CORECLR. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs | Call updated RegisterIsRootedCallback under CORECLR. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs | Update tracing QCall invocations to include out _. |
| src/libraries/System.Private.CoreLib/src/System/Environment.cs | Update ProcessorCount initialization to use CORECLR QCall signature. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/XplatEventLogger.cs | Add out QCallException plumbing to xplat EventSource QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs | Add conditional out _ to EventPipeInternal QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventDispatcher.Threads.cs | Add conditional out _ to EventPipe session wait/disable QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventDispatcher.cs | Add conditional out _ to EventPipe dispatcher QCalls. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipe.cs | Add conditional out _ to EventPipe enable QCall. |
| src/libraries/System.Private.CoreLib/src/System/AppContext.cs | Add conditional out _ to host property QCall path. |
| src/coreclr/vm/weakreferencenative.h | Add QCallException* to COM weak ref QCall signatures. |
| src/coreclr/vm/weakreferencenative.cpp | Add QCallException* parameter to COM weak ref QCall implementations. |
| src/coreclr/vm/wasm/entrypoints.h | Add QCallException* to WASI finalizer exported entrypoints. |
| src/coreclr/vm/versionresilienthashcode.h | Add QCallException* to version-resilient hash QCall signature. |
| src/coreclr/vm/versionresilienthashcode.cpp | Add QCallException* to version-resilient hash QCall implementation. |
| src/coreclr/vm/varargsnative.h | Add QCallException* to varargs iterator QCall signatures. |
| src/coreclr/vm/varargsnative.cpp | Add QCallException* to varargs iterator QCall implementations. |
| src/coreclr/vm/unsafeaccessors.cpp | Add QCallException* to unsafe accessor resolver QCall. |
| src/coreclr/vm/tailcallhelp.h | Add QCallException* to tailcall arg buffer allocation QCall. |
| src/coreclr/vm/tailcallhelp.cpp | Add QCallException* to tailcall arg buffer allocation QCall implementation. |
| src/coreclr/vm/stubhelpers.h | Add QCallException* to multiple interop/stub helper QCall signatures. |
| src/coreclr/vm/stackwalk.h | Add QCallException* to AppendExceptionStackFrame QCall signature. |
| src/coreclr/vm/reflectioninvocation.h | Add QCallException* to reflection invocation QCall signatures. |
| src/coreclr/vm/qcall.h | Update QCall guidance/comments and route BEGIN/END macros to capture dispatcher. |
| src/coreclr/vm/qcall.cpp | Add QCallException::SetThrowable implementation and thread rooting logic. |
| src/coreclr/vm/olevariant.h | Add QCallException* to VARIANT record conversion QCall signature. |
| src/coreclr/vm/olevariant.cpp | Add QCallException* to VARIANT record conversion QCall implementation. |
| src/coreclr/vm/object.h | Add m_QCallException field + setter on ThreadBaseObject. |
| src/coreclr/vm/nativelibrarynative.h | Add QCallException* to NativeLibrary load-by-name QCall signature. |
| src/coreclr/vm/nativelibrarynative.cpp | Add QCallException* to NativeLibrary load-by-name QCall implementation. |
| src/coreclr/vm/nativeeventsource.h | Add QCallException* to EventSource/perf tracing QCall signatures. |
| src/coreclr/vm/multicorejit.h | Add QCallException* to MultiCoreJIT QCall signatures. |
| src/coreclr/vm/multicorejit.cpp | Add QCallException* to MultiCoreJIT QCall implementations. |
| src/coreclr/vm/mlinfo.cpp | Add QCallException* to StubHelpers_CreateCustomMarshaler QCall implementation. |
| src/coreclr/vm/method.hpp | Add QCallException* to unsafe accessor resolver declaration. |
| src/coreclr/vm/managedmdimport.hpp | Add QCallException* to metadata enum QCall signature. |
| src/coreclr/vm/managedmdimport.cpp | Add QCallException* to metadata enum QCall implementation. |
| src/coreclr/vm/loaderallocator.hpp | Add QCallException* to loader allocator destroy QCall signature. |
| src/coreclr/vm/loaderallocator.cpp | Add QCallException* to loader allocator destroy QCall implementation. |
| src/coreclr/vm/JitQCallHelpers.h | Add QCallException* to JIT helper QCall signatures. |
| src/coreclr/vm/jithelpers.cpp | Add QCallException* to JIT helper QCalls and adjust handler installation in rare paths. |
| src/coreclr/vm/interoplibinterface.h | Add QCallException* to ObjC/Java marshal QCall signatures. |
| src/coreclr/vm/interoplibinterface_objc.cpp | Add QCallException* to ObjC marshal QCall implementations. |
| src/coreclr/vm/interoplibinterface_java.cpp | Add QCallException* to Java marshal QCall implementations. |
| src/coreclr/vm/interoplibinterface_comwrappers.h | Add QCallException* to ComWrappers QCall signatures. |
| src/coreclr/vm/interoplibinterface_comwrappers.cpp | Add QCallException* to ComWrappers QCall implementations. |
| src/coreclr/vm/ilmarshalers.h | Add QCallException* to safe array marshaler QCall signatures. |
| src/coreclr/vm/ilmarshalers.cpp | Add QCallException* to safe array marshaler QCall implementations. |
| src/coreclr/vm/finalizerthread.cpp | Add QCallException* to WASI finalizer QCall implementations. |
| src/coreclr/vm/exceptmacros.h | Introduce QCallException native struct and capture-dispatcher exception macros. |
| src/coreclr/vm/exceptionhandlingqcalls.h | Add QCallException* to exception-handling-related QCall signatures. |
| src/coreclr/vm/exceptionhandling.cpp | Add QCallException* to exception-handling-related QCall implementations. |
| src/coreclr/vm/excep.cpp | Add QCallException* to message QCalls and introduce QCall-specific unwind helper. |
| src/coreclr/vm/eventpipeinternal.h | Add QCallException* to EventPipeInternal QCall signatures. |
| src/coreclr/vm/eventpipeinternal.cpp | Add QCallException* to EventPipeInternal QCall implementations. |
| src/coreclr/vm/debugdebugger.h | Add QCallException* to debugger/stacktrace QCall signatures. |
| src/coreclr/vm/debugdebugger.cpp | Add QCallException* to debugger/stacktrace QCall implementations. |
| src/coreclr/vm/customattribute.h | Add QCallException* to custom attribute QCall signatures. |
| src/coreclr/vm/customattribute.cpp | Add QCallException* to custom attribute QCall implementations. |
| src/coreclr/vm/corelib.h | Add corelib field mapping for new thread _qcallException field. |
| src/coreclr/vm/coreassemblyspec.cpp | Add QCallException* to assembly spec initialization QCall signature. |
| src/coreclr/vm/comsynchronizable.h | Add QCallException* to thread/lock-related QCall signatures. |
| src/coreclr/vm/comsynchronizable.cpp | Add QCallException* to thread/lock-related QCall implementations. |
| src/coreclr/vm/commodule.h | Add QCallException* to Reflection.Emit module builder QCall signatures. |
| src/coreclr/vm/commodule.cpp | Add QCallException* to Reflection.Emit module builder QCall implementations. |
| src/coreclr/vm/comdependenthandle.h | Add QCallException* to dependent handle QCall signatures. |
| src/coreclr/vm/comdependenthandle.cpp | Add QCallException* to dependent handle QCall implementations. |
| src/coreclr/vm/comdelegate.h | Add QCallException* to delegate-related QCall signatures. |
| src/coreclr/vm/comdelegate.cpp | Add QCallException* to delegate-related QCall implementations. |
| src/coreclr/vm/clrconfignative.h | Add QCallException* to CLR config QCall signature. |
| src/coreclr/vm/clrconfignative.cpp | Add QCallException* to CLR config QCall implementation. |
| src/coreclr/vm/arraynative.h | Add QCallException* to array creation/ctor QCall signatures. |
| src/coreclr/vm/arraynative.cpp | Add QCallException* to array creation/ctor QCall implementations. |
| src/coreclr/vm/appdomainnative.hpp | Add QCallException* to AppDomain/AppContext/string intern QCall signatures. |
| src/coreclr/vm/appdomainnative.cpp | Add QCallException* to AppDomain/AppContext/string intern QCall implementations. |
| src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj | Include new QCallException.cs in CoreCLR CoreLib build. |
| src/coreclr/System.Private.CoreLib/src/System/Variant.cs | Add out QCallException to COM VARIANT-related QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/ValueType.cs | Add out QCallException to ValueType helper QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/TypeLoadException.CoreCLR.cs | Add out QCallException to message-formatting QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Threading/ObjectHeader.CoreCLR.cs | Add out QCallException to lock-object QCall. |
| src/coreclr/System.Private.CoreLib/src/System/String.CoreCLR.cs | Add out QCallException to string intern/is-interned QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CreateUninitializedCache.CoreCLR.cs | Add out QCallException to create-uninitialized info QCall. |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs | Add out QCallException to multiple RuntimeType/reflection QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeType.BoxCache.cs | Add out QCallException to boxing cache QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.CoreCLR.cs | Add out QCallException to wrapper-cache registration QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs | Add out QCallException to ObjC marshal QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.CoreCLR.cs | Add out QCallException to NativeLibrary load QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Java/JavaMarshal.CoreCLR.cs | Add out QCallException to Java marshal QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs | Add out QCallException to GCHandle QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreCLR.cs | Add out QCallException to ComWrappers QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/InternalCalls.cs | Add out QCallException to EH-related QCall internal calls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs | Add out QCallException to dependent handle QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs | Add out QCallException to ThreadNative_Abort QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/VirtualDispatchHelpers.cs | Add out QCallException to virtual dispatch resolver QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticsHelpers.cs | Add out QCallException to thread statics QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallException.cs | Introduce managed QCall exception status type + custom marshaller. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/InitHelpers.cs | Add out QCallException to init-class QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericsHelpers.cs | Add out QCallException to generic handle worker QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs | Add out QCallException to cast helper QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs | Add out QCallException to async continuation QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.CoreCLR.cs | Add out QCallException to unsafe accessor resolver QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs | Add out QCallException to module name/types QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs | Add out QCallException to custom attribute QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBase.CoreCLR.cs | Add out QCallException to GetCurrentMethod QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs | Add out QCallException to metadata update QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs | Add out QCallException to raw metadata QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/MdImport.cs | Add out QCallException to metadata import enum QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/LoaderAllocator.cs | Add out QCallException to loader allocator destroy QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimePropertyBuilder.cs | Add out _ to Reflection.Emit QCall invocations. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeParameterBuilder.cs | Add out _ to Reflection.Emit QCall invocations. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeMethodBuilder.cs | Add out _ to Reflection.Emit QCall invocations. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeFieldBuilder.cs | Add out _ to Reflection.Emit QCall invocations. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeEventBuilder.cs | Add out _ to Reflection.Emit QCall invocations. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeAssemblyBuilder.cs | Add out QCallException to dynamic assembly creation QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.CoreCLR.cs | Add out _ to compile-method QCall invocation. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicILGenerator.cs | Add out _ to destroy handle QCall invocation. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/AssemblyName.CoreCLR.cs | Add out QCallException to assembly spec init QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Assembly.CoreCLR.cs | Add out QCallException to assembly lookup QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs | Add out _ to clone allocation QCall invocation. |
| src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs | Add out QCallException to overridden-check QCall. |
| src/coreclr/System.Private.CoreLib/src/System/IO/FileLoadException.CoreCLR.cs | Add out QCallException to file load message QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs | Add out QCallException to exception-related QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Environment.CoreCLR.cs | Add out QCallException to Environment QCalls (Exit/FailFast/GetProcessorCount). |
| src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs | Add out QCallException to enum values/names QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackTrace.CoreCLR.cs | Add out QCallException to stack trace frames QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackFrame.CoreCLR.cs | Add out QCallException to method-desc-from-IP QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Diagnostics/Debugger.cs | Add out QCallException to debugger break/notification QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs | Add out QCallException to delegate binding/construct QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/ComAwareWeakReference.CoreCLR.cs | Add out QCallException to COM weak ref QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/CLRConfig.cs | Add out QCallException to CLR config QCall. |
| src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs | Add out QCallException to array creation/ctor QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/ArgIterator.cs | Add out QCallException to varargs iterator QCalls. |
| src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs | Add out QCallException to host property QCall. |
| src/coreclr/System.Private.CoreLib/src/Internal/VersionResilientHashCode.CoreCLR.cs | Add out QCallException to version-resilient hash QCall. |
| src/coreclr/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs | Add out QCallException to throw-helper QCalls. |
| src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs | Add conditional out _ to EH QCall internal calls. |
| src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs | Add conditional out _ to EH QCall internal calls. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 17, 2026
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
jkotas
reviewed
Aug 17, 2026
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 97d405d-8e39-4e76-afe4-2f94b74ecfa2
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 171 out of 171 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallException.cs:26
QCallExceptionMarshaller.FromUnmanagedonly treatsexceptionPending == 1as an exception. Since the native representation is anINT32flag, using a non-zero check (!= 0) is more robust against future expansion or accidental non-1 values, and avoids silently skipping exception propagation if the flag is ever set to another non-zero value.
Add managed forwarding overloads for helpers invoked by native IL stub generation so their CoreLib binder signatures remain stable after adding QCall exception parameters. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 97d405d-8e39-4e76-afe4-2f94b74ecfa2
Clean up accidental formatting and redundant conditionals introduced by the QCall migration. Make the COM stack trace assertion tolerate non-frame and additional lines while preserving frame-order validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 31769fe8-86a0-4379-91f3-5353097eb47c
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs:794
GetMethod(...)can return null, which would make failures here surface as a NullReferenceException later (or even pass silently if callers change). Since this helper is only for tests, it’s better to assert the MethodInfo is found so failures are clear and localized.
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallExceptionStatus.cs:45- The
<= 2check is a protocol constant that’s easy to lose track of when the native side evolves. Consider naming it as a constant so the intent is explicit and future updates are harder to miss.
- Files reviewed: 155/155 changed files
- Comments generated: 1
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
Restore normal native QCall return values and pass exception status through a hidden final parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3bf4a72c-55aa-4b64-8dc3-c0fbeb82730c
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CoreCLR’s QCall exception-handling ABI and dispatch machinery across a wide set of managed and native entrypoints, making the regression surface too large to approve without focused human validation.
Review details
- Files reviewed: 155/155 changed files
- Comments generated: 1
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
Member
Was there a specific problem with HiddenReturnValue that made you switch to HiddenLastParameter? |
jkotas
reviewed
Sep 2, 2026
Uh oh!
There was an error while loading. Please reload this page.
jkotas
reviewed
Sep 2, 2026
Uh oh!
There was an error while loading. Please reload this page.
Avoid relying on GET_THREAD and explicit MAKE_CURRENT_THREAD_AVAILABLE calls in QCall exception and reflection paths. Retrieve the current thread directly with GetThread, and correct the QCall casing in the exception rethrow helper name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3bf4a72c-55aa-4b64-8dc3-c0fbeb82730c
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The generated WASM callhelpers tables appear to drop the CompressionNative_CompressBound entry while managed code still imports it, which risks runtime entrypoint resolution failures on browser/WASI.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallExceptionStatus.cs:45
- QCallExceptionStatusMarshaller treats status values 1 and 2 as special cases, but the code currently uses a hard-coded magic number ("<= 2"). This makes the reserved-status contract harder to maintain and audit.
Consider introducing named constants (e.g., QCallOutOfMemoryExceptionStatus = 1, QCallStackOverflowExceptionStatus = 2, and/or MaxSpecialExceptionStatus = 2) and using them in the comparison.
src/coreclr/vm/wasm/browser/callhelpers-pinvoke.cpp:22
- CompressionNative_CompressBound was removed from the generated WASM callhelpers P/Invoke table, but managed code still declares a [LibraryImport] for it (Interop.ZLib.compressBound). If this entry is missing at runtime, CompressionNative_CompressBound resolution is likely to fail (EntryPointNotFound / missing symbol) on browser/WASI.
Please ensure the callhelpers generator includes CompressionNative_CompressBound in both browser and WASI outputs (and the table counts match).
- Files reviewed: 155/155 changed files
- Comments generated: 0 new
- Review effort level: Lite
Keep the CompressionNative_CompressBound entry in the browser and WASI static P/Invoke tables and update their entry counts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3bf4a72c-55aa-4b64-8dc3-c0fbeb82730c
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The marshaller’s special-status handling can preserve stale OutOfMemoryException stack traces due to reuse of the preallocated instance, which should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 155/155 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+41
to
+61
| Exception exception; | ||
| if ((nuint)status <= 2) | ||
| { | ||
| exception = Thread.GetQCallSpecialException(status); | ||
| } | ||
| else | ||
| { | ||
| GCHandle exceptionHandle = GCHandle.FromIntPtr(status); | ||
| try | ||
| { | ||
| exception = (Exception)exceptionHandle.Target!; | ||
| } | ||
| finally | ||
| { | ||
| exceptionHandle.Free(); | ||
| } | ||
| } | ||
| // Throw during unmarshalling so QCall exception propagation remains as close as | ||
| // possible to throwing directly from native code, as QCalls did previously. | ||
| ExceptionDispatchInfo.Throw(exception); |
Comment on lines
+60
to
+65
| internal static Exception GetQCallSpecialException(nint status) | ||
| { | ||
| Exception? exception = null; | ||
| GetQCallSpecialException(status, ObjectHandleOnStack.Create(ref exception)); | ||
| return exception!; | ||
| } |
jkotas
reviewed
Sep 3, 2026
| // [DllImport(JitHelpers.QCall, EntryPoint = "FooNative_Bar", CharSet = CharSet.Unicode)] | ||
| // // QCalls should always be static extern. | ||
| // private static extern bool Bar(int flags, string inString, StringHandleOnStack retString); | ||
| // // All QCalls that use BEGIN_QCALL must use the hidden last parameter error handler. |
Member
There was a problem hiding this comment.
This is also described in docs\design\coreclr\botr\corelib.md . Can we update that copy as well? (Or deduplicate the description by replacing one of the copies with a link.)
jkotas
reviewed
Sep 3, 2026
| { | ||
| "System.StubHelpers.StubHelpers.ThrowInteropParamException", | ||
| "Server.Contract.IStringTesting.Reverse_LPWStr_OutAttr", | ||
| "Xunit.Assert.RecordException", |
Member
There was a problem hiding this comment.
This is likely to be unreliable with JIT stress when some of the methods can be aggressively inlined and no longer show up in the stacktrace.
jkotas
commented
Sep 3, 2026
Member
LGTM otherwise |
jkotas
reviewed
Sep 3, 2026
| internal static unsafe partial class ThrowHelpers | ||
| { | ||
| [DoesNotReturn] | ||
| [ErrorHandler(typeof(QCallExceptionStatusMarshaller), ErrorLocation.HiddenLastParameter)] |
Member
There was a problem hiding this comment.
(Reposting earlier question since it got lost in the comment noise)
Was there a specific problem with HiddenReturnValue that made you switch to HiddenLastParameter?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LibraryImportsupport for custom error handlers sourced from the native return value, final parameter, or injected hidden return/parameter locationsBEGIN_QCALL/END_QCALL, while preserving their existing managed signaturesQCallExceptionStatusand move ordinary native return values to explicit out parametersOutOfMemoryException/StackOverflowExceptioninstances or a handle to the exact managed exceptionTesting
build.cmd clr+libs -c Checkedbuild.cmd mono+libsbuild.cmd clr+libs -os browser -c Debugbuild.cmd clr.corelib -os osx -a x64 -c Debugbuild.cmd clr.runtime -a x86 -c Checkedsrc\tasks\WasmAppBuilder\generate-coreclr-helpers.cmd -c DebugAmbiguousImplementationExceptionruntime test, including repeated exception propagationRuntimeHelpersTests.QCallExceptionStatus_ThrowsSpecialExceptionRuntimeHelpersTests.QCallExceptionStatus_ThrowsExactExceptionFromHandleBEGIN_QCALLwere convertedgit diff --checkNote
This pull request description was generated by GitHub Copilot.