Fix CrossGen2 TypeMap blob-only type references - #131610
Open
jkoritzinsky wants to merge 5 commits into
Open
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. |
jkoritzinsky
added
area-ReadyToRun
and removed
area-crossgen2-coreclr
Contributor
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. |
Comment on lines
49
to
53
| internal Vertex EncodeReferenceToType(NativeWriter writer, TypeDesc type, ModuleDesc module = null) | ||
| { | ||
| Import typeImport = GetImportToType(type); | ||
| 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)); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
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));
Comment on lines
+140
to
151
| // Some producers encode type map custom attributes without emitting matching TypeRef rows | ||
| // for the referenced types. In that case, fall back to the target type's defining assembly. | ||
| if (module is EcmaModule ecmaModule && type.Module is EcmaModule targetTypeModule) | ||
| { | ||
| string targetAssemblyName = targetTypeModule.Assembly.GetName().Name; | ||
|
|
||
| // The assembly name in a custom attribute type argument is valid even when there is no | ||
| // corresponding AssemblyRef row in the source module metadata. | ||
| return targetAssemblyName; | ||
|
|
||
| throw new KeyNotFoundException($"Unable to resolve an assembly reference from module '{module}' to type '{type}'."); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.