New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ILLink to the build #17825

Merged
merged 5 commits into from Apr 5, 2017

Conversation

Projects
None yet
8 participants
@ericstj
Member

ericstj commented Apr 3, 2017

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

ericstj added some commits Apr 3, 2017

Make BinPlacing support custom source & set props
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.
Fix incremental build of S.R.CS.Unsafe
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.
Add ILLink to the build
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
Add manual roots for ILLink
These represent dynamic dependencies that are not caught by ILLink.

See individual files for details on the dependencies.
@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 3, 2017

Member

@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

Member

ericstj commented Apr 3, 2017

@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 ericstj added the * NO MERGE * label Apr 3, 2017

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 3, 2017

Member

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

Member

ericstj commented Apr 3, 2017

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

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 3, 2017

Member

@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

Member

ericstj commented Apr 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 3, 2017

Member

/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?

Member

ericstj commented Apr 3, 2017

/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?

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Apr 4, 2017

Member

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.

Member

onovotny commented Apr 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

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

Member

ericstj commented Apr 4, 2017

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

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

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?)
Member

ericstj commented Apr 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

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

Member

ericstj commented Apr 4, 2017

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

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny
Member

onovotny commented Apr 4, 2017

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

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.

Member

ericstj commented Apr 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

@dotnet-bot test OuterLoop netcoreapp Fedora24 Debug x64

Member

ericstj commented Apr 4, 2017

@dotnet-bot test OuterLoop netcoreapp Fedora24 Debug x64

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

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

Member

ericstj commented Apr 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 4, 2017

Member

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

@weshaggard can you have a look at the targets?

Member

ericstj commented Apr 4, 2017

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

@weshaggard can you have a look at the targets?

@ericstj ericstj referenced this pull request Apr 4, 2017

Open

Clean up dead code #17905

67 of 95 tasks complete
"frameworks": {
"netcoreapp1.1": { }
},
"runtimes": {

This comment has been minimized.

@weshaggard

weshaggard Apr 4, 2017

Member

Is this RID specific?

@weshaggard

weshaggard Apr 4, 2017

Member

Is this RID specific?

This comment has been minimized.

@ericstj

ericstj Apr 4, 2017

Member

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.

@ericstj

ericstj Apr 4, 2017

Member

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.

Show outdated Hide outdated dir.targets Outdated
Update ILLinkTrimAssembly condition
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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 5, 2017

Member

Yeah, we want to eventually put this in buildtools. #17955

Member

ericstj commented Apr 5, 2017

Yeah, we want to eventually put this in buildtools. #17955

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

19 checks passed

CROSS Check Build finished.
Details
Innerloop CentOS7.1 Debug x64 Build and Test Build finished.
Details
Innerloop CentOS7.1 Release x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Debug x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Release x64 Build and Test Build finished.
Details
Innerloop PortableLinux Debug x64 Build and Test Build finished.
Details
Innerloop PortableLinux Release x64 Build and Test Build finished.
Details
Innerloop Tizen armel Debug Cross Build Build finished.
Details
Innerloop Tizen armel Release Cross Build Build finished.
Details
Innerloop Ubuntu14.04 Debug x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 arm Release Cross Build Build finished.
Details
Innerloop Ubuntu16.04 arm Debug Cross Build Build finished.
Details
Innerloop Windows_NT Debug x86 Build and Test Build finished.
Details
Innerloop Windows_NT Release x64 Build and Test Build finished.
Details
Vertical netfx Build Build finished.
Details
Vertical uap Build Build finished.
Details
Vertical uapaot Build Build finished.
Details
Windows_NT Debug AllConfigurations Build Build finished.
Details

@karelz karelz modified the milestone: 2.0.0 Apr 8, 2017

@EgorBo

This comment has been minimized.

Show comment
Hide comment
@EgorBo

EgorBo Apr 12, 2017

Contributor

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).

Contributor

EgorBo commented Apr 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 12, 2017

Member

@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 (

# Always copy over the Tools-Override
,

corefx/run.cmd

Line 42 in 8c777d8

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

Member

ericstj commented Apr 12, 2017

@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 (

# Always copy over the Tools-Override
,

corefx/run.cmd

Line 42 in 8c777d8

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

@TerribleDev TerribleDev referenced this pull request Apr 20, 2017

Open

ILMerge #11672

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Jun 20, 2018

Member

@ericstj, why do we limit this to only be for assemblies in the shared framework? I was surprised today to find we don't run this on System.Drawing.Common, for example.

Member

stephentoub commented on dir.targets in 145762c Jun 20, 2018

@ericstj, why do we limit this to only be for assemblies in the shared framework? I was surprised today to find we don't run this on System.Drawing.Common, for example.

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Jun 20, 2018

Member

It was a scoping decision back when we first added it. Folks weren't willing to test the output of things that weren't going in the shared framework (eg: UAP assemblies, packages that would be used by desktop, etc) so we constrained it to only the shared framework. Folks were confident that we had better test coverage of the shared framework since that's what we were shipping at the time. We could broaden that scope but we'd probably want to make sure such a change had decent test coverage + auditing of the new assemblies it touched across all their supported frameworks.

Member

ericstj replied Jun 20, 2018

It was a scoping decision back when we first added it. Folks weren't willing to test the output of things that weren't going in the shared framework (eg: UAP assemblies, packages that would be used by desktop, etc) so we constrained it to only the shared framework. Folks were confident that we had better test coverage of the shared framework since that's what we were shipping at the time. We could broaden that scope but we'd probably want to make sure such a change had decent test coverage + auditing of the new assemblies it touched across all their supported frameworks.

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Jun 20, 2018

Member

Hmm. Ok, thanks for the background info.

Member

stephentoub replied Jun 20, 2018

Hmm. Ok, thanks for the background info.

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Jun 20, 2018

Member

We should have enough test coverage of the Windows compat pack assemblies. What would the change be if I wanted to include those?

Member

danmosemsft replied Jun 20, 2018

We should have enough test coverage of the Windows compat pack assemblies. What would the change be if I wanted to include those?

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Jun 20, 2018

Member
Member

ericstj replied Jun 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment