Update Stepping through GenericPInvokeCalli Stub under the Debugger

[mikelle-rogers]

Create a trigger and new trace type for the GenericPInvokeCalli stub.

[mikelle-rogers]

In the case where the IL Stub is already made, it does not go down this path even though the trigger is enabled. I need to figure out how to disable the trigger in that scenario.

[mikelle-rogers]

When the ILStub is made, the path of execution is through JIT_ReversePInvokeEnterRare2 and into TraceCall. Eventually, a managed patch is set and then DisableAll is called, so the GenericPInvokeCalli trigger will be disabled with that call.

[AaronRobinsonMSFT]

Is there some state we can assert here or there?

In the case where the IL Stub is already made

What is this path then? How is this path different from the other, with respect to the IL Stub state?

[mikelle-rogers]

After returning from the exception that was the step into, we eventually make it to HCIMPL1_RAW(void, JIT_ReversePInvokeEnter, ReversePInvokeFrame* frame), which is some kind of jitHelper. This calls into JIT_ReversePInvokeEnterRare2, which calls into TraceCall. We then go into DispatchTraceCall, which calls TriggerTraceCall because we are working with a LEAF_MOST_FRAME. We go into an if statement with TraceStub, FollowTrace and PatchTrace. TraceStub decides we are managed code and followTrace agrees and a managed trace patch is set in PatchTrace. All three functions (TS, FT and PT) returned true, so inside of TriggerTraceCall, we DisableAll() which disables all triggers.

[noahfalk]

I'm a bit confused by the description here. In the code above it looks like we are creating an ILStub that facilitates a call from managed code to native code (a pinvoke). However in your description you say the code is executing JIT_ReversePInvokeEnter which is code I'd expect to run when a native function is calling a managed function, the reverse of a pinvoke. I'd like to understand better what does the test case you are running look like and what methods are calling one another. My worry is that you might have a test-case that is relying on the reverse pinvoke to be successful, but some other test case that doesn't include the reverse pinvoke might fail.

[mikelle-rogers]

This is the test case I am using:

[UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] 
static int DoubleInt(int i) => i * 2;
[UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] 
static float MultiplyInt (float a, float b) => (a * b);
static unsafe void Function()
{
    var fnPtr = (delegate* unmanaged[Cdecl]<int,int>) &DoubleInt;
    var fnPtr2 = (delegate* unmanaged[Cdecl]<float,float, float>) &MultiplyInt;
    fnPtr(21); // step into me
    Console.WriteLine("We are in the middle");
    fnPtr2(1.5f, 2.5f); //
    fnPtr(34); // step into me
    Console.WriteLine("We made it");
}
Function();

The case discussed above happens when we step into the following line: fnPtr(34);

[noahfalk]

@AaronRobinsonMSFT - does it match your understanding that a calli to managed method marked UnmanagedCallersOnly would first do a pinvoke followed by a reverse p/invoke? That sounds plausible to me but I've never looked into the details of the implementation.

@mikelle-rogers - I'd suggest trying an alternate test case where you write the native function in a separate dll using C++ and call that. I'd expect you will find that the reverse pinvoke part disappears. @AaronRobinsonMSFT - do you know of any good examples of this? I poked around a little bit but of the few examples I saw it was always passing a managed callback to native code rather than passing a native callback to managed code.

[AaronRobinsonMSFT]

UnmanagedCallersOnly would first do a pinvoke followed by a reverse p/invoke?

@noahfalk When you say "do a pinvoke", what does that mean in this case? Meaning, what part of the machinery are you referring to as pinvoke?

[noahfalk]

Sorry, let me try to be more precise :) By 'Do a pinvoke' I was refering to the calli executing down the code path commented here GetILStubForCalli(...), followed by invoking the ILStub that is generated in this method. PInvoke may not have been accurate terminology for that part. Does that make more sense?

[dotnet-policy-service]

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

[tommcdon]

Should we check the return value here and conditionally execute PatchTrace?

[mikelle-rogers]

In this case, FollowTrace should always return True. Should we assert that that is the case? Then, if there is an issue (or a case that I do not know about) we will find it easily.

[tommcdon]

Same comment as above, check the return value? If it fails, should we avoid calling DisableGenericPInvokeCalli?

[mikelle-rogers]

The same goes for this one. PatchTrace should return true always, so maybe we assert that it has? And if it hasn't then that possible provides us with an easy issue to track?

[tommcdon]

Left a couple comments for your review, but otherwise LGTM!

[noahfalk]

Nit: it seems a little odd to have an unrelated assert in between the comment and the line it is commenting on

Suggested change

	//fStopInUnmanaged only matters for TRACE_UNMANAGED
	_ASSERTE(hasTraceType);
	//fStopInUnmanaged only matters for TRACE_UNMANAGED
	bool setPatch = PatchTrace(&trace, fp, /*fStopInUnmanaged*/false);

[stephentoub]

@tommcdon, what are the next steps for this PR? Thanks.

[tommcdon]

@tommcdon, what are the next steps for this PR? Thanks.

@stephentoub I believe there was some testing that needs to done and if that looked good this was very close to ready. @noahfalk did you have any other concerns?

[noahfalk]

@noahfalk did you have any other concerns?

