Fix EH handling in PInvoke stubs and remove workaround #327

fadimounir · 2019-11-27T01:44:07Z

Fix for #248
Reverts workaround from #249

The fix in the JIT is to inline the raw PInvoke call if:

We are compiling a PInvoke IL Stub
We are compiling the stub without a secret parameter

In crossgen1/jit, the raw pinvoke call is a calli operation that uses the secret parameter passed to the stub, and gets inlined by-design (we don't even call impCanPInvokeInlineCallSite for it).

With crossgen2, the secret parameter is not used and we have a separate pinvoke stub for each pinvoke. Each pinvoke IL stub generated by crossgen2 will have some marshaling operations if applicable, and a call to the raw pinvoke target (which gets wrapped by calls to JIT_PInvokeBegin/End helpers). This raw pinvoke call has to be inlined for IL stubs, even in the presence of EH blocks due to marshaling, otherwise we would get an IL stub that infinitely recursively calls itself because it can't inline the actual pinvoke call.

cc @dotnet/crossgen-contrib @dotnet/jit-contrib

The fix in the JIT is to inline the raw PInvoke call if: 1) We are compiling a PInvoke IL Stub 2) We are compiling the stub without a secret parameter In crossgen1/jit, the raw pinvoke call is a calli operation that uses the secret parameter passed to the stub, but with crossgen2, that parameter is not used and we have a separate pinvoke stub for each pinvoke.

jkotas · 2019-11-27T01:55:22Z

src/coreclr/src/jit/importer.cpp

@@ -6393,6 +6393,15 @@ bool Compiler::impCanPInvokeInlineCallSite(BasicBlock* block)
    //   jit\jit64\ebvts\mcpp\sources2\ijw\__clrcall\vector_ctor_dtor.02\deldtor_clr.exe
    if (block->hasTryIndex())
    {
+        if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) &&
+            opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PUBLISH_SECRET_PARAM))


Is this condition correct?

JIT_FLAG_PUBLISH_SECRET_PARAM is never set in crossgen2. It means that this if is going to be always false in crossgen2 and this change has no effect on what crossgen2 does.

Oops... Typo probably during the repro reforking

Fixed. Thanks for catching this Jan

Do we need the JIT_FLAG_PUBLISH_SECRET_PARAM part of the condition at all? What is not going to work if the condition is just if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) ?

What is not going to work if the condition is just if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) ?

In my mind, i think it should work, but I put a more conservative check to avoid an unintentional break somewhere else. I don't know enough context outside of crossgen2 to be able to say with confidence that we should remove the check for JIT_FLAG_PUBLISH_SECRET_PARAM

Some interop stubs generated at runtime are not shared and do not have CORJIT_FLAG_PUBLISH_SECRET_PARAM. It is not reliable to use CORJIT_FLAG_PUBLISH_SECRET_PARAM to tell whether you are generating stub for crossgen2. (And even if it was reliable, it would be rather cryptic to use this flag for this purpose.)

It means that you are still potentially changing the behavior for the JIT case with this change. The potential bugs are just going to be more subtle and harder to find.

I see. Ok let me remove CORJIT_FLAG_PUBLISH_SECRET_PARAM and see if tests are going to start to fail in CI/outerloop. If all tests are passing, I'll merge without CORJIT_FLAG_PUBLISH_SECRET_PARAM.

Sounds good?

I think it would be most appropriate to change the condition to use CORJIT_FLAG_USE_PINVOKE_HELPERS (see my other comment).

CORJIT_FLAG_USE_PINVOKE_HELPERS happens to be used by crossgen only, so you can be sure that you are not changing any of the JIT behavior.

AndyAyersMS

Looks good.

It would be nice to verify that the within try check bailout is still really needed. Can you open an issue to look into that?

fadimounir · 2019-11-27T02:06:46Z

verify that the within try check bailout is still really needed

What's that?

AndyAyersMS · 2019-11-27T02:09:12Z

What's that?

Make sure we really understand why we can't just delete that whole ifdef'd block, and ideally, add a test case.

fadimounir · 2019-11-27T02:11:48Z

Make sure we really understand why we can't just delete that whole ifdef'd block, and ideally, add a test case.

Ah, yes. Will do.

jkotas · 2019-11-27T02:17:02Z

The problem described by the comment on the ifdefed block can't happen with CORJIT_FLAG_USE_PINVOKE_HELPERS because the InlinedFrame is not reused. I think the most appropriate way to change the condition would be:

if (block->hasTryIndex() && !opts.jitFlags->IsSet(JitFlags::CORJIT_FLAG_USE_PINVOKE_HELPERS))
{
    return false;
}

fadimounir · 2019-11-27T02:25:25Z

