Skip to content

Resolve dllimport.cpp runtimeasync todos #122466

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.

Sign up for GitHub

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

Open
jkoritzinsky wants to merge 2 commits into
base: main
Choose a base branch
from
+36 −18
Open

Resolve dllimport.cpp runtimeasync todos #122466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3112d49
Resolve dllimport.cpp runtimeasync todos.
jkoritzinsky Dec 11, 2025
ab72707
Rewrite to block loading a p/invoke method or jitting a UCO method wi…
jkoritzinsky Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/mscorrc.rc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ BEGIN
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."
Copy link
Member

@jkotas jkotas Dec 17, 2025

Choose a reason for hiding this comment

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

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

IDS_EE_CLASS_CONSTRAINTS_VIOLATION "GenericArguments[%1], '%2', on '%3' violates the constraint of type parameter '%4'."
IDS_EE_METHOD_CONSTRAINTS_VIOLATION "Method %1.%2: type argument '%3' violates the constraint of type parameter '%4'."
IDS_EE_NOSYNCHRONIZED "Synchronized attribute cannot be used with this method type."
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,3 +524,4 @@
#define IDS_EE_NDIRECT_DISABLEDMARSHAL_LCID 0x264E
#define IDS_EE_NDIRECT_DISABLEDMARSHAL_PRESERVESIG 0x264F
#define IDS_EE_NDIRECT_DISABLEDMARSHAL_VARARGS 0x2650
#define BFA_ASYNC_PINVOKE_METHOD 0x2651
4 changes: 4 additions & 0 deletions src/coreclr/vm/comdelegate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2045,6 +2045,10 @@ void COMDelegate::ThrowIfInvalidUnmanagedCallersOnlyUsage(MethodDesc* pMD)
if (pMD->HasClassOrMethodInstantiation())
EX_THROW(EEResourceException, (kInvalidProgramException, W("InvalidProgram_GenericMethod")));

// No async methods
Copy link
Member

@jkotas jkotas Dec 17, 2025

Choose a reason for hiding this comment

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

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

if (pMD->IsAsyncMethod())
EX_THROW(EEResourceException, (kInvalidProgramException, W("InvalidProgram_AsyncMethod")));

// Arguments - Scenarios involving UnmanagedCallersOnly are handled during the jit.
bool unmanagedCallersOnlyRequiresMarshalling = false;
if (PInvoke::MarshalingRequired(pMD, NULL, NULL, NULL, unmanagedCallersOnlyRequiresMarshalling))
Expand Down
33 changes: 15 additions & 18 deletions src/coreclr/vm/dllimport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ StubSigDesc::StubSigDesc(MethodDesc *pMD)
GC_NOTRIGGER;
SUPPORTS_DAC;
PRECONDITION(pMD != NULL);
PRECONDITION(!pMD->IsAsyncMethod());
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
CONTRACTL_END;

Expand All @@ -108,9 +109,6 @@ StubSigDesc::StubSigDesc(MethodDesc *pMD)
m_sig = pMD->GetSignature();
m_pModule = pMD->GetModule(); // Used for token resolution.

// TODO: (async) revisit and examine if this needs to be supported somehow
_ASSERTE(!pMD->IsAsyncMethod());