Based on #110677 (comment) it wasn't clear that we've gotten calls into the correct runtime code paths. Mikelle's explanation sounded like the current code relied on a reverse p/invoke to occur during the operation but I wouldn't expect all scenarios involving calli to contain a reverse p/invoke. Testing a call to a pure C++ dll may reveal changes are needed.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "5967cc84f10dd329530def6f3daafa7be80350dd",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "1d69684de8801d500e12de8efd86e7ecfa5ed168",
  "last_reviewed_commit": "5967cc84f10dd329530def6f3daafa7be80350dd",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "1d69684de8801d500e12de8efd86e7ecfa5ed168",
  "last_recorded_worker_run_id": "29672707509",
  "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": "5967cc84f10dd329530def6f3daafa7be80350dd",
      "review_id": 4729989152
    }
  ]
}

[github-actions]

Holistic Review

Motivation: When stepping under the debugger into an unmanaged calli (GenericPInvokeCalliHelper), the IL stub used to marshal the call is created lazily inside GetILStubForCalli. Because that stub doesn't exist yet at trace time, the debugger previously had no reliable way to place a step-in patch and would step over the call. This PR adds a dedicated trace type and trigger so the debugger can follow into the generic P/Invoke calli stub, mirroring the existing MulticastDelegate and ExternalMethodFixup step-through mechanisms.

Approach: A new TRACE_GENERIC_PINVOKE_CALLI trace type is added to TraceType, with TraceDestination::InitForGenericPInvokeCalli and string mappings. InteropDispatchStubManager::DoTraceStub now emits this trace type specifically for GenericPInvokeCalliHelper (instead of a generic manager push). A global refcount g_genericPInvokeCalliHelperTraceActiveCount is incremented/decremented as controllers enable/disable the new trace, following the exact externalMethodFixup pattern (controller flag m_genericPInvokeCalli, Enable/Disable/Dispatch/Trigger methods, DebuggerController::PatchTrace case, DisableAll cleanup, and a dbginterface/Debugger GenericPInvokeCalliNextStep hook). GetILStubForCalli in dllimport.cpp calls back into the debugger once the IL stub is materialized when the refcount is nonzero, letting DebuggerStepper::TriggerGenericPInvokeCalli follow the trace into the freshly created stub.

Summary: The change is well-scoped and closely follows the established ExternalMethodFixup step-through infrastructure, which is the right model. The wiring (trace type, refcount, controller flag, dispatch/trigger, EE callback) is consistent and correct. Two issues are worth addressing before merge: (1) DebuggerStepper::TriggerGenericPInvokeCalli adds _ASSERTEs on the FollowTrace/PatchTrace return values that the analogous external-method trigger deliberately omits, and both can legitimately be false for non-patchable/unmanaged targets (a common calli scenario), risking spurious checked-build asserts. (2) A stray whitespace-only line was introduced. Neither is a correctness bug in retail builds; the assert concern is the more important of the two. I could not build or run tests in this environment, and I did not find an accompanying debugger step-through test for the new path — confirming coverage (e.g. via the diagnostics/stepping tests) would strengthen the change.

Detailed Findings

• src/coreclr/debug/ee/controller.cpp (TriggerGenericPInvokeCalli) — overly strict _ASSERTE(hasTraceType) / _ASSERTE(setPatch); see inline comment. Recommend matching TriggerExternalMethodFixup, which ignores both return values.
• src/coreclr/debug/ee/controller.cpp — stray tab-only blank line and inconsistent blank-line spacing between the new helper definitions; see inline comment.
• Include layering (src/coreclr/vm/dllimport.cpp) — adding #include "../debug/ee/debugger.h" to reach g_pDebugger->GenericPInvokeCalliNextStep matches the existing precedent in prestub.cpp for ExternalMethodFixupNextStep, so this is consistent. No action needed.
• Placement of the EE callback — the callback fires after UNINSTALL_UNWIND_AND_CONTINUE_HANDLER using the interlocked pVASigCookie->pNDirectILStub, which is the committed stub address; this is correct and if anything safer than the pre-uninstall placement used for external-method fixup. No action needed.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 154.1 AIC · ⌖ 11.8 AIC · ⊞ 10K

[github-actions]

Nit: this line contains only two stray tab characters (trailing whitespace on an otherwise blank line). Please remove it so there is a single clean blank line between DispatchExternalMethodFixup and EnableGenericPInvokeCalli. Also consider adding a blank line between the new EnableGenericPInvokeCalli, DisableGenericPInvokeCalli, and DispatchGenericPInvokeCalli definitions for consistency with the surrounding external-method helpers.

[github-actions]

These two _ASSERTEs are stricter than the analogous TriggerExternalMethodFixup (which ignores both return values), and both can legitimately be false in a checked build:

• FollowTrace returns FALSE whenever the resolved trace type is TRACE_OTHER (see StubManager::FollowTrace), e.g. when the IL stub target lands in an EE helper with no managed code to step into. _ASSERTE(hasTraceType) would then fire spuriously.
• PatchTrace returns false for several non-patchable destinations (TRACE_OTHER, TRACE_UNMANAGED when fStopInUnmanaged is false, etc.), so _ASSERTE(setPatch) can also fire.

Since this callback runs whenever a step is active over GenericPInvokeCalliHelper, an unmanaged CALLI target (a very common case for calli) can hit exactly these paths. Consider dropping the asserts (matching TriggerExternalMethodFixup) or only asserting after confirming the trace type is actually patchable, so debugging a calli step doesn't assert-crash checked runtimes.