[dotnet-linker] Use [DynamicDependency] attributes instead of manual marking when marking static registrar methods.#25018
Conversation
…marking when preserving smart enum methods. This makes it easier to move this code out of a custom linker step in the future. Contributes towards #17693.
…marking when preserving block code. This makes it easier to move this code out of a custom linker step in the future. Contributes towards #17693.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…marking when optimizing generated code. This makes it easier to move this code out of a custom linker step in the future. Also simplify a few things: * There's no need to compute whether the code is optimizable, because the result is never used. * Unify code to determine whether the InlineIsARM64CallingConvention optimization is to be applied. * Any code that modifies the current assembly now returns a boolean saying so (so that we know if the current assembly has to be saved). Contributes towards #17693.
…dependency-attributes-preservesmartenumconversion
…tenumconversion' into dev/rolf/use-dynamic-dependency-attributes-preserveblockcode
…kcode' into dev/rolf/use-dynamic-dependency-attributes-optimizegeneratedcode
…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.
…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 |
rolfbjarne
force-pushed
the
dev/rolf/use-dynamic-dependency-attributes-applypreserveattribute
branch
from
4036efb to
9b7d008
Compare
rolfbjarne
force-pushed
the
dev/rolf/use-dynamic-dependency-attributes-applypreserveattribute
branch
2 times, most recently
from
d8b772e to
d7fde5c
Compare
This makes it easier to move this code out of a custom linker step in the future.
Contributes towards #17693.