dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.26k stars 4.73k forks source link

Inliner: don't inline callees with pinvokes inside EH #79125

Open EgorBo opened 1 year ago

EgorBo commented 1 year ago
[DllImport("user32.dll")]
static extern byte MessageBeep(uint uType);

static void Foo()
{
    MessageBeep(42);
}

static void Test()
{
    try
    {
        Foo();
    }
    catch
    {
    }
}

Inliner should not inline callees with pinvokes to try/catch handlers because we never inline pinvoke machinery in JIT for those. So ^ snippet ends up generating:

  21: JIT compiled (dynamicClass):IL_STUB_PInvoke(uint):ubyte [FullOpts, IL size=34, code size=152, hash=0xeb066932]

but if we mark Foo as [MethodImpl(MethodImplOptions.NoInlining)] we won't emit IL_STUB_PInvoke

ghost commented 1 year ago

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

Issue Details
```csharp [DllImport("user32.dll")] static extern byte MessageBeep(uint uType); static void Foo() { MessageBeep(42); } static void Test() { try { Foo(); } catch { } } ``` Inliner should not inline callees with pinvokes to try/catch handlers because we never inline pinvoke machinery in JIT for those. So ^ snippet ends up generating: ``` 21: JIT compiled (dynamicClass):IL_STUB_PInvoke(uint):ubyte [FullOpts, IL size=34, code size=152, hash=0xeb066932] ``` but if we mark `Foo` as `[MethodImpl(MethodImplOptions.NoInlining)]` we won't emit `IL_STUB_PInvoke`
Author: EgorBo
Assignees: -
Labels: `area-CodeGen-coreclr`
Milestone: -
jkoritzinsky commented 1 year ago

Related issue around enabling inlining a P/Invoke inside a try block with a catch block (amongst other things around P/Invokes and exception handling): https://github.com/dotnet/runtime/issues/70109

EgorBo commented 1 year ago

@jkoritzinsky thanks, didn't notice that - do you think it's worth doing as a quick workaround (I'd expect it to be a simple change) until we support what you suggested in https://github.com/dotnet/runtime/issues/70109?

jkoritzinsky commented 1 year ago

Yes, I think it's definitely worth doing! It might also be worth avoiding inlining calls to functions that contain P/Invokes in finally blocks, as it has the same issue.

If we do both cases (don't inline call to function with P/Invoke in try or finally blocks), I think we can finally complete https://github.com/dotnet/runtime/issues/69758 and remove the P/Invoke stub emitter support from crossgen2 without causing any regressions for common use cases.