[dotnet-linker] Use [DynamicDependency] attributes instead of manual marking when applying the [Preserve] attribute.#25001
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors how [Preserve] is handled in the dotnet linker pipeline by generating [DynamicDependency] attributes (so preservation intent is expressed in IL rather than via manual marking), aligning with the longer-term goal of reducing/removing custom linker steps (Issue #17693).
Changes:
- Replaces the older
ApplyPreserveAttributeimplementation with a shared implementation (ApplyPreserveAttributeImpl) and introduces a newApplyPreserveAttributeStepthat emits[DynamicDependency]attributes before marking. - Refactors
AssemblyModifierStepto support assembly-level modification hooks (ModifyAssembly) and ensures assemblies are only saved when modified. - Wires the new step into the build via MSBuild properties/step registration, and adds a link-context flag to prevent running both the old marking-based logic and the new dynamic-dependency logic.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/linker/ApplyPreserveAttribute.cs | Removes the legacy linker-step implementation. |
| tools/dotnet-linker/dotnet-linker.csproj | Drops compilation/linking of the removed legacy file. |
| tools/dotnet-linker/Steps/AssemblyModifierStep.cs | Introduces ModifyAssembly and seals TryProcessAssembly to standardize assembly-save behavior. |
| tools/dotnet-linker/ApplyPreserveAttributeStep.cs | New step that translates [Preserve] into [DynamicDependency] attributes pre-mark. |
| tools/dotnet-linker/ApplyPreserveAttributeBase.cs | Refactors into ApplyPreserveAttribute substep + shared ApplyPreserveAttributeImpl used by both approaches. |
| tools/dotnet-linker/AppBundleRewriter.cs | Exposes helpers used by the new step (GetOrCreateStaticConstructor, AddAttributeOnlyOnce). |
| tools/common/DerivedLinkContext.cs | Adds a flag to coordinate whether the new step ran. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Adds MSBuild property + registers ApplyPreserveAttributeStep before MarkStep. |
| MethodDefinition GetOrCreateModuleConstructor (ModuleDefinition @module) | ||
| { | ||
| var moduleType = @module.GetModuleType (); | ||
| return abr.GetOrCreateStaticConstructor (moduleType, out var modified); |
There was a problem hiding this comment.
GetOrCreateModuleConstructor calls abr.GetOrCreateStaticConstructor (moduleType, out var modified) but never uses modified. With TreatWarningsAsErrors=true, this unused local is likely to cause a build break. Consider using out _ (or otherwise consuming/propagating the flag) to avoid the unused-variable warning.
| return abr.GetOrCreateStaticConstructor (moduleType, out var modified); | |
| return abr.GetOrCreateStaticConstructor (moduleType, out _); |
| for (int i = attributes.Count - 1; i >= 0; i--) { | ||
| var attribute = attributes [i]; | ||
|
|
||
| bool remote_attribute; | ||
| if (!IsPreservedAttribute (provider, attribute, out remote_attribute)) | ||
| if (!IsPreservedAttribute (provider, attribute, out var remove_attribute)) | ||
| continue; | ||
|
|
||
| attrs.Add (attribute); | ||
| if (remote_attribute) | ||
| if (remove_attribute) | ||
| attributes.RemoveAt (i); | ||
| } |
There was a problem hiding this comment.
GetPreserveAttributes removes [Preserve] attributes from the provider (attributes.RemoveAt (i)), but this mutation isn't reflected in the modified value returned from Process/BrowseTypes/ProcessAssemblyAttributes. This means the dynamic-dependency step may decide the assembly wasn't modified (and skip SaveCurrentAssembly) even though attributes were removed (e.g., if the equivalent DynamicDependency attribute was already present so no new attributes get added). Consider tracking whether any attributes were removed and OR-ing that into the returned modified value.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…marking when applying the [Preserve] attribute. This makes it easier to move this code out of a custom linker step in the future. This was simplified a bit because we already had code to do this, but it was only in effect when using NativeAOT. Generalize it, move it out of the marking phase of the linker, and run it before marking. Contributes towards #17693.
rolfbjarne
force-pushed
the
dev/rolf/use-dynamic-dependency-attributes-applypreserveattribute
branch
from
d09d3b4 to
81457b4
Compare
✅ [CI Build #81457b4] Build passed (Build packages) ✅Pipeline on Agent |
🔥 [CI Build #81457b4] Test results 🔥Test results❌ Tests failed on VSTS: test results 5 tests crashed, 93 tests failed, 38 tests passed. Failures❌ dotnettests tests (iOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (macOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (Multiple platforms)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (tvOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ framework tests1 tests failed, 1 tests passed.Failed tests
Html Report (VSDrops) Download ❌ fsharp tests3 tests failed, 1 tests passed.Failed tests
Html Report (VSDrops) Download ❌ interdependent-binding-projects tests3 tests failed, 1 tests passed.Failed tests
Html Report (VSDrops) Download ❌ linker tests28 tests failed, 16 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS)11 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (MacCatalyst)15 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (macOS)12 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (tvOS)11 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ windows tests1 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ xcframework tests3 tests failed, 1 tests passed.Failed tests
Html Report (VSDrops) Download ❌ Tests on macOS Monterey (12) tests🔥 Failed catastrophically on VSTS: test results - mac_monterey (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Ventura (13) tests🔥 Failed catastrophically on VSTS: test results - mac_ventura (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Sonoma (14) tests🔥 Failed catastrophically on VSTS: test results - mac_sonoma (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Sequoia (15) tests🔥 Failed catastrophically on VSTS: test results - mac_sequoia (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Tahoe (26) tests🔥 Failed catastrophically on VSTS: test results - mac_tahoe (no summary found). Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS testsLinux Build VerificationPipeline on Agent |
✅ [PR Build #4036efb] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This makes it easier to move this code out of a custom linker step in the future.
This was simplified a bit because we already had code to do this, but it was only
in effect when using NativeAOT. Generalize it, move it out of the marking phase of
the linker, and run it before marking.
Contributes towards #17693.