The problem described by the comment on the ifdefed block can't happen with CORJIT_FLAG_USE_PINVOKE_HELPERS. I think the most appropriate way to change the condition would be:
if (block->hasTryIndex() && !opts.jitFlags->IsSet(JitFlags::CORJIT_FLAG_USE_PINVOKE_HELPERS))
{
    return false;
}

That was my first fix attempt, and it wasn't working (see #248 for the error I got). Based on what I debugged in the EH handling, we need to teach the runtime to handle inlined pinvokes where the caller has EH blocks. The unwinding after executing the catch block wasn't working correctly.
Main issue is InlinedCallFrame::UpdateRegDisplay not filling all the registers in the context, because we don't save the state of all registers in the ICF.

This is a much more involved change

BTW, if we fix the EH handling, it will be general goodness because we'll be able to inline pinvokes with marshaling into callers.

fadimounir · 2019-11-27T02:40:39Z

Thinking about it more, I think that this fix will most likely work too, and look better (this is similar to my initial fix, which is along the line of what you're thinking too, but limited to the stubs only):

if (block->hasTryIndex())
{
    if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && opts.ShouldUsePInvokeHelpers())
        return true;

    return false;
}

I will try this out tomorrow

jkotas · 2019-11-27T03:34:18Z

Yes, that would look better. Thanks!

fadimounir requested review from AndyAyersMS and janvorli November 27, 2019 01:44

jkotas reviewed Nov 27, 2019

View reviewed changes

Fix typo

e1b08fc

AndyAyersMS approved these changes Nov 27, 2019

View reviewed changes

fadimounir mentioned this pull request Nov 27, 2019

Verify limitation in Compiler::impCanPInvokeInlineCallSite still applies #330

Closed

maryamariyan added the area-crossgen2-coreclr label Nov 27, 2019

fadimounir added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 27, 2019

fadimounir changed the title ~~Fix EH handling in PInvoke stubs and remove workaround~~ [WIP] Fix EH handling in PInvoke stubs and remove workaround Nov 27, 2019

CR feedback

bbfe05e

fadimounir removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 27, 2019

fadimounir changed the title ~~[WIP] Fix EH handling in PInvoke stubs and remove workaround~~ Fix EH handling in PInvoke stubs and remove workaround Nov 27, 2019

jkotas approved these changes Nov 27, 2019

View reviewed changes

fadimounir merged commit d47081a into dotnet:master Nov 27, 2019

fadimounir deleted the FixPinvokeEHIssue branch December 19, 2019 20:42

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EH handling in PInvoke stubs and remove workaround #327

Fix EH handling in PInvoke stubs and remove workaround #327

fadimounir commented Nov 27, 2019 •

edited

Loading

jkotas Nov 27, 2019

fadimounir Nov 27, 2019

fadimounir Nov 27, 2019

jkotas Nov 27, 2019

fadimounir Nov 27, 2019 •

edited

Loading

jkotas Nov 27, 2019

fadimounir Nov 27, 2019

jkotas Nov 27, 2019 •

edited

Loading

jkotas Nov 27, 2019

AndyAyersMS left a comment

fadimounir commented Nov 27, 2019

AndyAyersMS commented Nov 27, 2019

fadimounir commented Nov 27, 2019

jkotas commented Nov 27, 2019 •

edited

Loading

fadimounir commented Nov 27, 2019 •

edited

Loading

fadimounir commented Nov 27, 2019 •

edited

Loading

jkotas commented Nov 27, 2019

Fix EH handling in PInvoke stubs and remove workaround #327

Fix EH handling in PInvoke stubs and remove workaround #327

Conversation

fadimounir commented Nov 27, 2019 • edited Loading

jkotas Nov 27, 2019

Choose a reason for hiding this comment

fadimounir Nov 27, 2019

Choose a reason for hiding this comment

fadimounir Nov 27, 2019

Choose a reason for hiding this comment

jkotas Nov 27, 2019

Choose a reason for hiding this comment

fadimounir Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

jkotas Nov 27, 2019

Choose a reason for hiding this comment

fadimounir Nov 27, 2019

Choose a reason for hiding this comment

jkotas Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

jkotas Nov 27, 2019

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

fadimounir commented Nov 27, 2019

AndyAyersMS commented Nov 27, 2019

fadimounir commented Nov 27, 2019

jkotas commented Nov 27, 2019 • edited Loading

fadimounir commented Nov 27, 2019 • edited Loading

fadimounir commented Nov 27, 2019 • edited Loading

jkotas commented Nov 27, 2019

fadimounir commented Nov 27, 2019 •

edited

Loading

fadimounir Nov 27, 2019 •

edited

Loading

jkotas Nov 27, 2019 •

edited

Loading

jkotas commented Nov 27, 2019 •

edited

Loading

fadimounir commented Nov 27, 2019 •

edited

Loading

fadimounir commented Nov 27, 2019 •

edited

Loading