Update to new linker custom steps API #5748

sbomer · 2021-03-16T01:02:01Z

This updates the custom steps to use the new API which runs logic on mark (dotnet/linker#1774). As part of this, I've removed the workaround that loads all referenced assemblies up-front and simplified some of the linker msbuild targets. Some of the MonoDroid.Tuner steps were duplicated and changed to implement the new IMarkHandler interface, to avoid touching the MonoDroid.Tuner code.

@radekdoulik PTAL - in particular I would appreciate your opinion on whether the below assumptions are reasonable. Please also add anyone else who you think should review the change.

Notes from dotnet/linker#1774 (comment):
In my analysis of the custom steps, most of them "just work" if we call them only for marked members, because they ultimately either:

Just call AddPreserved* to conditionally keep members of types (which with the new API will just happen when the type is marked)
In the case of FixAbstractMethodsStep, inject missing interface implementations which will only be kept if they are marked for some other reason.

Most of the steps have been updated in a straightforward way based on the above.

The exceptions are:

ApplyPreserveAttribute: this step globally marks members with PreserveAttribute.
- The step is only active for non-SDK link assemblies. Usually we root non-SDK assemblies, but it will be a problem if linking all assemblies.
- For now, I updated it to use the new custom step API on assembly mark - so it will scan for the attribute in all marked assemblies - but we should investigate whether this is good enough.
PreserveExportedTypes: similar to the above, this globally marks members with Export*Attribute. It's used for java interop in cases where the java methods aren't referenced statically - like from xml, or for special serialization methods.
- It's only active for non-SDK assemblies, so like above it will be a problem only if linking all assemblies.
- Like above, I've made it scan marked assemblies.
PreserveApplications: globally scans for ApplicationAttribute on types/assemblies, and sets the TypePreserve for any types referenced by the attribute.
- This step technically does a global scan, but it's likely to work on marktype/markassembly - since presumably, ApplicationAttribute is only used on types/assembies that are already kept.
- I've updated it to use the new customstep API based on this assumption.

edit: per conversation with @jonathanpeppers the plan is to move all of the steps (including the exceptions I mentioned above) to use the new custom steps API, and get rid of the workaround that loads references up-front, even if there is a small chance that they will miss PreserveAttribute and similar in assemblies which are unused (never marked by the linker). (done in this PR)

sbomer · 2021-03-16T17:51:01Z

/azp run

azure-pipelines · 2021-03-16T17:51:07Z

Commenter does not have sufficient privileges for PR 5748 in repo xamarin/xamarin-android

sbomer · 2021-03-16T17:51:46Z

@jonathanpeppers could you help me trigger a ci run for this change?

jonathanpeppers · 2021-03-16T18:26:05Z

/azp run

azure-pipelines · 2021-03-16T18:26:46Z

Azure Pipelines successfully started running 1 pipeline(s).

jonathanpeppers · 2021-03-16T19:33:26Z

Looks like dotnet-install.sh failed: curl: (35) LibreSSL SSL_connect: SSL_ERROR_SYSCALL in connection to dot.net:443

Will just try again.

jonathanpeppers · 2021-03-16T19:33:44Z

/azp run

azure-pipelines · 2021-03-16T19:34:23Z

Azure Pipelines successfully started running 1 pipeline(s).

src/Microsoft.Android.Sdk.ILLink/FixAbstractMethodsHandler.cs

jonathanpeppers · 2021-03-24T14:15:21Z

/azp run

azure-pipelines · 2021-03-24T14:15:46Z

Azure Pipelines successfully started running 1 pipeline(s).

sbomer · 2021-03-25T18:39:07Z

Getting failures like:

/home/AzDevOps/.dotnet/sdk/5.0.103/NuGet.targets(131,5): error : Unable to load the service index for source https://devdiv.pkgs.visualstudio.com/_packaging/xamarin-xvs/nuget/v3/index.json. [/agent/_work/1/s/xamarin-android/external/monodroid/external/android-sdk-installer/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj] [/agent/_work/1/s/xamarin-android/external/monodroid/monodroid.proj]
/home/AzDevOps/.dotnet/sdk/5.0.103/NuGet.targets(131,5): error : Response status code does not indicate success: 403 (Forbidden - User '6d3b3c1a-123d-454c-a78b-b4a426164711' lacks permission to complete this action. You need to have 'ReadPackages'. (DevOps Activity ID: 892BABAE-AEC5-40AC-8C60-E5509582972E)). [/agent/_work/1/s/xamarin-android/external/monodroid/external/android-sdk-installer/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj] [/agent/_work/1/s/xamarin-android/external/monodroid/monodroid.proj]
/home/AzDevOps/.dotnet/sdk/5.0.103/NuGet.targets(131,5): error : Unable to load the service index for source https://devdiv.pkgs.visualstudio.com/_packaging/xamarin-xvs/nuget/v3/index.json. [/agent/_work/1/s/xamarin-android/external/monodroid/external/android-sdk-installer/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj] [/agent/_work/1/s/xamarin-android/external/monodroid/monodroid.proj]
/home/AzDevOps/.dotnet/sdk/5.0.103/NuGet.targets(131,5): error : Response status code does not indicate success: 403 (Forbidden - User '6d3b3c1a-123d-454c-a78b-b4a426164711' lacks permission to complete this action. You need to have 'ReadPackages'. (DevOps Activity ID: 892BABAE-AEC5-40AC-8C60-E5509582972E)). [/agent/_work/1/s/xamarin-android/external/monodroid/external/android-sdk-installer/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj] [/agent/_work/1/s/xamarin-android/external/monodroid/monodroid.proj]
/agent/_work/1/s/xamarin-android/external/monodroid/monodroid.proj(66,5): error MSB3073: The command "dotnet build -p:Configuration=Release -p:DebuggingToolsOutputDirectory=/agent/_work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android -v:n -p:OutputPath=/agent/_work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android -p:PublicIniParser=true -bl:"/agent/_work/1/s/xamarin-android/external/monodroid/bin/BuildRelease/sdkinstaller.binlog" "/agent/_work/1/s/xamarin-android/external/monodroid/external/android-sdk-installer\Xamarin.Installer.Build.Tasks\Xamarin.Installer.Build.Tasks.csproj"" exited with code 1.
    0 Warning(s)
    5 Error(s)

@jonathanpeppers is there a way I could get permissions to trigger the pipelines myself?

jonathanpeppers

I manually tested the changes here with the projects in dotnet/net6-mobile-samples.

@sbomer I think the couple CI failures here we can ignore. The test phases all look good.

@dellis1972 or @jonpryor care to review?

sbomer · 2021-03-25T19:32:20Z

Thanks @jonathanpeppers!

src/Microsoft.Android.Sdk.ILLink/SetupStep.cs

src/Microsoft.Android.Sdk.ILLink/SubStepDispatcher.cs

jonathanpeppers · 2021-04-27T18:52:04Z

/azp run

azure-pipelines · 2021-04-27T18:52:27Z

Azure Pipelines successfully started running 1 pipeline(s).

sbomer · 2021-04-27T18:58:56Z

Just FYI the latest changes depend on #5870 so I expect to see test failures.

src/Microsoft.Android.Sdk.ILLink/LinkContextExtensions.cs

src/Microsoft.Android.Sdk.ILLink/PreserveSubStepDispatcher.cs

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs

marek-safar · 2021-04-28T07:12:17Z

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs

+		{
+			AssemblyAction action = Annotations.HasAction (assembly) ? Annotations.GetAction (assembly) : AssemblyAction.Skip;
+			if (action == AssemblyAction.Skip || action == AssemblyAction.Copy || action == AssemblyAction.Delete)
+				Annotations.SetAction (assembly, AssemblyAction.Save);


Do we really want to do this? I don't think we should change copy action to save not alone skip action to save.

I don't have full context on how we can get into this situation - it looks to me like it was intended to inject methods into non-SDK assemblies which may be copy. I could get rid of skip/delete, but I'm trying to minimize functional changes here.

It'd be fine to fix this in follow-up PR but I think this needs to be addressed because we changed the meaning of "copy" action in net6. It also does not make much sense to be to update code in deleted assembly.

I got rid of the skip/delete cases because those don't make any sense to me. If anyone can provide more context on the copy case I'd be happy to address it in a follow-up.

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs

src/Microsoft.Android.Sdk.ILLink/LinkContextExtensions.cs

...marin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets

sbomer · 2021-05-25T21:26:36Z

/azp run

azure-pipelines · 2021-05-25T21:27:01Z

Azure Pipelines successfully started running 1 pipeline(s).

jonpryor · 2021-05-27T23:09:52Z

Current state of unit test affairs:

MSBuild With Emulator - macOS - Legacy > Run check-boot-times: failure is somewhat expected, should be fixed by 85d6938. Ignorable.
APKs Legacy - macOS > ** run VSAndroidApp**: failure is somewhat expected; should be fixed by 85d6938. Ignorable.
MSBuildDeviceIntegration On Device - macOS - Legacy > JsonDeserializationCreatesJavaHandle(False): failed due to an XA0010. Ignorable.
MSBuildDeviceIntegration On Device - macOS - One .NET > LocalizedAssemblies_ShouldBeFastDeployed: failed due to an XA0010. Ignorable.
MSBuildDeviceIntegration On Device - macOS - One .NET > JsonDeserializationCreatesJavaHandle(False): should be fixed by 85d6938. Ignorable.
MSBuildDeviceIntegration On Device - macOS - One .NET > RunWithInterpreterEnabled(True): should be fixed by 85d6938. Ignorable.

I've restarted MSBuild Legacy - Windows-2 and MSBuild One .NET - Windows-3 suites, as thy previously because they couldn't download test infrastructure.

src/Microsoft.Android.Sdk.ILLink/BaseMarkHandler.cs

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs

- Use "IMetadataResolver cache" fields

jonpryor · 2021-05-28T18:44:57Z

Proposed commit message:

Context: https://github.com/mono/linker/issues/1953
Context: https://github.com/mono/linker/pull/1774
Context: https://github.com/mono/linker/pull/1774#issuecomment-783563526
Context: https://github.com/mono/linker/pull/1979
Context: https://github.com/mono/linker/pull/2019
Context: https://github.com/mono/linker/pull/2045
Context: https://github.com/xamarin/java.interop/commit/2573dc8c84fd4eb68e75bcae73912c26f4942356
Context: https://github.com/xamarin/xamarin-android/pull/5870
Context: https://github.com/xamarin/xamarin-android/pull/5878

Update the custom linker steps to use the new `IMarkHandler` API
which runs logic during "MarkStep".

(See [this list][0] of pipeline steps for additional context.)

As part of this, I've removed the workaround that loads all referenced
assemblies up-front and simplified some of the linker MSBuild targets.
Some of the `MonoDroid.Tuner` steps were duplicated and changed to
implement the new `IMarkHandler` interface, to avoid touching the
`MonoDroid.Tuner` code.

In my analysis of the custom steps, most of them "just work" if we
call them only for marked members, because they ultimately either:

  - Just call `AddPreserved*()` to conditionally keep members of types
    (which with the new API will just happen when the type is marked)

  - In the case of `FixAbstractMethodsStep()`, inject missing interface
    implementations which will only be kept if they are marked for some
    other reason.

Most of the steps have been updated in a straightforward way based on
the above.

The exceptions are:

  - `AddKeepAlivesStep` needs to run on all code that survived
    linking, and can run as a normal step.

  - `ApplyPreserveAttribute`: this step globally marks members with
    `PreserveAttribute`.
    - The step is only active for non-SDK link assemblies.  Usually we
      root non-SDK assemblies, but it will be a problem if linking all
      assemblies.
    - For now, I updated it to use the new custom step API on assembly
      mark -- so it will scan for the attribute in all marked
      assemblies -- but we should investigate whether this is good
      enough.
    - This can't be migrated to use `IMarkHandler` because it needs
      to scan code for attributes, including unmarked code.

  - `PreserveExportedTypes`: similar to the above, this globally marks
    members with `Export*Attribute`.  It's used for java interop in
    cases where the java methods aren't referenced statically, like
    from xml, or for special serialization methods.
    - It's only active for non-SDK assemblies, so like above it will
      be a problem only if linking all assemblies.
    - Like above, I've made it scan marked assemblies.

  - `PreserveApplications`: globally scans for `ApplicationAttribute`
    on types/assemblies, and sets the `TypePreserve` annotation for
    any types referenced by the attribute.
    - This step technically does a global scan, but it's likely to work
      on "mark type"/"mark assembly", as `ApplicationAttribute` is only
      used on types/assembies that are already kept.
    - I've updated it to use the new `IMarkHandler` API.

Additionally, as per xamarin/java.interop@2573dc8c, stop using
`TypeDefinitionCache` everywhere and instead use `IMetadataResolver`
to support caching `TypeDefinition`s across shared steps.
For .NET 6, `LinkContextMetadataResolver` implements the
`IMetadataResolver` interface in terms of `LinkContext`.

TODO: Replace `LinkContextMetadataResolver` with `IMetadataResolver`
implemented directly on `LinkContext` in mono/linker#2045.

TODO: Get rid of all reflection in `SetupStep` with the fix for shared
state in mono/linker#2019.

[0]: https://github.com/mono/linker/blob/main/src/linker/Linker/Driver.cs#L714-L730

sbomer · 2021-05-28T19:32:27Z

@jonpryor thank you! I would remove the TODO - that work was done in this PR (I updated the original PR description to reflect this).

Could you also add:

Context: https://github.com/mono/linker/issues/1953
Context: https://github.com/xamarin/java.interop/pull/842
Context: https://github.com/mono/linker/pull/1979

This includes an update to remove the use of a `TypeDefinitionCache` shared
across steps, to allow passing individual custom steps as separate command-line
arguments. This cache is replaced by the use of a cache built in to the linker,
adapted to match the `IMetadataResolver` interface which is implemented on
`LinkContextMetadataResolver`.

TODO: Replace `LinkContextMetadataResolver` with `IMetadataResolver implemented
directly on `LinkContext` in https://github.com/mono/linker/pull/2045
TODO: Get rid of all reflection in SetupStep with the fix for shared state
in https://github.com/mono/linker/pull/2019

(Feel free to adjust wording or formatting)

sbomer · 2021-05-28T19:46:00Z

Actually, looks like the merge from main brought in the update from dotnet/linker#2045 which is a breaking change so I'll need to address that here.

LinkContext now directly implements IMetadataResolver.

jonpryor · 2021-05-28T19:56:32Z

Indeed, commit da536fc bumped our .NET 6 build, which includes a mono/linker bump, which includes dotnet/linker#2045…. Explains why my changes before the merge also failed to build, while this PR was otherwise green yesterday…

sbomer · 2021-06-02T01:04:38Z

@jonpryor I addressed the remaining TODOs in the latest changes (they can be removed from the proposed commit message).

@jonpryor

This will allow extension methods that take an `IMetadataResolver` like in dotnet/java-interop#842 to use the LinkContext Resolve cache. Context: dotnet/android#5748 (comment) The Resolve cache added in dotnet/linker#1979 requires calling `Resolve*Definition` methods directly on `LinkContext`, which means that any extension methods that do resolution logic need to take a `LinkContext`. This doesn't work well with the layering in xamarin-android, where java.interop uses a resolution cache with cecil, but doesn't depend on the linker. Instead it uses a custom `TypeResolutionCache` for extension methods like `GetBaseDefinition`: https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16 These extension methods are also used from xamarin-android, but there's a desire to use the `LinkContext` cache in this case. @jonpryor had the idea to change the extension methods to use cecil's `IMetadataResolver`, which can be implemented by `TypeDefinitionCache` and by `LinkContext`. Java.interop will continue using their `TypeDefinitionCache`, and xamarin-android will use `LinkContext`. One limitation of this approach is that `LinkContext.TryResolve*Definition` (renamed to just `TryResolve` for consistency) methods aren't usable from the extension methods. Commit migrated from dotnet/linker@aaf4880

sbomer marked this pull request as ready for review March 22, 2021 23:29

sbomer requested review from dellis1972, jonathanpeppers, jonpryor and radekdoulik as code owners March 22, 2021 23:29

sbomer force-pushed the illinkMarkHandler2 branch from 83e9029 to 336b7ce Compare March 22, 2021 23:37

jonathanpeppers reviewed Mar 23, 2021

View reviewed changes

src/Microsoft.Android.Sdk.ILLink/FixAbstractMethodsHandler.cs Outdated Show resolved Hide resolved

jonathanpeppers approved these changes Mar 25, 2021

View reviewed changes

marek-safar reviewed Apr 3, 2021

View reviewed changes

src/Microsoft.Android.Sdk.ILLink/SetupStep.cs Outdated Show resolved Hide resolved

marek-safar reviewed Apr 3, 2021

View reviewed changes

src/Microsoft.Android.Sdk.ILLink/SubStepDispatcher.cs Outdated Show resolved Hide resolved

sbomer force-pushed the illinkMarkHandler2 branch from 76b2718 to dac30f3 Compare April 27, 2021 18:03

marek-safar reviewed Apr 28, 2021

View reviewed changes

sbomer mentioned this pull request Apr 28, 2021

Update to new linker custom steps API xamarin/xamarin-macios#11374

Merged

jonathanpeppers reviewed Apr 28, 2021

View reviewed changes

src/Microsoft.Android.Sdk.ILLink/LinkContextExtensions.cs Outdated Show resolved Hide resolved

marek-safar reviewed Apr 29, 2021

View reviewed changes

...marin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets Outdated Show resolved Hide resolved

Implement IMetadataResolver for LinkContextMetadataResolver

1cc2d5a

jonpryor reviewed May 27, 2021

View reviewed changes

src/Microsoft.Android.Sdk.ILLink/BaseMarkHandler.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 27, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/AddKeepAlivesStep.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 27, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs Show resolved Hide resolved

jonpryor reviewed May 27, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs Show resolved Hide resolved

sbomer and others added 7 commits May 28, 2021 01:47

PR feedback

67c9593

- Use "IMetadataResolver cache" fields

Fix indentation

01b56ec

Fix indentation

7c70a52

Fix indentation

6c6d4c8

Fix indentation

d83a793

Add comment for why we're setting _TrimmerDumpDependencies

b13b50c

Fix indentation

8b4c908

sbomer mentioned this pull request May 28, 2021

Missing cases in base/override method comparisons #5974

Open

Merge remote-tracking branch 'origin/main' into illinkMarkHandler2

7501a0c

Remove LinkContextMetadataResolver

bda4b4f

LinkContext now directly implements IMetadataResolver.

sbomer added 2 commits June 2, 2021 01:01

Remove reflection workaround

ff9ea51

Merge remote-tracking branch 'origin/main' into illinkMarkHandler2

742507a

jonpryor merged commit 45a9adc into dotnet:main Jun 2, 2021

jonathanpeppers mentioned this pull request Jun 14, 2021

Replace reflection usage when preparing linker pipeline #4709

Closed

sbomer mentioned this pull request Sep 16, 2022

Rethink how SubStepsDispatcher is used to mark types and assemblies dotnet/linker#1735

Closed

github-actions bot locked and limited conversation to collaborators Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to new linker custom steps API #5748

Update to new linker custom steps API #5748

sbomer commented Mar 16, 2021 •

edited

Loading

sbomer commented Mar 16, 2021

azure-pipelines bot commented Mar 16, 2021

sbomer commented Mar 16, 2021

jonathanpeppers commented Mar 16, 2021

azure-pipelines bot commented Mar 16, 2021

jonathanpeppers commented Mar 16, 2021

jonathanpeppers commented Mar 16, 2021

azure-pipelines bot commented Mar 16, 2021

jonathanpeppers commented Mar 24, 2021

azure-pipelines bot commented Mar 24, 2021

sbomer commented Mar 25, 2021

jonathanpeppers left a comment

sbomer commented Mar 25, 2021

jonathanpeppers commented Apr 27, 2021

azure-pipelines bot commented Apr 27, 2021

sbomer commented Apr 27, 2021

marek-safar Apr 28, 2021

sbomer Apr 28, 2021

marek-safar Apr 29, 2021

sbomer Apr 29, 2021

sbomer commented May 25, 2021

azure-pipelines bot commented May 25, 2021

jonpryor commented May 27, 2021

jonpryor commented May 28, 2021 •

edited

Loading

sbomer commented May 28, 2021

sbomer commented May 28, 2021

jonpryor commented May 28, 2021

sbomer commented Jun 2, 2021 •

edited

Loading

Update to new linker custom steps API #5748

Update to new linker custom steps API #5748

Conversation

sbomer commented Mar 16, 2021 • edited Loading

sbomer commented Mar 16, 2021

azure-pipelines bot commented Mar 16, 2021

sbomer commented Mar 16, 2021

jonathanpeppers commented Mar 16, 2021

azure-pipelines bot commented Mar 16, 2021

jonathanpeppers commented Mar 16, 2021

jonathanpeppers commented Mar 16, 2021

azure-pipelines bot commented Mar 16, 2021

jonathanpeppers commented Mar 24, 2021

azure-pipelines bot commented Mar 24, 2021

sbomer commented Mar 25, 2021

jonathanpeppers left a comment

Choose a reason for hiding this comment

sbomer commented Mar 25, 2021

jonathanpeppers commented Apr 27, 2021

azure-pipelines bot commented Apr 27, 2021

sbomer commented Apr 27, 2021

marek-safar Apr 28, 2021

Choose a reason for hiding this comment

sbomer Apr 28, 2021

Choose a reason for hiding this comment

marek-safar Apr 29, 2021

Choose a reason for hiding this comment

sbomer Apr 29, 2021

Choose a reason for hiding this comment

sbomer commented May 25, 2021

azure-pipelines bot commented May 25, 2021

jonpryor commented May 27, 2021

jonpryor commented May 28, 2021 • edited Loading

sbomer commented May 28, 2021

sbomer commented May 28, 2021

jonpryor commented May 28, 2021

sbomer commented Jun 2, 2021 • edited Loading

sbomer commented Mar 16, 2021 •

edited

Loading

jonpryor commented May 28, 2021 •

edited

Loading

sbomer commented Jun 2, 2021 •

edited

Loading