Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/5.0] Take SuppressGCTransition into account in IL stub caching #47619

Merged

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Jan 29, 2021

Issue: #46184
Fix in master: #47320

This is a stripped down version of the fix that went into master and the change it depended on: adds the NDIRECTSTUB_FL_SUPPRESSGCTRANSITION flag, updates the P/Invoke stub flags based on SuppressGCTransition, and uses the updated flags in the IL stub cache.

Customer Impact

When this issue results in the runtime suppressing the GC transition for P/Invokes that should not have them suppressed, users may hit GC starvation, data corruption, or runtime termination. When the runtime doesn't suppress the transition when it should, users will miss getting the performance benefits expected from suppressing the transition.

This is difficult to diagnose and does not have a reasonable workaround (only options would be to never use SuppressGCTransition or to re-work P/Invokes such that they do not have the same signature).

Example based on the customer-reported issue:

private class NativeLib
{
    [SuppressGCTransition]
    [DllImport(nameof(NativeLib), CallingConvention = CallingConvention.Cdecl)]
    public static extern bool set_callback(void* callback);

    [DllImport(nameof(NativeLib), CallingConvention = CallingConvention.Cdecl)]
    public static extern bool invoke_callback(void* args);
}

[UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })]
private static void Callback(IntPtr args) { ... }

static void Main(string[] _)
{
    NativeLib.set_callback((delegate* unmanaged[Cdecl]<IntPtr, void>)&Callback);

    // IL stub from set_callback is reused. GC transition is suppressed. Fatal error occurs trying to invoke callback. 
    NativeLib.invoke_callback(IntPtr.Zero.ToPointer());
}
typedef void (__cdecl *CALLBACK_FN)(void *args);
CALLBACK_FN g_cb;

extern "C" BOOL __declspec(dllexport) set_callback(CALLBACK_FN cb)
{
    g_cb = cb;
    return TRUE;
}

extern "C" BOOL __declspec(dllexport) invoke_callback(void* args)
{
    if (g_cb == nullptr)
        return FALSE;

    g_cb(args);
    return TRUE;
}

Testing

Tests added to verify that P/Invokes with the same signature, differing only in SuppressGCTransition, do not re-use the same IL stub.

Risk

This is a fairly targeted fix, but it does affect IL stub caching and the stub flags used during generation,

Regression

No. SuppressGCTransition was introduced in 5.0.

* Use updated stub flags (taking into account SuppressGCTransition) when caching
* Add tests
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. We will take for consideration in .NET 5.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jan 29, 2021
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 2, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.4 Feb 2, 2021
@Anipik Anipik merged commit b222d5c into dotnet:release/5.0 Feb 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 12, 2021
@elinor-fung elinor-fung deleted the stubCache-suppressGCTransition-port branch April 8, 2021 17:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Interop-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants