Fix CrossGen2 TypeMap blob-only type references - #131610
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes ReadyToRun/CrossGen2 compilation of TypeMap payloads when mapped System.Type values appear only in custom-attribute blobs (no TypeRef rows), by ensuring required tokens can still be generated and by adding a regression test that exercises this metadata shape.
Changes:
- Add a new TypeMap regression input assembly authored in IL that encodes mapped types only in custom-attribute blobs.
- Update TypeMap emission/import paths to pass the “triggering module” context so CrossGen2 can create/resolve the needed tokens for blob-only type references.
- Add a fallback in
MutableModulewhen anAssemblyRefname can’t be inferred viaTypeRef-table scanning (blob-only scenarios).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/Interop/TypeMap/TypeMapBlobOnlyLib.ilproj | New IL test project producing the blob-only typemap assembly. |
| src/tests/Interop/TypeMap/TypeMapBlobOnlyLib.il | IL that encodes TypeMap types in CA blobs without TypeRef rows for mapped types. |
| src/tests/Interop/TypeMap/TypeMapApp.csproj | Wires the new IL project into the TypeMap test app build. |
| src/tests/Interop/TypeMap/TypeMapApp.cs | Adds an xUnit test validating external/proxy maps for the blob-only assembly. |
| src/tests/Interop/TypeMap/GroupTypes.cs | Introduces the new group marker type used by the test. |
| src/coreclr/tools/Common/Compiler/INativeFormatTypeReferenceProvider.cs | Extends type-reference encoding to accept an optional module context. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.cs | Adds blob-only fallback for resolving an assembly name when no TypeRef mapping exists. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/TypeMapAssemblyTargetsNode.cs | Passes module context when importing/encoding group type references. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunProxyTypeMapNode.cs | Passes triggering module context for typemap key/value type imports and encoding. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunExternalTypeMapNode.cs | Passes triggering module context for typemap value type imports and encoding. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs | Initializes token manager earlier and provides it to the import reference provider. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ImportReferenceProvider.cs | Ensures def tokens are available when a module context is provided. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs | Updates the external references provider implementation signature to match the interface change. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ImportReferenceProvider.cs:53
EncodeReferenceToTypetakes an optionalmodule = null(and the interface also allows callers to omit it), but the method assertsmodule is not null. This is inconsistent and would cause Debug builds to fail if a future call site uses the optional parameter. Either makemodulerequired everywhere or allownullhere (and just skip token pre-creation when it’s null, asGetImportToTypealready does).
internal Vertex EncodeReferenceToType(NativeWriter writer, TypeDesc type, ModuleDesc module = null)
{
Debug.Assert(module is not null);
Import typeImport = GetImportToType(type, module);
return writer.GetTuple(writer.GetUnsignedConstant((uint)typeImport.Table.IndexFromBeginningOfArray), writer.GetUnsignedConstant((uint)typeImport.IndexFromBeginningOfArray));
jtschuster
left a comment
There was a problem hiding this comment.
Looks good to me assuming you confirmed the TypeMapBlobOnlyLib.il does put the ref in the custom attribute blob.
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ImportReferenceProvider.cs:53
INativeFormatTypeReferenceProvider.EncodeReferenceToTypeis now called withmodule == nullby non-R2R providers (e.g.,ExternalReferencesTableIndex), so assertingmodule is not nullhere makes the contract inconsistent and risks debug-only failures if this provider is ever used in a context that doesn't have/need a requiring-module.GetImportToTypealready supportsmoduleRequiringImport == null; consider allowing that here as well.
internal Vertex EncodeReferenceToType(NativeWriter writer, TypeDesc type, ModuleDesc module)
{
Debug.Assert(module is not null);
Import typeImport = GetImportToType(type, module);
return writer.GetTuple(writer.GetUnsignedConstant((uint)typeImport.Table.IndexFromBeginningOfArray), writer.GetUnsignedConstant((uint)typeImport.IndexFromBeginningOfArray));
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ImportReferenceProvider.cs:26
GetImportToTypenow dereferences_externalReferenceTokenManagerwhenmoduleRequiringImportis non-null, but there is no assertion/guard thatInitialize(...)has been called. IfGetImportToTypeis ever used before initialization (or in a code path where initialization is skipped), this will throw aNullReferenceExceptionand be hard to diagnose.
if (moduleRequiringImport is not null)
{
_externalReferenceTokenManager.EnsureDefTokensAreAvailable(type, moduleRequiringImport, referencesAreForAsyncMethod: false);
}
|
/ba-g libraries test failure unrelated |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/tests/Interop/TypeMap/TypeMapApp.cs:330
- The proxy type map assertions appear to use the wrong key direction for the Lib5 entry, and they also use
typeof(...)lookups even though this file already documents that proxy-map lookups should usenew ...().GetType()to satisfy trimming rules.TypeMapping.GetOrCreateProxyTypeMapping<T>maps source type → proxy type (see existingTypeMapEntryAssemblyApp.csusage), so the Lib5 assertion should index byLib5Type1and expectLib5Proxy1.
IReadOnlyDictionary<Type, Type> proxyMap = TypeMapping.GetOrCreateProxyTypeMapping<BlobOnlyAttributeTypeNames>();
Assert.Equal(typeof(S1), proxyMap[typeof(C1)]);
Assert.Equal(typeof(C1), proxyMap[typeof(S1)]);
Assert.Equal(typeof(Lib5Type1), proxyMap[typeof(Lib5Proxy1)]);
jtschuster
left a comment
There was a problem hiding this comment.
Do we want to backport this to rc1?
|
I thought this got merged in already. Yeah we need to backport this. |
|
/ba-g build monitor infra failure |
|
/backport to release/11.0-rc1 |
|
Started backporting to |
…132537) Backport of #131610 to release/11.0-rc1 /cc @jkoritzinsky ## Customer Impact - [ ] Customer reported - [X] Found internally Crossgen2 crashes during build for .NET-for-iOS scenarios without workaround. The iOS team currently has a workaround in place in their code. ## Regression - [ ] Yes - [X] No ## Testing Comprehensive unit tests were added for the scenario. ## Risk Low, extensive testing for the fix and the affected API is very self-contained. **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. --------- Co-authored-by: Jeremy Koritzinsky <jekoritz@microsoft.com>
…nable to encode a type in crossgen2's precomputed TypeMap (#132984) PR #131610 enabled Crossgen2 to precompute TypeMap entries for types referenced only by type name strings in custom-attribute blobs. These references in attributes do not inherently require corresponding `TypeRef` or `AssemblyRef` metadata rows. Roslyn generally emits full assembly identities for external types and may also introduce metadata references, but other valid producers may emit only the serialized name. Crossgen2 successfully resolves that string to a `TypeDesc`. It therefore knows the target module and full assembly identity. The problem occurs when translating that result into a ReadyToRun type fixup. The mutable manifest currently represents an out-of-bubble reference using: ``` #<assembly-simple-name>:<source-module-index> ``` At runtime this means: 1. Resolve the source module by index. 2. Find an `AssemblyRef` with that simple name in the source module. 3. Bind that `AssemblyRef`. 4. Resolve the synthetic `TypeRef`. PR #131610 supplied the target assembly’s simple name even but step 2 was impossible because the source module had no corresponding `AssemblyRef`. The resulting fixup is well-formed, but cannot be resolved. See #132811 for additional info. To workaround the issue, we emit the (TypeMapGroup, ProxyOrExternalMap) entry with a `0` state, which tells the runtime to build the map from attributes instead of using the precomputed map. This was done previously for maps that are expected to throw a type system exception. This allows some parts of the type map to still be encoded if a single entry is unencodable. The fix going into main is #133038, which is more involved, modifies the R2R format, and bumps the R2R version. Since there are workarounds for the mobile team, we can do this less risky change for the backport. ## Customer Impact Found internally. Crossgenned apps with a valid precomputed type map can throw at runtime. ## Regression Yes. The original TypeMap implementation would create the map only at runtime. The TypeMap precomputation PR introduced issue #131527. The fix for that introduced issue #132811, the one this fixes. ## Testing The existing test suite caught the issue but wasn't run in the PRs because crossgen tests aren't a part the default 'runtime' pipeline. The crossgen2 pipelines catch this issue and will be green if the issue is fixed. ## Risk Low. The fix doesn't introduce new features or a change to the TypeMap section format. It utilizes existing functionality to force runtime computation of the map. There is a risk that we haven't caught all possible edge cases.
Add support for resolving types referenced only by custom attribute arguments (no TypeRef rows, no AssemblyRef rows for the type's containing assembly).
Fixes #131527
Note
This PR description was generated with assistance from AI/Copilot.