Add ILLink to the build #17825

ericstj · 2017-04-03T18:16:31Z

This adds ILLink (a .NET Core build of the mono linker) to the build
tools and uses it to trim non-public unreachable IL and metadata from
our assemblies.

This is enabled by default for any assembly that is part of NETCore.App.

This can be disabled by setting ILLinkTrimAssembly=false.

In some cases ILLink may trim too much, for example a runtime
dependency via reflection on private or internal API.
If we cannot update ILLink to understand this dependency via heuristic
then we can manually "root" the private or internal API.
This is done by adding an XML file next to the project with the name
ILLinkTrim.xml that follows the format documented here:
https://github.com/mono/linker/blob/master/linker/README

Replaces #17632

/cc @erozenfeld @sbomer @weshaggard

This adds two features to the binplacing logic. 1. On a BinPlaceConfiguration the ItemName metadata will control binplacing something other than the primary output of the project. This can be useful when you want a set of projects to binplace a common asset (eg a report or intermediate file) to a common location. 2. On a BinPlaceConfiguration the SetProperties metadata will control additional properties set when that configuration is active. This can be useful when you want to correlate some piece of infrastructure with a particular configuration for all projects.

This project had a hack to run TargetsTriggeredByCompilation unconditionally. That was OK when all those did was run APICompat, but now that we're rewriting the assemblies this is broken: it reruns trimming on rebuild and tries to resign the already signed assembly as well. This hack is no longer needed as of dotnet/buildtools@2c3aea5 so removing.

This adds ILLink (a .NET Core build of the mono linker) to the build tools and uses it to trim non-public unreachable IL and metadata from our assemblies. This is enabled by default for any assembly that is part of NETCore.App. This can be disabled by setting ILLinkTrimAssembly=false. In some cases ILLink may trim too much, for example a runtime dependency via reflection on private or internal API. If we cannot update ILLink to understand this dependency via heuristic then we can manually "root" the private or internal API. This is done by adding an XML file next to the project with the name ILLinkTrim.xml that follows the format documented here: https://github.com/mono/linker/blob/master/linker/README

These represent dynamic dependencies that are not caught by ILLink. See individual files for details on the dependencies.

ericstj · 2017-04-03T18:20:06Z

@dotnet-bot test outerloop netcoreapp CentOS7.1 Debug
@dotnet-bot test outerloop netcoreapp CentOS7.1 Release
@dotnet-bot test outerloop netcoreapp Debian8.4 Debug
@dotnet-bot test outerloop netcoreapp Debian8.4 Release
@dotnet-bot test outerloop netcoreapp Fedora24 Debug
@dotnet-bot test outerloop netcoreapp Fedora24 Release
@dotnet-bot test outerloop netcoreapp OpenSUSE13.2 Debug
@dotnet-bot test outerloop netcoreapp OpenSUSE13.2 Release
@dotnet-bot test outerloop netcoreapp OpenSUSE42.1 Debug
@dotnet-bot test outerloop netcoreapp OpenSUSE42.1 Release
@dotnet-bot test outerloop netcoreapp OSX10.12 Debug
@dotnet-bot test outerloop netcoreapp OSX10.12 Release
@dotnet-bot test outerloop netcoreapp PortableLinux Debug
@dotnet-bot test outerloop netcoreapp PortableLinux Release
@dotnet-bot test outerloop netcoreapp RHEL7.2 Debug
@dotnet-bot test outerloop netcoreapp RHEL7.2 Release
@dotnet-bot test outerloop netcoreapp Ubuntu14.04 Debug
@dotnet-bot test outerloop netcoreapp Ubuntu14.04 Release
@dotnet-bot test outerloop netcoreapp Ubuntu16.04 Debug
@dotnet-bot test outerloop netcoreapp Ubuntu16.04 Release
@dotnet-bot test outerloop netcoreapp Ubuntu16.10 Debug
@dotnet-bot test outerloop netcoreapp Ubuntu16.10 Release
@dotnet-bot test outerloop netcoreapp Windows 10 Debug
@dotnet-bot test outerloop netcoreapp Windows 10 Release

ericstj · 2017-04-03T23:51:15Z

@dotnet-bot test Innerloop OSX10.12 Release x64 Build and Test

ericstj · 2017-04-03T23:53:42Z

@dotnet-bot test outerloop netcoreapp CentOS7.1 Debug
@dotnet-bot test outerloop netcoreapp CentOS7.1 Release
@dotnet-bot test outerloop netcoreapp Fedora24 Debug
@dotnet-bot test outerloop netcoreapp Windows 10 Debug

ericstj · 2017-04-03T23:59:28Z

/cc @Petermarcu I just heard from @russellhadley that we're going to put this in to get feedback on it.

@weshaggard can you review please?

clairernovotny · 2017-04-04T13:02:25Z

I fully support the idea of a linker in .NET Core but please consolidate the linker directives. There's rd.xml for .NET Native and this new thing. Please pick one and use it for both toolchains. Otherwise we'll need to duplicate linker directives and that's going to lead to errors.

