Conversation
- Add UseRuntimeAsync property for source projects - Add TestRuntimeAsync property for test projects - Enable RuntimeAsync with crossgen2 - Add pipeline leg for runtimeasync tests with crossgen2
|
/azp run runtime-coreclr crossgen2 |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR adds initial support for emitting and consuming runtime-async-related methods in crossgen2 ReadyToRun (R2R) images, including new signature flags and runtime-side lookup/GC-walk plumbing for async variants and resumption stubs.
Changes:
- Enables
runtime-async=onfeature flagging for selected source and test projects, and adds a dedicated CI leg to run R2R + runtime-async library tests. - Extends R2R method signature encoding/decoding to represent async variants and resumption stubs, and updates crossgen2 tooling/readers to surface these modifiers.
- Adds runtime support for locating and registering resumption stub entrypoints so stack walking/GC can associate R2R resumption stubs with a
MethodDesc.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Directory.Build.targets | Enables preview features + runtime-async=on for source projects when UseRuntimeAsync=true. |
| eng/testing/tests.targets | Adjusts runtime-async enablement for tests and adds TestRuntimeAsync override knob. |
| eng/pipelines/coreclr/crossgen2.yml | Adds a new CI test matrix leg for R2R + runtime-async libraries testing. |
| src/coreclr/inc/corcompile.h | Adds ENCODE_METHOD_SIG_ResumptionStub flag for method signature encoding. |
| src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs | Adds managed enum flag READYTORUN_METHOD_SIG_ResumptionStub. |
| src/coreclr/vm/zapsig.cpp | Reads new ResumptionStub flag during method sig decode (currently unused in logic). |
| src/coreclr/vm/stackwalk.cpp | Relaxes a debug assert for unwind table registration to accommodate async R2R scenarios. |
| src/coreclr/vm/readytoruninfo.h | Declares runtime API to look up resumption stub entrypoints for async variants. |
| src/coreclr/vm/readytoruninfo.cpp | Implements resumption stub lookup and registers R2R-backed stub MethodDesc for GC stack walks. |
| src/coreclr/vm/method.hpp | Extends async lookup enum and dynamic IL stub types to represent R2R resumption stubs. |
| src/coreclr/vm/methodtable.cpp | Adds an AsyncResumptionStub lookup path (currently duplicative of the existing slow path). |
| src/coreclr/vm/ilstubcache.h | Declares helper to create a DynamicMethodDesc wrapper around precompiled (R2R) stub code. |
| src/coreclr/vm/ilstubcache.cpp | Implements creation of an R2R-backed IL-stub MethodDesc with native entrypoint set directly. |
| src/coreclr/vm/jitinterface.cpp | Tweaks READYTORUN_HELPER handling (includes an unexpected printf). |
| src/coreclr/inc/readytorunhelpers.h | Adds mapping for READYTORUN_HELPER_ThrowExact. |
| src/coreclr/inc/readytorun.h | Clarifies formatting/commenting for async continuation helpers. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunSignature.cs | Shows [RESUME] in method display and improves BadImageFormatException message. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunReader.cs | Tracks async/resume modifiers when parsing instance method + PGO sections. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunMethod.cs | Stores/display method modifiers (async/resume) in signature string. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/PgoInfoKey.cs | Includes modifiers in PGO key signature string generation. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Adds AsyncMethodVariant.Mangling.cs to the build. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs | Broadens IL provisioning to handle async variants and resumption stubs. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunTableManager.cs | Categorizes async variants/resumption stubs with instantiated methods for table emission. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs | Avoids inlining async call/thunk methods and force-adds required async metadata references once. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs | Minor whitespace/style fix. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs | Emits ResumptionStub flag and hashes resumption stubs with their async variant method signature. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs | Adjusts method token resolution and adds field token resolution helper. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs | Ensures async variants/resumption stubs aren’t incorrectly optimized as ordinary defs. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InstanceEntryPointTableNode.cs | Handles async variants/resumption stubs in instantiated entrypoint table emission. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs | Skips emitting inlining info for async thunks and avoids work for methods with no inlinees. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ExceptionInfoLookupTableNode.cs | Skips EH-info table processing for resumption stubs. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs | Adjusts GC map encoding size computation (currently incorrect for non-64-bit pointer sizes). |
| src/coreclr/tools/Common/TypeSystem/IL/Stubs/AsyncResumptionStub.cs | Updates diagnostic naming and hashing; marks token generation on emit. |
| src/coreclr/tools/Common/TypeSystem/IL/InstantiatedMethodIL.cs | Relaxes a Debug.Assert to accommodate non-standard owning method relationships. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Enables READYTORUN path to provide an async resumption stub and relaxes a debug assert for async variants. |
| src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Async.cs | Tracks continuation types as valid types. |
| src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs | Treats resumption stubs as async thunks for compilation decisions. |
Comments suppressed due to low confidence (1)
src/coreclr/vm/jitinterface.cpp:14153
- Avoid calling
printffrom the VM here. This will write to stdout in production and can interfere with host output; it also bypasses existing logging/diagnostics patterns already present in this block (STRESS_LOG+_ASSERTE). Please remove theprintfand rely on the existing logging/assertion mechanisms (or route through the runtime logging infrastructure if an additional message is needed).
result = (size_t)GetEEFuncEntryPoint(DelayLoad_Helper_Obj);
break;
case READYTORUN_HELPER_DelayLoad_Helper_ObjObj:
...tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs:296
- Inconsistent variable naming: the variable is named
methodil(lowercase 'il') while in other similar branches it'smethodIL(uppercase 'IL'). For consistency with lines 264, 273, and 285, this should bemethodIL.
if (_manifestModuleWrappedMethods.TryGetValue(ars, out var methodil))
src/coreclr/vm/readytoruninfo.cpp
Outdated
| // For async variant methods, we also need to look up the corresponding resumption stub | ||
| // and create a DynamicMethodDesc wrapper for it so that GC stack walks work correctly. | ||
| if (pMD->IsAsyncVariantMethod()) | ||
| { | ||
| uint stubRuntimeFunctionIndex = 0; | ||
| PCODE stubEntryPoint = LookupResumptionStubEntryPoint(pMD, pConfig, fFixups, &stubRuntimeFunctionIndex); | ||
| if (stubEntryPoint != (PCODE)NULL) | ||
| { | ||
| // Create a DynamicMethodDesc wrapper for the R2R resumption stub | ||
| AllocMemTracker amTracker; | ||
| MethodTable* pStubMT = m_pModule->GetILStubCache()->GetOrCreateStubMethodTable(m_pModule); | ||
|
|
||
| // Build the resumption stub signature: object(object, ref byte) | ||
| // This matches BuildResumptionStubSignature in jitinterface.cpp | ||
| SigBuilder sigBuilder; | ||
| sigBuilder.AppendByte(IMAGE_CEE_CS_CALLCONV_DEFAULT); | ||
| sigBuilder.AppendData(2); // 2 arguments | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // return type: object (continuation) | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // arg0: object (continuation) | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_BYREF); // arg1: ref byte (result location) | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_U1); | ||
|
|
||
| DWORD cbStubSig; | ||
| PVOID pStubSig = sigBuilder.GetSignature(&cbStubSig); | ||
|
|
||
| MethodDesc* pStubMD = ILStubCache::CreateR2RBackedILStub( | ||
| pMD->GetLoaderAllocator(), | ||
| pStubMT, | ||
| stubEntryPoint, | ||
| DynamicMethodDesc::StubAsyncResume, | ||
| (PCCOR_SIGNATURE)pStubSig, | ||
| cbStubSig, | ||
| FALSE, | ||
| &amTracker); | ||
|
|
||
| amTracker.SuppressRelease(); | ||
|
|
||
| // Register the stub's entry point so GC can find it during stack walks | ||
| m_pCompositeInfo->SetMethodDescForEntryPointInNativeImage(stubEntryPoint, pStubMD); | ||
| } | ||
| else | ||
| { | ||
| pEntryPoint = (PCODE)NULL; | ||
| goto done; | ||
| } | ||
| } |
There was a problem hiding this comment.
This PR introduces significant new functionality for emitting async methods and resumption stubs in ReadyToRun images, but no test files are included in the changes. Given the complexity of this feature (R2R code generation, signature encoding, method descriptor creation, GC stack walking), comprehensive tests should be added to verify:
- Async methods are correctly emitted to R2R images
- Resumption stubs are created and registered properly
- GC stack walks work correctly with R2R async methods
- Signature matching and lookup logic for resumption stubs functions correctly
- Edge cases like generic async methods are handled
Consider adding tests in src/tests/readytorun/ and src/tests/async/ directories.
src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/coreclr/vm/readytoruninfo.cpp:1307
- Missing break statement after finding a matching signature in the hash table lookup loop. When a matching signature is found and offset is set, the loop should exit immediately. Currently, the code continues iterating through remaining entries in the hash bucket, potentially overwriting the correct offset value with data from non-matching entries that happen to follow in the enumeration.
Add a break statement after line 1306 to exit the loop once a match is found, similar to how the PGO instrumentation lookup handles this case (which returns immediately on match at line 1188).
// Get the updated SigPointer location, so we can calculate the size of the blob,
// in order to skip the blob and find the entry point data.
offset = entryParser.GetOffset() + (uint)(sig.GetPtr() - pBlob);
break;
}
}
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:1296
- The TODO comment describes a known issue where encoding async continuation helpers as R2R helpers creates eager fixups that trigger type loading during fixup processing, causing assertion failures. The current workaround creates method entrypoints instead of using helper cells.
While the current implementation works around the issue, consider documenting this limitation more thoroughly and tracking it in a GitHub issue for future resolution. The workaround may have performance implications compared to using proper R2R helper cells.
// TODO: Encoding these as r2r helpers creates eager fixups. During fixup processing, the runtime asserts that no typeloading happens.
// However, in resolving these methods, they are loaded and their containing types are loaded, and the assertion fails.
case CorInfoHelpFunc.CORINFO_HELP_ALLOC_CONTINUATION:
{
var method = _compilation.NodeFactory.TypeSystemContext.GetCoreLibEntryPoint("System.Runtime.CompilerServices"u8, "AsyncHelpers"u8, "AllocContinuation"u8, null);
var methodWithToken = new MethodWithToken(method, _compilation.CompilationModuleGroup.Resolver.GetModuleTokenForMethod(method, true, true), null, false, null);
return _compilation.NodeFactory.MethodEntrypoint(methodWithToken, false, false, false);
}
case CorInfoHelpFunc.CORINFO_HELP_ALLOC_CONTINUATION_METHOD:
{
var method = _compilation.NodeFactory.TypeSystemContext.GetCoreLibEntryPoint("System.Runtime.CompilerServices"u8, "AsyncHelpers"u8, "AllocContinuationMethod"u8, null);
var methodWithToken = new MethodWithToken(method, _compilation.CompilationModuleGroup.Resolver.GetModuleTokenForMethod(method, true, true), null, false, null);
return _compilation.NodeFactory.MethodEntrypoint(methodWithToken, false, false, false);
}
case CorInfoHelpFunc.CORINFO_HELP_ALLOC_CONTINUATION_CLASS:
{
var method = _compilation.NodeFactory.TypeSystemContext.GetCoreLibEntryPoint("System.Runtime.CompilerServices"u8, "AsyncHelpers"u8, "AllocContinuationClass"u8, null);
var methodWithToken = new MethodWithToken(method, _compilation.CompilationModuleGroup.Resolver.GetModuleTokenForMethod(method, true, true), null, false, null);
return _compilation.NodeFactory.MethodEntrypoint(methodWithToken, false, false, false);
}
| // and create a DynamicMethodDesc wrapper for it so that GC stack walks work correctly. | ||
| if (pMD->IsAsyncVariantMethod()) | ||
| { | ||
| uint stubRuntimeFunctionIndex = 0; |
There was a problem hiding this comment.
stubRuntimeFunctionIndex is declared but never used. This will trigger unused-variable warnings (often treated as errors) in CoreCLR builds. Remove it, or use it as part of the stub lookup/registration logic if it was intended for something.
| uint stubRuntimeFunctionIndex = 0; |
| // Create a DynamicMethodDesc wrapper for the R2R resumption stub | ||
| AllocMemTracker amTracker; | ||
| MethodTable* pStubMT = m_pModule->GetILStubCache()->GetOrCreateStubMethodTable(m_pModule); | ||
|
|
||
| // Build the resumption stub signature: object(object, ref byte) | ||
| // This matches BuildResumptionStubSignature in jitinterface.cpp | ||
| SigBuilder sigBuilder; | ||
| sigBuilder.AppendByte(IMAGE_CEE_CS_CALLCONV_DEFAULT); | ||
| sigBuilder.AppendData(2); // 2 arguments | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // return type: object (continuation) | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // arg0: object (continuation) | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_BYREF); // arg1: ref byte (result location) | ||
| sigBuilder.AppendElementType(ELEMENT_TYPE_U1); | ||
|
|
||
| DWORD cbStubSig; | ||
| PVOID pStubSig = sigBuilder.GetSignature(&cbStubSig); | ||
|
|
||
| MethodDesc* pStubMD = ILStubCache::CreateR2RBackedILStub( | ||
| pMD->GetLoaderAllocator(), | ||
| pStubMT, | ||
| stubEntryPoint, | ||
| DynamicMethodDesc::StubAsyncResume, | ||
| (PCCOR_SIGNATURE)pStubSig, | ||
| cbStubSig, | ||
| FALSE, | ||
| &amTracker); | ||
|
|
||
| amTracker.SuppressRelease(); | ||
|
|
||
| // Register the stub's entry point so GC can find it during stack walks | ||
| m_pCompositeInfo->SetMethodDescForEntryPointInNativeImage(stubEntryPoint, pStubMD); |
There was a problem hiding this comment.
The new resumption-stub path allocates a new DynamicMethodDesc on every GetEntryPoint call for async variant methods, even if the resumption stub entry point was already registered in the entryPoint->MethodDesc map. This can lead to repeated loader-heap growth (or races creating duplicate stubs). Consider first checking whether stubEntryPoint is already mapped and only creating/registering a stub MethodDesc when there isn’t one yet.
| // Create a DynamicMethodDesc wrapper for the R2R resumption stub | |
| AllocMemTracker amTracker; | |
| MethodTable* pStubMT = m_pModule->GetILStubCache()->GetOrCreateStubMethodTable(m_pModule); | |
| // Build the resumption stub signature: object(object, ref byte) | |
| // This matches BuildResumptionStubSignature in jitinterface.cpp | |
| SigBuilder sigBuilder; | |
| sigBuilder.AppendByte(IMAGE_CEE_CS_CALLCONV_DEFAULT); | |
| sigBuilder.AppendData(2); // 2 arguments | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // return type: object (continuation) | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // arg0: object (continuation) | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_BYREF); // arg1: ref byte (result location) | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_U1); | |
| DWORD cbStubSig; | |
| PVOID pStubSig = sigBuilder.GetSignature(&cbStubSig); | |
| MethodDesc* pStubMD = ILStubCache::CreateR2RBackedILStub( | |
| pMD->GetLoaderAllocator(), | |
| pStubMT, | |
| stubEntryPoint, | |
| DynamicMethodDesc::StubAsyncResume, | |
| (PCCOR_SIGNATURE)pStubSig, | |
| cbStubSig, | |
| FALSE, | |
| &amTracker); | |
| amTracker.SuppressRelease(); | |
| // Register the stub's entry point so GC can find it during stack walks | |
| m_pCompositeInfo->SetMethodDescForEntryPointInNativeImage(stubEntryPoint, pStubMD); | |
| // Check whether we already have a MethodDesc registered for this resumption stub entry point. | |
| MethodDesc* pStubMD = m_pCompositeInfo->GetMethodDescForEntryPointInNativeImage(stubEntryPoint); | |
| if (pStubMD == nullptr) | |
| { | |
| // Create a DynamicMethodDesc wrapper for the R2R resumption stub | |
| AllocMemTracker amTracker; | |
| MethodTable* pStubMT = m_pModule->GetILStubCache()->GetOrCreateStubMethodTable(m_pModule); | |
| // Build the resumption stub signature: object(object, ref byte) | |
| // This matches BuildResumptionStubSignature in jitinterface.cpp | |
| SigBuilder sigBuilder; | |
| sigBuilder.AppendByte(IMAGE_CEE_CS_CALLCONV_DEFAULT); | |
| sigBuilder.AppendData(2); // 2 arguments | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // return type: object (continuation) | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_OBJECT); // arg0: object (continuation) | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_BYREF); // arg1: ref byte (result location) | |
| sigBuilder.AppendElementType(ELEMENT_TYPE_U1); | |
| DWORD cbStubSig; | |
| PVOID pStubSig = sigBuilder.GetSignature(&cbStubSig); | |
| pStubMD = ILStubCache::CreateR2RBackedILStub( | |
| pMD->GetLoaderAllocator(), | |
| pStubMT, | |
| stubEntryPoint, | |
| DynamicMethodDesc::StubAsyncResume, | |
| (PCCOR_SIGNATURE)pStubSig, | |
| cbStubSig, | |
| FALSE, | |
| &amTracker); | |
| amTracker.SuppressRelease(); | |
| // Register the stub's entry point so GC can find it during stack walks | |
| m_pCompositeInfo->SetMethodDescForEntryPointInNativeImage(stubEntryPoint, pStubMD); | |
| } |
| if (pMD->IsAsyncVariantMethod()) | ||
| goto done; | ||
|
|
||
| ETW::MethodLog::GetR2RGetEntryPointStart(pMD); |
There was a problem hiding this comment.
ETW::MethodLog::GetR2RGetEntryPointStart(pMD) is now emitted inside the new private GetEntryPoint(..., matchResumptionStub) overload. For async variant methods, the public wrapper calls the overload twice (normal + resumption stub lookup), which will fire the Start event twice but the final GetEntryPoint event once. Consider moving the Start/End ETW logging to the public wrapper and suppressing it for the stub-lookup call (or gating on matchResumptionStub).
| ETW::MethodLog::GetR2RGetEntryPointStart(pMD); | |
| if (!matchResumptionStub) | |
| ETW::MethodLog::GetR2RGetEntryPointStart(pMD); |
| _ => method, | ||
| }; | ||
| return resumptionStub.TargetMethod.GetPrimaryMethodDesc(); | ||
| } |
There was a problem hiding this comment.
GetPrimaryMethodDesc no longer unwraps PInvokeTargetNativeMethod to its underlying declared target. Several ReadyToRun components assume GetPrimaryMethodDesc().GetTypicalMethodDefinition() is an EcmaMethod (e.g., ModuleTokenResolver and multiple direct casts) and will throw or mis-handle raw pinvoke methods if this unwrap is missing. Restore the PInvokeTargetNativeMethod -> Target unwrapping (and keep it recursive) or update all downstream callers to handle PInvokeTargetNativeMethod explicitly.
| } | |
| } | |
| if (method is PInvokeTargetNativeMethod pinvokeTarget) | |
| { | |
| return pinvokeTarget.Target.GetPrimaryMethodDesc(); | |
| } |
...ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ExceptionInfoLookupTableNode.cs
Outdated
Show resolved
Hide resolved
| EcmaMethod inlinerDefinition = (EcmaMethod)inliner.GetTypicalMethodDefinition(); | ||
| if (inliner.IsAsyncThunk()) | ||
| { | ||
| // Async thunks are generated by crossgen and diagnostic tools don't need to worry about them |
There was a problem hiding this comment.
Diagnostic tools do need to worry about async thunks, e.g. debugger needs to worry about them for stepping.
This comment should be more accurate. It should explain why we are not tracking methods inlined into async thunks.
| id = ReadyToRunHelper.ReversePInvokeExit; | ||
| break; | ||
|
|
||
| // TODO: Encoding these as r2r helpers creates eager fixups. During fixup processing, the runtime asserts that no typeloading happens. |
There was a problem hiding this comment.
What is the assert that you are hitting? This should not be different from other helpers implemented in C#, for example CORINFO_HELP_MON_ENTER? They must have the same problem.
There was a problem hiding this comment.
IIRC it was a TRIGGERS_TYPELOAD() assertion after a BEGIN_FORBID_TYPELOAD()
runtime/src/coreclr/vm/clsload.cpp
Line 3308 in c667ebc
Though I just found this comment that indicates it shouldn't have been happening.
runtime/src/coreclr/vm/ceeload.cpp
Lines 3534 to 3536 in c667ebc
I'll investigate.
There was a problem hiding this comment.
It looks like this comment is referring to when eager fixups are done when loading corelib, not when the target is corelib. When AllocContinuation is an eager fixup in the entrypoint assembly, AsyncHelpers type hasn't been loaded yet, and when trying to load it, the assert fires. I'll see if we can make this a delayload fixup instead.
There was a problem hiding this comment.
It is ok to load more CoreLib types during eager fixups. I would suppress this forbid type load debug assert when handling READYTORUN_FIXUP_Helper case in LoadDynamicInfoEntry.
| else | ||
| { | ||
| // If we cannot find the resumption stub, don't return an entry point for the async variant method | ||
| // TODO: This can cause valid async variant methods without an await to fail to resolve an entry point. |
There was a problem hiding this comment.
Have you considered hooking up the async resumption stubs using a fixup?
- Add a new fixup READYTORUN_FIXUP_AsyncResumptionStub
- Fixing up this fixup is going to restore the resumption stub
- The signature of the fixup can be RVA of the async resumption stub code, so we would avoid hash table lookup to find it.
I think this would better match the model. Async method can have zero to many resumption stubs. We do not use the many resumption stubs case at the moment, but the JIT can choose to do that as an optimization in principle.
There was a problem hiding this comment.
I did consider doing it like other fixups (having the fixup write the address of the entrypoint to the resumption stub table), but I hadn't considered doing it like you suggest. I'll try doing that.
| var cont = _continuationTypeHashtable.GetOrCreateValue(pointerMap); | ||
| _validTypes.TryAdd(cont); | ||
| return cont; |
There was a problem hiding this comment.
| var cont = _continuationTypeHashtable.GetOrCreateValue(pointerMap); | |
| _validTypes.TryAdd(cont); | |
| return cont; | |
| return _continuationTypeHashtable.GetOrCreateValue(pointerMap); |
| || (method.OwningType is MetadataType mdType && mdType.HasCustomAttribute("System.Diagnostics", "StackTraceHiddenAttribute")) | ||
| || (method is Internal.IL.Stubs.ILStubMethod) | ||
| || method.IsAsyncThunk()) // see MethodDesc::IsDiagnosticsHidden() in src/coreclr/vm/method.inl | ||
| || method.IsCompilerGeneratedILBodyForAsync()) // see MethodDesc::IsDiagnosticsHidden() in src/coreclr/vm/method.inl |
There was a problem hiding this comment.
I think we should keep IsAsyncThunk around and use IsCompilerGeneratedILBodyForAsync only where it makes sense.
I think IsAsyncThunk makes more sense here and it works fine here.
| } | ||
|
|
||
| if (callee.IsAsyncThunk()) | ||
| if (callee.IsCompilerGeneratedILBodyForAsync() || callee.IsAsyncCall()) |
There was a problem hiding this comment.
IsAsyncThunk would work just fine here.
| { | ||
| // Async thunks and variants cannot currently be inlined cross module | ||
| if (method.IsAsyncVariant() || method.IsAsync || method.IsAsyncThunk()) | ||
| if (method.IsAsyncVariant() || method.IsAsync || method.IsCompilerGeneratedILBodyForAsync()) |
There was a problem hiding this comment.
IsAsyncThunk should work just fine here.
Emit async methods, and their resumption stubs into ReadyToRun images. Also compiles and emits async thunks for task-returning methods.
Code for async methods and resumption stubs are emitted in the InstanceMethod table, with an additional ENCODE_METHOD_SIG_ResumptionStub (0x100) or ENCODE_METHOD_SIG_ResumptionStub (0x200) flag, respectively. Aside from those flags, the encoded signatures should be identical such that searching for a resumption stub can be done with the async variant MethodDesc. They also have identical VersionResilient hash codes for this purpose.
Resumption stubs are not the subject of any fixups, so in order to create MethodDescs and resolve GC info, the resumption stubs are searched for when an async variant method is resolved by
GetEntryPoint(). It's not trivial to use existing fixup mechanisms for resumption stubs because the jit puts the entrypoint address in the resumption stub table (which resides in the .text section), while fixups require an additional level of indirection. We could work around this in crossgen or the jit.Resumption stub MethodDescs are created when GetEntryPoint() is called for its async variant. Code for an async variant is rejected if the async variant cannot be found. This means some valid async methods which do not have resumption stubs will not load their precompiled code, which needs to be addressed in a followup. There are still some edge cases where an required resumption stub cannot be resolved (in particularly complicated generics).
The resumption stub MethodDescs are created following the existing pattern for ILStubs, but set the code to the R2R code rather than a precode thunk.