Resolve dllimport.cpp runtimeasync todos #122466

jkoritzinsky · 2025-12-11T21:16:00Z

Roslyn should never mark a P/Invoke or a delegate's Invoke method as an async method, so there's not a good way to add tests without writing them in IL and it doesn't seem particularly useful to do so.

Fixes #121758

dotnet-policy-service · 2025-12-11T21:17:17Z

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR resolves TODOs related to async method handling in the P/Invoke implementation. The changes replace placeholder TODO comments with explanatory comments about why async methods are not supported for P/Invoke scenarios, convert some assertions to preconditions, and adjust error handling in one case to defer exception throwing to the IL stub generation path.

Key changes:

Adds PRECONDITION checks for async methods in constructor contracts and function preconditions
Replaces TODO comments with clarifying comments explaining that async P/Invoke methods should use async variant stubs that call task-returning interop stubs
Changes MarshalingRequired to return TRUE for async methods (to trigger proper exception later) instead of throwing immediately

Copilot · 2025-12-11T21:21:59Z

src/coreclr/vm/dllimport.cpp

+            PRECONDITION(!pMD->IsAsyncMethod());
            PRECONDITION(callConv != NULL);
        }
        CONTRACTL_END;

        CorInfoCallConvExtension callConvLocal;
        IMDInternalImport* pInternalImport = pMD->GetMDImport();
        CorPinvokeMap mappingFlags = pmMaxValue;
-        // TODO: (async) revisit and examine if this needs to be supported somehow
-        if (pMD->IsAsyncMethod())
-            ThrowHR(COR_E_NOTSUPPORTED);
+


Replacing the runtime check with a PRECONDITION is incorrect. The removed runtime check at lines 3064 (if pMD->IsAsyncMethod() ThrowHR) was a necessary validation that throws COR_E_NOTSUPPORTED for async methods. PRECONDITION checks are typically only active in debug builds and are meant for validating programming contracts, not for runtime validation. The caller GetCallingConvention_IgnoreErrors does not validate that methods are not async before calling this function. This could allow async P/Invoke methods to proceed unchecked in release builds, leading to undefined behavior instead of a proper exception. The runtime check should be retained, similar to how DllImportInit (line 2783) and GetILStubForCalli (line 6087) handle async methods.

Copilot · 2025-12-11T21:21:59Z

src/coreclr/vm/dllimport.cpp

        GC_NOTRIGGER;
        SUPPORTS_DAC;
        PRECONDITION(pMD != NULL);
+        PRECONDITION(!pMD->IsAsyncMethod());


Inconsistent async method validation pattern between StubSigDesc constructors. The first constructor (line 103) now uses a PRECONDITION for the async check, but the second constructor at line 139 still uses an _ASSERTE inside a conditional block. For consistency and clarity, both constructors should use the same pattern. Consider either adding the PRECONDITION to the second constructor's CONTRACTL block (since pMD can be NULL in that constructor, you'd need to add it within the if block), or using the same validation approach in both places.

Copilot · 2025-12-11T21:22:00Z

src/coreclr/vm/dllimport.cpp


-        // TODO: (async) revisit and examine if this needs to be supported somehow
+        // unmanaged vararg signatures should never be marked as async.
+        // We don't support generating a interop IL stub for an async method.


Missing article 'an' before 'interop'. The comment should read "We don't support generating an interop IL stub" for grammatical correctness.

Suggested change

// We don't support generating a interop IL stub for an async method.

// We don't support generating an interop IL stub for an async method.

VSadov · 2025-12-11T21:43:50Z

Roslyn should never mark a P/Invoke or a delegate's Invoke method as an async method

Interestingly it allows Synchronized.
Like:

    [MethodImpl(MethodImplOptions.Synchronized)]
    [DllImport("kernel32.dll")]
    private static extern bool QueryPerformanceFrequency(out long frequency);

It results in

Unhandled exception. System.TypeLoadException: Synchronized attribute cannot be used with this method type.
   at Program.Main()

Perhaps MethodImplOptions.Async should result in a similar error.

In theory we could just ignore Async on DllImport methods, since we spec'd it as

The [MethodImpl(MethodImplOptions.Async)] only has effect when applied to method definitions with CIL implementation.

The runtime could be more strict than the spec and making it an error as it is the case with Synchronized might be more consistent.

VSadov · 2025-12-11T22:10:03Z

I can think of only two ways to get async involved with DllImport.

Non-task Async method.

Roslyn does not like adding Async outside of corlib/AsyncHelpers, but should be possible in IL.
Perhaps should produce the same error as with Synchronized

The runtime also does not allow non-task Async methods outside of corlib, so maybe that will error out first.

    [MethodImpl(MethodImplOptions.Async)]
    [DllImport("kernel32.dll")]
    private static extern bool QueryPerformanceFrequency(out long frequency);

Task-returning pinvokes:

    [DllImport("kernel32.dll")]
    private static extern Task QueryPerformanceFrequency(out long frequency);

    [DllImport("kernel32.dll")]
    private static extern ValueTask QueryPerformanceCounter(out long counter);

I am not sure if either of above can work right now.
Ideally the behavior with async enabled should be the same.

