-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[Trimming] Fix startup crash when TypeMapAssemblyTarget attribute survives but target assembly is fully trimmed #130589
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.
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
Copilot
wants to merge
7
commits into
main
Choose a base branch
from
copilot/fix-startup-crash-trimming
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c30fd11
Initial plan
Copilot dcf1d89
Fix TypeMapAssemblyTarget attribute removal when target assembly has …
Copilot 2c9bb9f
Address code review: rename param, improve comments in TypeMapHandler…
Copilot 516910c
Revert CoreCLR assemblynative.cpp changes - fix is trimmer-only
Copilot 1a5f5d0
Potential fix for pull request finding
jtschuster 27cd6c1
Fix ordering: call TriggerPendingAssemblyTargets from MarkAssembly in…
Copilot 0d63a40
Remove redundant TriggerPendingAssemblyTargets call from MarkTypeMapA…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,16 @@ sealed class TypeMapHandler | |
| // [type map group: custom attributes] | ||
| Dictionary<TypeReference, List<CustomAttributeWithOrigin>> _pendingExternalTypeMapEntries = null!; | ||
| Dictionary<TypeReference, List<CustomAttributeWithOrigin>> _pendingProxyTypeMapEntries = null!; | ||
| Dictionary<TypeReference, List<CustomAttributeWithOrigin>> _pendingAssemblyTargets = null!; | ||
|
|
||
| // [type map group: (custom attribute, resolved target assembly)] | ||
| // The resolved target assembly is null if it could not be found. In that case, the attribute will not be marked. | ||
| Dictionary<TypeReference, List<(CustomAttributeWithOrigin Attr, AssemblyDefinition? TargetAssembly)>> _pendingAssemblyTargets = null!; | ||
|
|
||
| // [target assembly: (type map group, custom attribute, calling method)] | ||
| // When a type map group is seen, assembly targets whose referenced assembly has not yet been marked are moved here. | ||
| // When the referenced assembly is eventually marked (due to a TypeMap entry being marked), all pending entries for | ||
| // it are also marked. | ||
| Dictionary<AssemblyDefinition, List<(TypeReference Group, CustomAttributeWithOrigin Attr, MethodDefinition? CallingMethod)>> _pendingAssemblyTargetsByAssembly = null!; | ||
|
|
||
| HashSet<TypeReference> _referencedExternalTypeMaps = null!; | ||
| HashSet<TypeReference> _referencedProxyTypeMaps = null!; | ||
|
|
@@ -52,6 +61,7 @@ public void Initialize(LinkContext context, MarkStep markStep, AssemblyDefinitio | |
| _pendingExternalTypeMapEntries = new(typeReferenceEqualityComparer); | ||
| _pendingProxyTypeMapEntries = new(typeReferenceEqualityComparer); | ||
| _pendingAssemblyTargets = new(typeReferenceEqualityComparer); | ||
| _pendingAssemblyTargetsByAssembly = []; | ||
| _referencedExternalTypeMaps = new(typeReferenceEqualityComparer); | ||
| _referencedProxyTypeMaps = new(typeReferenceEqualityComparer); | ||
| var typeMapResolver = new TypeMapResolver(entryPointAssembly); | ||
|
|
@@ -70,12 +80,11 @@ public void ProcessExternalTypeMapGroupSeen(MethodDefinition callingMethod, Type | |
| MarkTypeMapAttribute(entry, new DependencyInfo(DependencyKind.TypeMapEntry, callingMethod)); | ||
| } | ||
| } | ||
| if (_pendingAssemblyTargets.Remove(typeMapGroup, out List<CustomAttributeWithOrigin>? assemblyTargets)) | ||
| if (_pendingAssemblyTargets.Remove(typeMapGroup, out List<(CustomAttributeWithOrigin Attr, AssemblyDefinition? TargetAssembly)>? assemblyTargets)) | ||
| { | ||
| foreach (var entry in assemblyTargets) | ||
| foreach (var (entry, targetAssembly) in assemblyTargets) | ||
| { | ||
| var info = new DependencyInfo(DependencyKind.TypeMapAssemblyTarget, callingMethod); | ||
| MarkTypeMapAttribute(entry, info); | ||
| MarkAssemblyTargetIfReady(typeMapGroup, entry, targetAssembly, callingMethod); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -91,12 +100,11 @@ public void ProcessProxyTypeMapGroupSeen(MethodDefinition callingMethod, TypeRef | |
| MarkTypeMapAttribute(entry, new DependencyInfo(DependencyKind.TypeMapEntry, callingMethod)); | ||
| } | ||
| } | ||
| if (_pendingAssemblyTargets.Remove(typeMapGroup, out List<CustomAttributeWithOrigin>? assemblyTargets)) | ||
| if (_pendingAssemblyTargets.Remove(typeMapGroup, out List<(CustomAttributeWithOrigin Attr, AssemblyDefinition? TargetAssembly)>? assemblyTargets)) | ||
| { | ||
| foreach (var entry in assemblyTargets) | ||
| foreach (var (entry, targetAssembly) in assemblyTargets) | ||
| { | ||
| var info = new DependencyInfo(DependencyKind.TypeMapAssemblyTarget, callingMethod); | ||
| MarkTypeMapAttribute(entry, info); | ||
| MarkAssemblyTargetIfReady(typeMapGroup, entry, targetAssembly, callingMethod); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -112,6 +120,41 @@ void MarkTypeMapAttribute(CustomAttributeWithOrigin entry, DependencyInfo info) | |
| _markStep.MarkRequirementsForInstantiatedTypes(targetTypeDef); | ||
| } | ||
|
|
||
| void MarkAssemblyTargetIfReady(TypeReference typeMapGroup, CustomAttributeWithOrigin entry, AssemblyDefinition? targetAssembly, MethodDefinition? callingMethod) | ||
| { | ||
| // If the target assembly could not be resolved (it is not present in the linker input), | ||
| // the attribute cannot safely be kept: at runtime Assembly.Load would throw | ||
| // FileNotFoundException. Drop it silently; the assembly simply does not participate | ||
| // in the type map. | ||
| if (targetAssembly is null) | ||
| return; | ||
|
|
||
| if (_context.Annotations.IsMarked(targetAssembly)) | ||
| { | ||
| // Target assembly is already marked. Ideally we would only keep the attribute when the | ||
| // assembly has at least one surviving TypeMap/TypeMapAssociation entry, but checking that | ||
| // here would add complexity for a narrow case. We accept this slight over-approximation. | ||
| MarkTypeMapAttribute(entry, new DependencyInfo(DependencyKind.TypeMapAssemblyTarget, callingMethod)); | ||
| } | ||
| else | ||
| { | ||
| // Target assembly is not yet marked. Defer: mark when (if) the assembly eventually gets marked. | ||
| _pendingAssemblyTargetsByAssembly.AddToList(targetAssembly, (typeMapGroup, entry, callingMethod)); | ||
| } | ||
| } | ||
|
|
||
| // Called from MarkStep.MarkAssembly whenever an assembly is first marked (for any reason). | ||
| // This ensures that pending TypeMapAssemblyTarget attributes are flushed regardless of | ||
| // the order in which assemblies are visited. | ||
| internal void TriggerPendingAssemblyTargets(AssemblyDefinition newlyMarkedAssembly) | ||
| { | ||
| if (!_pendingAssemblyTargetsByAssembly.Remove(newlyMarkedAssembly, out List<(TypeReference Group, CustomAttributeWithOrigin Attr, MethodDefinition? CallingMethod)>? waiting)) | ||
| return; | ||
|
|
||
| foreach (var (_, attr, callingMethod) in waiting) | ||
| MarkTypeMapAttribute(attr, new DependencyInfo(DependencyKind.TypeMapAssemblyTarget, callingMethod)); | ||
| } | ||
|
|
||
| public void ProcessType(TypeDefinition definition) | ||
| { | ||
| EnsureInitialized(); | ||
|
|
@@ -192,21 +235,16 @@ static TypeReference UnwrapToResolvableType(TypeReference type) | |
| return type; | ||
| } | ||
|
|
||
| private void AddAssemblyTarget(TypeReference typeMapGroup, CustomAttributeWithOrigin attr) | ||
| private void AddAssemblyTarget(TypeReference typeMapGroup, CustomAttributeWithOrigin attr, AssemblyDefinition? resolvedTargetAssembly) | ||
| { | ||
| // Validate attribute | ||
| if (attr.Attribute.ConstructorArguments is not ([{ Value: string }])) | ||
| return; | ||
|
|
||
| // If the type map group has been seen, mark the attribute immediately | ||
| if (_referencedExternalTypeMaps.Contains(typeMapGroup) || _referencedProxyTypeMaps.Contains(typeMapGroup)) | ||
| { | ||
| _markStep.MarkCustomAttribute(attr.Attribute, new DependencyInfo(DependencyKind.TypeMapEntry, null), new MessageOrigin(attr.Origin)); | ||
| return; | ||
| } | ||
|
Comment on lines
-201
to
-206
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should keep this check, but call MarkAssemblyTargetIfReady instead of directly calling MarkCustomAttribute. |
||
|
|
||
| // Otherwise, it's pending until the type map group is seen | ||
| _pendingAssemblyTargets.AddToList(typeMapGroup, attr); | ||
| // Otherwise, it's pending until the type map group is seen. | ||
| // Note: resolvedTargetAssembly may be null if the assembly could not be resolved (e.g., it doesn't | ||
| // exist in the input). In that case the attribute will be dropped (not marked) when the group is seen. | ||
| _pendingAssemblyTargets.AddToList(typeMapGroup, (attr, resolvedTargetAssembly)); | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -297,18 +335,20 @@ public void Resolve(LinkContext context, TypeMapHandler manager) | |
| } | ||
| else if (attr.AttributeType.Name is "TypeMapAssemblyTargetAttribute`1") | ||
| { | ||
| manager.AddAssemblyTarget(typeMapGroup, (attr, assembly)); | ||
| AssemblyDefinition? resolvedTargetAssembly = null; | ||
| if (attr.ConstructorArguments[0].Value is string str) | ||
| { | ||
| var nextAssemblyName = AssemblyNameReference.Parse(str); | ||
| if (context.TryResolve(nextAssemblyName) is AssemblyDefinition nextAssembly) | ||
| { | ||
| resolvedTargetAssembly = nextAssembly; | ||
| if (seen.Add(nextAssembly)) | ||
| { | ||
| toVisit.Enqueue(nextAssembly); | ||
| } | ||
| } | ||
| } | ||
| manager.AddAssemblyTarget(typeMapGroup, (attr, assembly), resolvedTargetAssembly); | ||
|
jtschuster marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
|
|
||
17 changes: 17 additions & 0 deletions
17
...k/test/Mono.Linker.Tests.Cases/Reflection/Dependencies/TypeMapAllConditionalEntriesDep.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| // All TypeMap entries in this assembly are conditional (3-argument form). | ||
| // The trim target (AllConditionalTrimTarget) is never referenced by the test assembly, | ||
| // so ILLink drops the entry, which leaves this assembly with no surviving TypeMap entries. | ||
| // The test verifies that the TypeMapAssemblyTarget attribute pointing here is also removed. | ||
| using System.Runtime.InteropServices; | ||
| using Mono.Linker.Tests.Cases.Reflection.Dependencies; | ||
|
|
||
| [assembly: TypeMap<AllConditionalGroupType>("ConditionalEntry", typeof(AllConditionalTarget), typeof(AllConditionalTrimTarget))] | ||
|
|
||
| namespace Mono.Linker.Tests.Cases.Reflection.Dependencies | ||
| { | ||
| public class AllConditionalTarget; | ||
| public class AllConditionalTrimTarget; | ||
| } |
10 changes: 10 additions & 0 deletions
10
...ink/test/Mono.Linker.Tests.Cases/Reflection/Dependencies/TypeMapAllConditionalGroupDep.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| namespace Mono.Linker.Tests.Cases.Reflection.Dependencies | ||
| { | ||
| // Group marker type used by TypeMapAssemblyTargetRemovedWhenAllEntriesTrimmed. | ||
| // Lives in its own assembly so the test assembly's TypeMapAssemblyTarget<AllConditionalGroupType> | ||
| // generic argument does not create a compile-time reference to the conditional.dll dependency. | ||
| public class AllConditionalGroupType; | ||
| } |
37 changes: 37 additions & 0 deletions
37
...t/Mono.Linker.Tests.Cases/Reflection/TypeMapAssemblyTargetRemovedWhenAllEntriesTrimmed.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Runtime.InteropServices; | ||
| using Mono.Linker.Tests.Cases.Expectations.Assertions; | ||
| using Mono.Linker.Tests.Cases.Expectations.Metadata; | ||
| using Mono.Linker.Tests.Cases.Reflection.Dependencies; | ||
|
|
||
| // The test assembly references "conditional.dll" only by the assembly name string. | ||
| // All TypeMap entries in conditional.dll are conditional on AllConditionalTrimTarget, | ||
| // which is never marked. So conditional.dll ends up with no surviving TypeMap entries. | ||
| // The fix should cause this TypeMapAssemblyTarget attribute to be removed from the linked output. | ||
| [assembly: TypeMapAssemblyTarget<AllConditionalGroupType>("conditional")] | ||
|
|
||
| namespace Mono.Linker.Tests.Cases.Reflection | ||
| { | ||
| // Compile the group-type assembly first so both the test assembly and conditional.dll can reference it. | ||
| // Compile conditional.dll second with addAsReference:false so the test assembly has no compile-time | ||
| // dependency on it (only the string reference in TypeMapAssemblyTarget). | ||
| [SetupCompileBefore("allconditionalgroup.dll", new[] { "Dependencies/TypeMapAllConditionalGroupDep.cs" })] | ||
| [SetupCompileBefore("conditional.dll", new[] { "Dependencies/TypeMapAllConditionalEntriesDep.cs" }, | ||
| references: new[] { "allconditionalgroup.dll" }, addAsReference: false)] | ||
| [SetupLinkerAction("link", "System.Private.CoreLib")] // Needed to apply embedded XML (RemoveAttributeInstances) | ||
| [SetupLinkerArgument("--ignore-link-attributes", "false")] | ||
| [RemovedAssembly("conditional.dll")] | ||
| [RemovedAttributeInAssembly("test", typeof(TypeMapAssemblyTargetAttribute<AllConditionalGroupType>))] | ||
| [Kept] | ||
| class TypeMapAssemblyTargetRemovedWhenAllEntriesTrimmed | ||
|
Copilot marked this conversation as resolved.
|
||
| { | ||
| [Kept] | ||
| static void Main() | ||
| { | ||
| // Use the group so the trimmer processes the TypeMapAssemblyTarget attribute. | ||
| _ = TypeMapping.GetOrCreateExternalTypeMapping<AllConditionalGroupType>(); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.