[dotnet-linker] Use [DynamicDependency] attributes instead of manual marking when marking static registrar methods.#25018
Conversation
…marking when marking static registrar methods. This makes it easier to move this code out of a custom linker step in the future. Contributes towards #17693.
There was a problem hiding this comment.
Pull request overview
Updates the dotnet-linker pipeline to preserve static-registrar-related delegate proxy Invoke methods by injecting [DynamicDependency] attributes, moving away from manual marking to support future removal of custom linker steps (per #17693).
Changes:
- Add
MarkForStaticRegistrarStep(anAssemblyModifierStep) that adds dynamic dependency attributes for delegate proxyInvokemethods. - Disable the existing
MarkForStaticRegistrarmark substep when the new step runs (via a flag inDerivedLinkContext). - Wire the new step into the MSBuild trimmer custom steps behind a new
_UseDynamicDependenciesForMarkStaticRegistrarproperty.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/dotnet-linker/MarkForStaticRegistrarStep.cs | New assembly-modifier step that injects dynamic dependency attributes to preserve delegate proxy Invoke methods for the static registrar. |
| tools/dotnet-linker/MarkForStaticRegistrar.cs | Skips the existing mark substep when the new pre-MarkStep custom step has run. |
| tools/common/DerivedLinkContext.cs | Adds a DidRunMarkForStaticRegistrarStep flag to coordinate between the two implementations. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Adds a new build property and enables the new step before MarkStep when dynamic dependencies are enabled. |
| var getDelegateProxyType = DerivedLinkContext.StaticRegistrar.GetDelegateProxyType (method); | ||
| if (getDelegateProxyType is null) | ||
| return false; | ||
|
|
||
| var invokeMethod = getDelegateProxyType.Methods.SingleOrDefault (m => m.Name == "Invoke"); |
There was a problem hiding this comment.
The local name getDelegateProxyType is misleading (it’s a TypeDefinition, not a getter). Rename to something like delegateProxyType to avoid confusion, especially since the next lines treat it as a type instance.
| var getDelegateProxyType = DerivedLinkContext.StaticRegistrar.GetDelegateProxyType (method); | |
| if (getDelegateProxyType is null) | |
| return false; | |
| var invokeMethod = getDelegateProxyType.Methods.SingleOrDefault (m => m.Name == "Invoke"); | |
| var delegateProxyType = DerivedLinkContext.StaticRegistrar.GetDelegateProxyType (method); | |
| if (delegateProxyType is null) | |
| return false; | |
| var invokeMethod = delegateProxyType.Methods.SingleOrDefault (m => m.Name == "Invoke"); |
| if (!StaticRegistrar.IsDelegate (method.ReturnType.Resolve ())) | ||
| return false; | ||
|
|
||
| var getDelegateProxyType = DerivedLinkContext.StaticRegistrar.GetDelegateProxyType (method); | ||
| if (getDelegateProxyType is null) | ||
| return false; | ||
|
|
There was a problem hiding this comment.
ProcessDelegateProxyAttribute resolves method.ReturnType for every method in every linked assembly. Since this step iterates all methods, this can add significant overhead and may also trigger type resolution failures for otherwise-dead code. Consider first checking whether a delegate proxy type exists (or the presence of DelegateProxyAttribute on method.MethodReturnType) and only then resolving types.
| if (!StaticRegistrar.IsDelegate (method.ReturnType.Resolve ())) | |
| return false; | |
| var getDelegateProxyType = DerivedLinkContext.StaticRegistrar.GetDelegateProxyType (method); | |
| if (getDelegateProxyType is null) | |
| return false; | |
| var getDelegateProxyType = DerivedLinkContext.StaticRegistrar.GetDelegateProxyType (method); | |
| if (getDelegateProxyType is null) | |
| return false; | |
| var returnType = method.ReturnType.Resolve (); | |
| if (!StaticRegistrar.IsDelegate (returnType)) | |
| return false; |
| @@ -553,6 +553,7 @@ | |||
| <_UseDynamicDependenciesForGeneratedCodeOptimizations Condition="'$(_UseDynamicDependenciesForGeneratedCodeOptimizations)' == ''">$(_UseDynamicDependenciesInsteadOfMarking)</_UseDynamicDependenciesForGeneratedCodeOptimizations> | |||
| <_UseDynamicDependenciesForApplyPreserveAttribute Condition="'$(_UseDynamicDependenciesForApplyPreserveAttribute)' == '' And '$(_XamarinRuntime)' == 'NativeAOT'">true</_UseDynamicDependenciesForApplyPreserveAttribute> | |||
| <_UseDynamicDependenciesForApplyPreserveAttribute Condition="'$(_UseDynamicDependenciesForApplyPreserveAttribute)' == ''">$(_UseDynamicDependenciesInsteadOfMarking)</_UseDynamicDependenciesForApplyPreserveAttribute> | |||
There was a problem hiding this comment.
The new property _UseDynamicDependenciesForMarkStaticRegistrar is a bit inconsistent with the step/type name (MarkForStaticRegistrar*) and the other properties (...ForApplyPreserveAttribute, etc.). Consider aligning the property name (or adding a short comment) so it’s easier to grep and understand later.
| <_UseDynamicDependenciesForApplyPreserveAttribute Condition="'$(_UseDynamicDependenciesForApplyPreserveAttribute)' == ''">$(_UseDynamicDependenciesInsteadOfMarking)</_UseDynamicDependenciesForApplyPreserveAttribute> | |
| <_UseDynamicDependenciesForApplyPreserveAttribute Condition="'$(_UseDynamicDependenciesForApplyPreserveAttribute)' == ''">$(_UseDynamicDependenciesInsteadOfMarking)</_UseDynamicDependenciesForApplyPreserveAttribute> | |
| <!-- Controls whether Xamarin.Linker.Steps.MarkForStaticRegistrarStep uses DynamicDependency attributes instead of direct marking. --> |
✅ [CI Build #653b289] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #653b289] 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 |
✅ [CI Build #653b289] Build passed (Build macOS tests) ✅Pipeline on Agent |
🔥 [CI Build #653b289] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 28 tests failed, 128 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 (tvOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ linker tests5 tests failed, 39 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS)2 tests failed, 9 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (MacCatalyst)3 tests failed, 12 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (macOS)3 tests failed, 9 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (tvOS)11 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
This makes it easier to move this code out of a custom linker step in the future.
Contributes towards #17693.