m_tkMethodDef = pMD->GetMemberDef();
SigTypeContext::InitTypeContext(pMD, &m_typeContext);
m_pMetadataModule = pMD->GetModule();
Expand Down Expand Up @@ -1131,7 +1129,6 @@ class ILStubState : public StubState
DWORD dwToken = 0;
if (pTargetMD)
{
// TODO: (async) revisit and examine if this needs to be supported somehow
_ASSERTE(!pTargetMD->IsAsyncVariantMethod());
dwToken = pTargetMD->GetMemberDef();
}
Expand Down Expand Up @@ -2764,6 +2761,10 @@ void PInvokeStaticSigInfo::DllImportInit(

PRECONDITION(CheckPointer(pMD));

// P/Invoke methods should never be marked as async.
// This should be blocked in the class loader.
PRECONDITION(!pMD->IsAsyncMethod());

// These preconditions to prevent multithreaded regression
// where pMD->m_szLibName was passed in directly, cleared
// by this API, then accessed on another thread before being reset here.
Expand All @@ -2779,9 +2780,6 @@ void PInvokeStaticSigInfo::DllImportInit(
IMDInternalImport *pInternalImport = pMD->GetMDImport();
CorPinvokeMap mappingFlags = pmMaxValue;
mdModuleRef modref = mdModuleRefNil;
// TODO: (async) revisit and examine if this needs to be supported somehow
if (pMD->IsAsyncMethod())
ThrowHR(COR_E_NOTSUPPORTED);

if (FAILED(pInternalImport->GetPinvokeMap(pMD->GetMemberDef(), (DWORD*)&mappingFlags, ppEntryPointName, &modref)))
{
Expand Down Expand Up @@ -3053,16 +3051,15 @@ namespace
STANDARD_VM_CHECK;
PRECONDITION(pMD != NULL);
PRECONDITION(pMD->IsPInvoke());
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);

Comment on lines +3054 to +3062
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

HRESULT hr = pInternalImport->GetPinvokeMap(pMD->GetMemberDef(), (DWORD*)&mappingFlags, NULL /*pszImportName*/, NULL /*pmrImportDLL*/);
if (FAILED(hr))
Expand Down Expand Up @@ -3249,6 +3246,12 @@ BOOL PInvoke::MarshalingRequired(
{
STANDARD_VM_CHECK;
PRECONDITION(pMD != NULL || (!sigPointer.IsNull() && pModule != NULL));

// We should never see an async method here.
// Delegate Invoke methods should never be async.
// Async P/Invokes are not supported.
// Async UnmanagedCallersOnly methods are not supported.
PRECONDITION(pMD == NULL || !pMD->IsAsyncMethod());
}
CONTRACTL_END;

Expand Down Expand Up @@ -3324,10 +3327,6 @@ BOOL PInvoke::MarshalingRequired(
mdMethodDef methodToken = mdMethodDefNil;
if (pMD != NULL)
{
// TODO: (async) revisit and examine if this needs to be supported somehow
if (pMD->IsAsyncMethod())
ThrowHR(COR_E_NOTSUPPORTED);

methodToken = pMD->GetMemberDef();
}
CollateParamTokens(pMDImport, methodToken, numArgs - 1, pParamTokenArray);
Expand Down Expand Up @@ -6080,12 +6079,10 @@ static void GetILStubForCalli(VASigCookie* pVASigCookie, MethodDesc* pMD)

if (pMD != NULL)
{
_ASSERTE(pMD->IsPInvoke());
_ASSERTE(!pMD->IsAsyncMethod());
PInvokeStaticSigInfo sigInfo(pMD);

// TODO: (async) revisit and examine if this needs to be supported somehow
if (pMD->IsAsyncMethod())
ThrowHR(COR_E_NOTSUPPORTED);

md = pMD->GetMemberDef();
nlFlags = sigInfo.GetLinkFlags();
nlType = sigInfo.GetCharSet();
Expand Down
12 changes: 12 additions & 0 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3183,6 +3183,11 @@ MethodTableBuilder::EnumerateClassMethods()
CONSISTENCY_CHECK(hr == S_OK);
type = mcPInvoke;
}

if (type == mcPInvoke && IsMiAsync(dwImplFlags))
Copy link
Member

@jkotas jkotas Dec 17, 2025

Choose a reason for hiding this comment

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

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.

{
BuildMethodTableThrowException(BFA_ASYNC_PINVOKE_METHOD);
}
}
else if (IsMiRuntime(dwImplFlags))
{
Expand Down Expand Up @@ -3441,6 +3446,13 @@ MethodTableBuilder::EnumerateClassMethods()
asyncVariantType = mcIL;
}
#endif // FEATURE_COMINTEROP
if (type == mcPInvoke)
{
// For P/Invoke methods,
Copy link
Member

@jkotas jkotas Dec 17, 2025

Choose a reason for hiding this comment

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

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

// we don't want to treat the async variant as a P/Invoke method
// (as it isn't, it's a transient IL method).
asyncVariantType = mcIL;
}

Signature newMemberSig(pNewMemberSignature, cAsyncThunkMemberSignature);
pNewMethod = new (GetStackingAllocator()) bmtMDMethod(
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3107,6 +3107,9 @@
<data name="NotSupported_NonReflectedType" xml:space="preserve">
<value>Not supported in a non-reflected type.</value>
</data>
<data name="InvalidProgram_AsyncMethod" xml:space="preserve">
<value>Async methods with UnmanagedCallersOnlyAttribute are invalid.</value>
</data>
<data name="InvalidProgram_NonStaticMethod" xml:space="preserve">
<value>Non-static methods with UnmanagedCallersOnlyAttribute are invalid.</value>
</data>
Expand Down