Do not setup both an inlined call frame and perform a reverse pinvoke…

[davidwrighton]

… in the same function

• Pushing/popping the frame chain requires that the thread be in cooperative mode
• Before the reverse pinvoke logic engages, the thread is in preemptive mode
• The current implementation of InlinedCallFrame and reverse p/invoke sets up the InlinedCallFrame before the reverse p/invoke transition, which is unsafe
• A reverse pinvoke function directly calling back into native code is fairly rare, and so optimizing the situation is of marginal utility.
• The fix is to use the pinvoke helpers logic to setup the pinvoke instead of relying on the InlinedCallFrame logic. This avoid the problem of incorrect ordering in the prolog/epilog, but moving inlined call frame handling to point of use.

Fix bug #45326

[davidwrighton]

@dotnet/jit-contrib I'd like to have somebody from the JIT team review this.

@MichalStrehovsky Does this cause problems for NativeAOT? I don't know if the native aot project has these helpers.

[jkotas]

@MichalStrehovsky Does this cause problems for NativeAOT? I don't know if the native aot project has these helpers

It will work just fine for NativeAOT. NativeAOT always uses JIT_FLAG_USE_PINVOKE_HELPERS.

[jkotas]

The current implementation of InlinedCallFrame and reverse p/invoke sets up the InlinedCallFrame before the reverse p/invoke transition, which is unsafe

It would be better to reorder these instead. I assume that it is too hard to do for some reason?

[tannergooding]

A reverse pinvoke function directly calling back into native code is fairly rare, and so optimizing the situation is of marginal utility.

Is this actually rare? Things like the WNDPROC callback for Win32/WinForms is a reverse P/Invoke and that may itself query some information from native to access more data.

Edit Just browsing https://source.dot.net/#q=WndProc shows a number of handlers that themselves call P/Invokes relatively early.

[davidwrighton]

@jkotas reordering them would certainly be a better choice, but it isn't trivial for me, as I'm no expert in the details of how the JIT really works. As the reverse p/invoke is added to the prolog/epilog in the flowgraph stage, and the inlined call frame logic is handled during lower; however, I'm not comfortable making such a change myself, as the fix looked to be more involved than I was prepared to handle, and prolog/epilog code is notoriously fussy to get right. That said, if this gets checked in in the current state, I'll open an issue against the JIT team to look at coming up with the more optimal fix. I've been able to get this to reproduce reliably by running the determinism test in a loop under jitstress1 mode, so verifying a more complex fix is not a difficult task.

@tannergooding Rare is possibly the wrong word.... possibly, less common would be a better term. There are certainly scenarios in which the reverse pinvoke to pinvoke logic is in the same function, but I don't see that its all THAT common. (Also, the penalty from using the helper functions isn't terrible.) I'm pretty sure that we don't have significant test coverage of this scenario in our test beds as the characteristic assertion messages are quite rare.

[EgorBo]

Codegen diff example:

[UnmanagedCallersOnly]
static void Test() => Thread.Yield();

diff: https://www.diffchecker.com/PZkiY22r

I'm not a pinvoke expert, but I can try to investigate how to still have inlined call frame here if CORINFO_HELP_JIT_PINVOKE_BEGIN helpers noticeably affect performance (and for .NET 7.0 I guess) for this specific case.