ericstj · 2017-04-04T16:07:21Z

@onovotny please share that feedback in https://github.com/mono/linker.

ericstj · 2017-04-04T16:10:11Z

Both outerloop failures appear to be flakiness with network tests:

17:29:26   Starting:    System.Net.NameResolution.Pal.Tests
17:29:26   /mnt/resource/j/workspace/dotnet_corefx/master/outerloop_netcoreapp_fedora24_debug_prtest/src/Native/Unix/System.Native/pal_networking.cpp (414): error 22: Unknown gethostbyname/gethostbyaddr error code. Invalid argument (false failed)
17:29:26   dotnet: /mnt/resource/j/workspace/dotnet_corefx/master/outerloop_netcoreapp_fedora24_debug_prtest/src/Native/Unix/System.Native/pal_networking.cpp:414: int ConvertGetHostErrorPlatformToPal(int): Assertion `false && "assert_err failed"' failed.

System.Net.Tests.TaskWebClientTest.UploadString_Success(echoServer: https://corefx-net.cloudapp.net/Echo.ashx) (from (empty))

MESSAGE:
System.Net.WebException : An error occurred while sending the request. Problem with the SSL CA cert (path? access rights?)\n---- System.Net.Http.HttpRequestException : An error occurred while sending the request.\n-------- System.Net.Http.CurlException : Problem with the SSL CA cert (path? access rights?)

ericstj · 2017-04-04T16:10:33Z

@dotnet-bot test OuterLoop netcoreapp CentOS7.1 Debug x64
@dotnet-bot test OuterLoop netcoreapp Fedora24 Debug x64

clairernovotny · 2017-04-04T16:48:01Z

@ericstj done dotnet/linker#57

ericstj · 2017-04-04T17:06:52Z

Some data on build perf with this change.

On my dev box, before the change a clean build took 2:43.86, after the change clean build takes 3:41.13. That's an increase of 35%. @sbomer we'll definitely want to see what you can do with a task here (in future). That will at least save on load/JIT time for ILLink. You should also see about caching reference data for dlls that are identified to be "immutable". I believe roslyn does this.

I'll also share logs for what dead code was trimmed so that folks can look at that.

ericstj · 2017-04-04T18:10:31Z

@dotnet-bot test OuterLoop netcoreapp Fedora24 Debug x64

ericstj · 2017-04-04T19:22:39Z

Fedora appears to be hitting a recurring issue in the native shim.

17:29:26   Starting:    System.Net.NameResolution.Pal.Tests
17:29:26   /mnt/resource/j/workspace/dotnet_corefx/master/outerloop_netcoreapp_fedora24_debug_prtest/src/Native/Unix/System.Native/pal_networking.cpp (414): error 22: Unknown gethostbyname/gethostbyaddr error code. Invalid argument (false failed)
17:29:26   dotnet: /mnt/resource/j/workspace/dotnet_corefx/master/outerloop_netcoreapp_fedora24_debug_prtest/src/Native/Unix/System.Native/pal_networking.cpp:414: int ConvertGetHostErrorPlatformToPal(int): Assertion `false && "assert_err failed"' failed.

I'm trying it out on an empty PR to see if its caused by this change.

87c0062 looks like it may have introduced the failure /cc @steveharter

ericstj · 2017-04-04T20:41:24Z

Yeah, same failure occurring on a clean PR. We can ignore that one.

@weshaggard can you have a look at the targets?

weshaggard · 2017-04-04T22:40:32Z

external/ILLink/project.json.template

+  "frameworks": {
+    "netcoreapp1.1": { }
+  },
+  "runtimes": {


Is this RID specific?

No but we need a RID to get assets out. That's an old side-effect of the project.json resolvenugetpackageassets task: the only way it gives you runtime assets is if you specify a RID.

weshaggard · 2017-04-04T22:43:40Z

dir.targets

@@ -60,6 +60,8 @@
      <PackageFileRefPath Condition="'$(IsNETCoreAppRef)' == 'true'">$(NETCoreAppPackageRefPath)</PackageFileRefPath>
      <PackageFileRuntimePath>$(NETCoreAppPackageRuntimePath)</PackageFileRuntimePath>
      <RuntimePath Condition="'$(BinPlaceNETCoreAppPackage)' == 'true'">$(NETCoreAppPackageRuntimePath)\..\runtime</RuntimePath>
+      <!-- enable trimming for any project that's part of the shared framework -->
+      <SetProperties Condition="'$(IsRuntimeAssembly)' == 'true'">ILLinkTrimAssembly=true</SetProperties>


IsRuntimeAssembly doesn't equate to part of the shared framework that equates to all libraries.

Is the goal to enable this by default or should this be conditioned? Even if it is enabled by default we should have an opt-out.

IsRuntimeAssembly when combined with the above condition on the item ('$(IsNETCoreApp)' == 'true') and this configuration being selected does equate to shared framework.

The opt-out of setting ILLinkTrimAssembly to false does work, but it does so in a round-about way: it doesn't run the _SetILLinkTrimAssembly, which in turn doesn't cause GetBinPlaceConfiguration to run early enough to matter. I'll fix that so its a bit more explicit.

I see, I can use BinPlaceRuntime instead.

Make it so that we don't set ILLinkTrimAssembly if its already set.

weshaggard

LGTM

We should have an issue filed to track getting the targets file out of the root of your repo and the restoring of the tool into a more general place then under external.

ericstj · 2017-04-05T17:16:27Z

Yeah, we want to eventually put this in buildtools. https://github.com/dotnet/corefx/issues/17955

EgorBo · 2017-04-12T17:31:00Z

I am wondering why when I compile corefx (in my case - System.Data.Common) I sometimes receive an error "error MSB4057: The target "GetBinPlaceConfiguration" does not exist in the project. F:\corefx\illink.targets" (if I try to compile again - it disappears).

ericstj · 2017-04-12T18:05:29Z

@EgorBo that target exists in https://github.com/dotnet/corefx/blob/16625287b450bc623fe46683f0ff27da1ab5ad47/Tools-Override/FrameworkTargeting.targets. You could run into that error if you cleaned your repo and then ran init-tools without running a build.cmd. It looks like the copy of tools-override to the tools directory only happens when going through one of the root scripts (

corefx/run.sh

Line 9 in 2059248

# Always copy over the Tools-Override

,

corefx/run.cmd

Line 42 in 8c777d8

xcopy %~dp0Tools-Override\* %~dp0Tools /y >nul

)

