Enable inlining P/Invokes in exception clauses

[jkoritzinsky]

To fully complete #69758, we need to enable P/Invoke inlining in more places to ensure we generate zero IL stubs on startup for many target scenarios. Today, we don't inline P/Invokes for a few cases:

• P/Invoke in a try block
• P/Invoke in a catch clause
• P/Invoke in a filter clause
• P/Invoke in a finally clause
• P/Invoke in a cold (rarely executed) block

To get to zero managed->native IL stubs with a Hello World program where we use dotnet trace collect to track P/Invoke stub generation, we need to enable inlining P/Invokes in try blocks and finally clauses. We'll likely want to also enable inlining P/Invoke stubs into catch and filter clauses for more complex scenarios. We don't want to enable inlining P/Invokes in cold blocks as that has caused major perf regressions in the past.

This work will require both exception handling work to enable correctly handling exception interop on Windows (particularly handling unwinding exactly to the method that has the InlinedCallFrame), as well as some simple JIT work to remove blocking the inlining of these IL stubs.

cc: @dotnet/interop-contrib

[jkoritzinsky]

I have a test suite we can use for testing the exception interop behavior here. I'll put it out in a PR and reference it here so we can validate our behavior.

[jkotas]

we need to enable inlining P/Invokes in try blocks and finally clauses

It is pretty difficult to do this. I am not sure whether it is worth the complexity.

Can we generate "no-op" stubs that just repush all arguments for these cases instead?

[jkoritzinsky]

We can, but it does remove some of the benefits of removing the crossgen2 support for emitting P/Invoke IL stubs since we can’t remove the infrastructure.

It would be a reasonable first step to keep blittable-only support in crossgen2 (I can put up a PR for that), but I wouldn’t want to close the linked issue until we fully remove support from crossgen2.

[jkotas]

P/Invoke in a try block

This limitation is probably the easiest one to fix. #330 (comment) talks about it.

[jkoritzinsky]

I took a look at fixing that, and I hit some issues. Even after fixing that, there’s an additional issue. The problem comes when there’s a catch clause in the same frame as the inlined P/Invoke. It seems in this case, the stackwalker is able to unwind past the inlined call frame correctly to the managed method frame, but it clears the info about the non-volatile registers and doesn’t restore them correctly, which breaks the EH support for resuming the method after the catch clause.

Once I figured this out, I decided to file this issue before trying to fix that.

[jkoritzinsky]

After more investigation, it looks like InlinedCallFrame explicitly doesn't support restoring callee-saved registers (likely because we already ensure that no GC refs live across the call, and that combined with not supporting inlining P/Invokes in try blocks has led us to never need them), and in ExceptionTracker::FindNonvolatileRegisterPointers in exceptionhandling.cpp, there's a comment that assumes that the frame object will restore the nonvolatile registers.

runtime/src/coreclr/vm/exceptionhandling.cpp

Lines 385 to 390 in 61ca87c

	// Find the highest frame below the resume frame that will update the
	// REGDISPLAY. A normal StackWalkFrames will RtlVirtualUnwind through all
	// managed frames on the stack, so this avoids some unnecessary work. The
	// frame we find will have all of the nonvolatile registers/other state
	// needed to start a managed unwind from that point.
	//

Would it be worthwhile to save the callee-saved registers in the InlinedCallFrame when we're in a try block? Maybe by creating a TransitionBlock (or CalleeSavedRegisters block) inline and adding a pointer to it into the InlinedCallFrame?

An alternative solution would be to update the stackwalker to manually unwind native frames as it sees them to keep the non-volatile registers accurate.

[jkoritzinsky]

cc: @janvorli for additional ideas as he knows this space much better than I.

[jkotas]

Would it be worthwhile to save the callee-saved registers in the InlinedCallFrame when we're in a try block?

We do depend on the JIT to save the callee-saved registers for efficiency and simplicity. I do not think we would want to start saving the callee-saved registers in InlinedCallFrame.

The regular built-in PInvoke stubs can have exception handling and they work fine (?). What is the delta between the built-in stubs and the case that's broken?

[jkoritzinsky]

Yes, there is a slight difference. We never catch an exception and resume execution in the same IL stub that we have an InlinedCallFrame. We only emit catch blocks in Reverse P/Invokes. The break we’re currently facing is trying to resume execution after catching the exception in the same function as the P/Invoke call.

[jkotas]

It means that we should be able to relax

runtime/src/coreclr/src/jit/importer.cpp

Line 6479 in 4becc75

if (block->hasTryIndex())

to allow inlining in try/finally blocks. It is not a complete fix, but it will give us parity with the built-in interop.

[jkoritzinsky]

That will get us parity for built-in interop, but it doesn't solve the problem with the runtime-generated blittable IL stubs that show up if we remove IL stub generation from crossgen2, which is the main impetus behind this issue.

[jkotas]

The parity with built-in interop is interesting for throughput perf. If the source-generated interop has try/finally, it will get an extra frame at runtime that is not free. We may not be seeing it in our micro-benchmarks, but somebody will notice sooner or later.

I do not think we want to force the blittable stubs to be always inlineable by fixing all limitations you have listed. It is not profitable for cold code (catch blocks, filter blocks, cold based on profile data). That leaves try clauses associated with finally that should just work, and finally clauses where the PInvoke inlining is complicated.

[jkoritzinsky]

My goal with this work is to eventually support inlining in all try blocks (including ones with catch clauses) and finally clauses. I agree that inlining into catch, filter, or profiled cold blocks is likely not profitable or worthwhile.

[mangod9]

Hi @jkoritzinsky is this still being worked on for 7?

[jkoritzinsky]

I'm working on this in #73661. I have one last failure that I'm going to try to investigate today.

[AaronRobinsonMSFT]

I don't think we should be pushing on this at this point. There is no .NET 7 requirement and I'd prefer to defer this to .NET 8

[jkoritzinsky]

Sounds good.

[jkoritzinsky]

Deferring to Future as we're not going to get to this in .NET 8 and future work in the exception handling space may change the difficulty of this work significantly (either making it easier or more difficult, we'll see).