jkoritzinsky · 2025-12-11T23:39:50Z

@VSadov good point on MethodImpl attribute. I'll add some tests to make sure we don't crash.

…th the async bit set.

jkoritzinsky · 2025-12-15T20:30:00Z

@VSadov I've rewritten this PR to have the following implementation:

Loading a P/Invoke with the async bit set fails to load.

The async variant for a P/Invoke method (if even usable) will not be considered a P/Invoke (similar to how we handled this for COM interop).

Jitting a method with UnmanagedCallersOnly with the async bit set fails with InvalidProgramException.

dllimport.cpp asserts that it never sees a method with the async bit set.

VSadov · 2025-12-16T02:00:48Z

The [MethodImpl(MethodImplOptions.Async)] + [DllImport] scenario could be hard to test since Roslyn would not allow MethodImplOptions.Async on random methods.

The scenario like the following is testable though. It is also a preexisting scenario, so regression is a bigger deal. Perhaps we should have a test.

    [DllImport("kernel32.dll")]
    private static extern Task QueryPerformanceFrequency(out long frequency);

    [DllImport("kernel32.dll")]
    private static extern ValueTask QueryPerformanceCounter(out long counter);

Otherwise looks good to me.

jkotas · 2025-12-17T00:35:43Z

src/coreclr/dlls/mscorrc/mscorrc.rc

    IDS_EE_NDIRECT_DISABLEDMARSHAL_LCID     "The LCIDConversionAttribute is not supported when runtime marshalling is disabled."
    IDS_EE_NDIRECT_DISABLEDMARSHAL_PRESERVESIG "Setting PreserveSig to false for a P/Invoke is not supported when runtime marshalling is disabled."
    IDS_EE_NDIRECT_DISABLEDMARSHAL_VARARGS  "Using a variable argument list in a P/Invoke is not supported when runtime marshalling is disabled."
+    BFA_ASYNC_PINVOKE_METHOD                "A PInvoke method cannot be marked as async."


All BFA_.... are in a block together. Can this be part of that block? (Alternatively, do not call it BFA...)

jkotas · 2025-12-17T04:41:53Z

src/coreclr/vm/methodtablebuilder.cpp

 #endif // FEATURE_COMINTEROP
+                if (type == mcPInvoke)
+                {
+                    // For P/Invoke methods,


Can a valid P/Invoke actually return Task (marshalled as IUknown) that can awaited by the caller? Should we have a test for it?

jkotas · 2025-12-17T04:44:02Z

src/coreclr/vm/comdelegate.cpp

    if (pMD->HasClassOrMethodInstantiation())
        EX_THROW(EEResourceException, (kInvalidProgramException, W("InvalidProgram_GenericMethod")));

+    // No async methods


We have tests for the other negative cases in src\tests\Interop\UnmanagedCallersOnly\InvalidCallbacks.il . We can add the async one too for completeness.

jkotas · 2025-12-17T05:07:59Z

src/coreclr/vm/methodtablebuilder.cpp

                type = mcPInvoke;
            }
+
+            if (type == mcPInvoke && IsMiAsync(dwImplFlags))


Should we reject MiAsync on anything that is not mcIL or mcInstantiated?

For example, FCalls or delegate methods marked with MiAsync are not going to work well either. We just do not have them at the moment.

Resolve dllimport.cpp runtimeasync todos.

3112d49

jkoritzinsky requested review from AaronRobinsonMSFT, VSadov and Copilot December 11, 2025 21:16

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 11, 2025

dotnet-policy-service bot assigned jkoritzinsky Dec 11, 2025

jkoritzinsky added area-Interop-coreclr runtime-async and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 11, 2025

Copilot started reviewing on behalf of jkoritzinsky December 11, 2025 21:16 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

build-analysis bot mentioned this pull request Dec 12, 2025

"Over release of MOW - COM" assert in COM wrappers #122447

Closed

Copilot AI mentioned this pull request Dec 12, 2025

Disable UniqueMarshalling COM tests causing "Over release of MOW - COM" assertion #122473

Closed

Rewrite to block loading a p/invoke method or jitting a UCO method wi…

ab72707

…th the async bit set.

VSadov requested a review from jkotas December 16, 2025 02:01

This was referenced Dec 16, 2025

[RuntimeAsync] Revisit TODOs in places of questionable intersections of existing scenarios and Async #115094

Open

Enable runtime async config switch by default #121732

Open

jkotas reviewed Dec 17, 2025

View reviewed changes

	// We don't support generating a interop IL stub for an async method.
	// We don't support generating an interop IL stub for an async method.

Resolve dllimport.cpp runtimeasync todos #122466

Are you sure you want to change the base?

Resolve dllimport.cpp runtimeasync todos #122466

Conversation

jkoritzinsky commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

VSadov commented Dec 11, 2025

Uh oh!

VSadov commented Dec 11, 2025

Uh oh!

jkoritzinsky commented Dec 11, 2025

Uh oh!

jkoritzinsky commented Dec 15, 2025

Uh oh!

VSadov commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkoritzinsky commented Dec 11, 2025 •

edited

Loading

VSadov commented Dec 16, 2025 •

edited

Loading