Verify limitation in Compiler::impCanPInvokeInlineCallSite still applies

[fadimounir]

https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/importer.cpp#L6358

Revisit the ifdef'd block on 64-bit, and understand if we still need it. And if so, verify there's test coverage for it.

Context: #327

category:implementation
theme:pinvoke
skill-level:beginner
cost:small

[AndyAyersMS]

Adding a permalink...

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

Lines 6456 to 6490 in 4becc75

	#ifdef TARGET_64BIT
	// On 64-bit platforms, we disable pinvoke inlining inside of try regions.
	// Note that this could be needed on other architectures too, but we
	// haven't done enough investigation to know for sure at this point.
	//
	// Here is the comment from JIT64 explaining why:
	// [VSWhidbey: 611015] - because the jitted code links in the
	// Frame (instead of the stub) we rely on the Frame not being
	// 'active' until inside the stub. This normally happens by the
	// stub setting the return address pointer in the Frame object
	// inside the stub. On a normal return, the return address
	// pointer is zeroed out so the Frame can be safely re-used, but
	// if an exception occurs, nobody zeros out the return address
	// pointer. Thus if we re-used the Frame object, it would go
	// 'active' as soon as we link it into the Frame chain.
	//
	// Technically we only need to disable PInvoke inlining if we're
	// in a handler or if we're in a try body with a catch or
	// filter/except where other non-handler code in this method
	// might run and try to re-use the dirty Frame object.
	//
	// A desktop test case where this seems to matter is
	// jit\jit64\ebvts\mcpp\sources2\ijw\__clrcall\vector_ctor_dtor.02\deldtor_clr.exe
	if (block->hasTryIndex())
	{
	// This does not apply to the raw pinvoke call that is inside the pinvoke
	// ILStub. In this case, we have to inline the raw pinvoke call into the stub,
	// otherwise we would end up with a stub that recursively calls itself, and end
	// up with a stack overflow.
	if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && opts.ShouldUsePInvokeHelpers())
	{
	return true;
	}
	return false;

Seems like we could still hit the problem described in the comment on platforms where we have exception interop. Would be good to verify with an actual test.

Assuming the concern is valid, seems like we could zero out the inline frame return address on entry to any handler or filter in the method to make sure the frame object is properly reset if an exception happens, instead of banishing inline pinvoke. Probably worth some prospecting to see the impact of this on our current codegen.

[AndyAyersMS]

I'm going to push this out of 5.0 as it's a nice to have.

[jakobbotsch]

@jkoritzinsky Should this be closed in favor of #70109?

[jkoritzinsky]

Yes, I think so