Add ILLink to the build Commit migrated from dotnet/corefx@1662528

ericstj added 4 commits April 3, 2017 09:52

Add manual roots for ILLink

2277db9

These represent dynamic dependencies that are not caught by ILLink. See individual files for details on the dependencies.

dnfclas added the cla-already-signed label Apr 3, 2017

ericstj mentioned this pull request Apr 3, 2017

[WIP] Add ILLink to trim dead code from corefx assemblies #17632

Closed

ericstj added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 3, 2017

karelz added the area-Infrastructure-libraries label Apr 4, 2017

karelz assigned ericstj Apr 4, 2017

tillig mentioned this pull request Apr 4, 2017

Crash on IOS with linker enabled autofac/Autofac#842

Closed

weshaggard reviewed Apr 4, 2017

View reviewed changes

Update ILLinkTrimAssembly condition

145762c

Make it so that we don't set ILLinkTrimAssembly if its already set.

weshaggard approved these changes Apr 5, 2017

View reviewed changes

ericstj merged commit 1662528 into dotnet:master Apr 5, 2017

karelz added this to the 2.0.0 milestone Apr 8, 2017

danmoseley mentioned this pull request Nov 2, 2018

[master] Skip CoreFx.Private.TestUtilities in source-build #33218

Merged

ericstj mentioned this pull request Jan 31, 2020

Clean up dead code dotnet/runtime#20888

Closed

95 tasks

ericstj mentioned this pull request Apr 24, 2020

Run illink task with at the end of build dotnet/runtime#31712

Closed

eerhardt mentioned this pull request Jan 25, 2021

Facade assemblies contain dead SR strings dotnet/runtime#45665

Closed

eerhardt mentioned this pull request Aug 30, 2021

ILLinkTrimAssembly condition applies to NetCoreAppCurrent only dotnet/runtime#58343

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#17825 from ericstj/ILLinkTrimAssembly

0a2460e

Add ILLink to the build Commit migrated from dotnet/corefx@1662528

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ILLink to the build #17825

Add ILLink to the build #17825

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

clairernovotny commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017

clairernovotny commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017 •

edited

ericstj commented Apr 4, 2017

weshaggard Apr 4, 2017

ericstj Apr 4, 2017 •

edited

weshaggard Apr 4, 2017

ericstj Apr 4, 2017 •

edited

ericstj Apr 4, 2017

weshaggard left a comment

ericstj commented Apr 5, 2017 •

edited

EgorBo commented Apr 12, 2017 •

edited

ericstj commented Apr 12, 2017

Add ILLink to the build #17825

Add ILLink to the build #17825

Conversation

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

ericstj commented Apr 3, 2017

clairernovotny commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017

clairernovotny commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017

ericstj commented Apr 4, 2017 • edited

ericstj commented Apr 4, 2017

weshaggard Apr 4, 2017

Choose a reason for hiding this comment

ericstj Apr 4, 2017 • edited

Choose a reason for hiding this comment

weshaggard Apr 4, 2017

Choose a reason for hiding this comment

ericstj Apr 4, 2017 • edited

Choose a reason for hiding this comment

ericstj Apr 4, 2017

Choose a reason for hiding this comment

weshaggard left a comment

Choose a reason for hiding this comment

ericstj commented Apr 5, 2017 • edited

EgorBo commented Apr 12, 2017 • edited

ericstj commented Apr 12, 2017

ericstj commented Apr 4, 2017 •

edited

ericstj Apr 4, 2017 •

edited

ericstj Apr 4, 2017 •

edited

ericstj commented Apr 5, 2017 •

edited

EgorBo commented Apr 12, 2017 •

edited