[ILLink/ILCompiler] Fix crash when array types are used as TypeMap trim targets#126380
[ILLink/ILCompiler] Fix crash when array types are used as TypeMap trim targets#126380jkoritzinsky merged 16 commits intomainfrom
Conversation
Strip parameterized types (array, pointer, etc.) from TypeMap trim targets and source types, using the element type instead. This fixes the crash when typeof(SomeType[]) is used as a trim target in TypeMap attributes during PublishTrimmed. Add test cases for array type trim targets (used and unused) to TypeMap.cs. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/82e75422-6094-47d8-be6f-7a4527457908 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes an ILLink crash in PublishTrimmed when TypeMap attributes specify non-resolvable TypeSpecification types (notably arrays) as trim targets by stripping wrappers down to a resolvable element type before calling LinkContext.Resolve.
Changes:
- Normalize
TypeMaptrim targets and association source types by unwrappingTypeSpecification(array/pointer/byref/…) to an element type until reaching aTypeDefinitionorGenericInstanceType. - Add new linker test coverage for an array-type trim target scenario (kept case), and introduce a second unused-case scenario that should assert removal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/tools/illink/src/linker/Linker/TypeMapHandler.cs |
Unwraps TypeSpecification wrappers before resolving trim targets / proxy source types to avoid NotSupportedException crashes. |
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs |
Adds test inputs for array-type trim targets and ensures at least the reachable-element case is preserved. |
- Add RemovedTypeInAssembly("test", ...) for unused array trim target types
to make the expected removal behavior explicit
- Fix misleading comment in AddProxyTypeMapEntry: TypeMapAssociationAttribute
has two constructor type arguments (source and proxy), not one
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/8d0dbb63-91fb-4d87-8ccd-60e91da2e1e9
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Done in af0ac93:
Both |
- TrimTarget1-4 struct declarations now use trailing spaces: TrimTarget1 ; - Array constructors now match: new TrimTarget1 [1], new TrimTarget2 [1,1], new TrimTarget3 *[1] - Closing bracket of return array moved to match: ]; (with leading whitespace) Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/7cdf5296-b91d-4997-90ed-3fe071042c48 Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
TypeMapLazyDictionary.ContainsKey throws NotSupportedException. Use TryGetValue (which is properly supported) to check that entries D and E are absent from the map. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b5e7b52a-b2ba-4366-bb40-7b1961844e70 Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
|
This fixes the correctness issue, but doesn't address #126177 (comment) the optimization on native AOT we could do if the element is a valuetype. I've set up the testing to ensure we have coverage of the areas off the top of my head where there are dragons (I expect the tests to fail on the first try at optimizing this). Do we need the optimization too? If so, we need to add more tests that target the optimization and implement it. Cc @Sergio0694 |
|
I mean, if we have to root all this stuff for reference types and take the size regression, it would surely be very welcome to get that optimization to at least try to minimize this for all projected structs/enum types, if possible, yes 😅 |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Native AOT part LGTM. We should do the possible optimization separately.
|
test
|
PublishTrimmedcrashes withNotSupportedException: TypeDefinition cannot be resolved from 'Mono.Cecil.ArrayType'when aTypeMapattribute specifies an array type as its trim target (e.g.,typeof(Foo[])).Root cause (ILLink):
LinkContext.Resolvethrows for anyTypeSpecificationthat isn't aGenericInstanceType. Array types, pointer types, and byref types all fall into this category.Root cause (ILCompiler):
ExternalTypeMapNode.GetConditionalStaticDependenciesusedcontext.NecessaryTypeSymbol(trimmingTargetType)as the condition node. When the trim target isFoo[], the TypeMap entry should be present wheneverFoo(the element type) is reachable — butNecessaryTypeSymbol(Foo[])is a different node fromNecessaryTypeSymbol(Foo)and is only transitively marked whenFoo[]itself is used. If code only usesFoo(e.g.,new Foo()), the condition never fires and the entry is silently dropped.Description
ILLink (
TypeMapHandler.cs):TypeMapHandler.AddExternalTypeMapEntry: Strip non-resolvableTypeSpecificationwrappers (array, pointer, byref, etc.) from the trim target before passing toRecordTypeMapEntry, using the element type instead.TypeMapHandler.AddProxyTypeMapEntry: Same fix applied symmetrically to the source type parameter. Corrected the comment to accurately state thatTypeMapAssociationAttributehas two constructor type arguments (source and proxy).TypeMapHandler.MarkTypeMapAttribute: SameTypeSpecification-stripping fix applied before calling_context.Resolve(targetType).TypeMapHandler.UnwrapToResolvableType: The three previously duplicate stripping loops have been extracted into a singleprivate static TypeReference UnwrapToResolvableType(TypeReference)helper, so future fixes only need to be made in one place. All call sites use idiomatic C# property pattern matching (TypeSpecification { ElementType: var elementType }) to avoid explicit casts.ILCompiler (
ExternalTypeMapNode.cs):ExternalTypeMapNode.GetEffectiveTrimTargetType: New private static helper that stripsParameterizedTypewrappers (arrays, pointers, byrefs) to reach the element type.InstantiatedType(generics likeList<T>) does not extendParameterizedTypeand is correctly left alone.ExternalTypeMapNode.GetConditionalStaticDependenciesandGetMarkedEntries: Updated to strip the trim target type viaGetEffectiveTrimTargetTypeand useNecessaryTypeSymbolon the resulting element type. This matches ILLink's stripping behavior: the TypeMap entry is included whenever the element type is reachable (vianew Foo(),is Foocasts, array constructionnew Foo[n], etc.), regardless of whetherFoo[]itself is explicitly used.Both fixes handle nested arrays (
Foo[][]→Foo), pointer types (Foo*[]→Foo), multi-dimensional arrays (Foo[,]→Foo), and mixed cases likeList<Foo>[](strips toList<Foo>, which is not aParameterizedType).Customer Impact
PublishTrimmedbuilds fail with an unhandledNotSupportedException/ fatal ILLink error when any assembly attribute uses an array type as aTypeMaptrim target. In ILCompiler (PublishAot), the TypeMap entry for an array trim target whose element type is reachable is silently dropped. No workaround short of removing the array type.Regression
No — this is a new feature (
TypeMap) that never handled array trim targets correctly in either ILLink or ILCompiler.Testing
Added test cases to
TypeMap.cs(ILLink / ILCompiler trimming tests):TrimTargetIsUsedArrayType: array trim target whose element type is reachable (vianew ArrayTypeTrimTargetClass()) — attribute must be preserved; verified withKeptAttributeAttribute.TrimTargetIsUnusedArrayType: array trim target whose element type is unreachable — attribute and target types are removed; verified with explicitRemovedTypeInAssembly("test", ...)assertions on bothArrayTypeTrimTargetUnusedClassandArrayTypeTrimTargetUnusedTarget.Added
TestInteropMapArrayTrimmingtosrc/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs(NativeAOT smoke test):TypeMapentries (A–G) with array trim targets coveringT[],T[,], andT*[]variants.MakeGenerics<T>()is a generic method that constructsTrimTarget1 [1],TrimTarget2 [1,1],TrimTarget3 *[1], andTrimTarget6[1], invoked viatypeof(TestInteropMapArrayTrimming).GetMethod(nameof(MakeGenerics)).MakeGenericMethod([GetAtom()]).Invoke(null, []). Entry G is rooted via anis TrimTarget7[]cast.Atomclass andGetAtom()method are present to provide the generic type argument for the reflection-based call.TrimTarget1throughTrimTarget4use trailing spaces before the semicolon (e.g.,TrimTarget1 ;) to visually distinguish the array-targeted trim types from the non-array ones.TryGetValue; verifies entries D, E are absent (element type unreachable) also usingTryGetValue—ContainsKeyis not used becauseTypeMapLazyDictionarythrowsNotSupportedExceptionfor that method.Both existing
Reflection.TypeMapandReflection.TypeMapEntryAssemblytests pass.Risk
Low. The ILLink change is localized to
TypeMapHandler— three call sites and one new private static helper. The ILCompiler change is localized toExternalTypeMapNode— two methods updated and one new private static helper. The NativeAOT smoke test addition is additive only. No behavior change for existing valid (non-array) trim targets.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.