Linker into runtime2 #78077

tlakollo · 2022-11-09T00:23:43Z

Updating the consolidation branch into a new branch called LinkerIntoRuntime2 inside the runtime repo
The difference with #77569 is that this PR contains the latest linker changes and leaves outside changes that are not functional and are difficult to review (formatting the repo, moving docs, formatting the headers). Also, this PR takes into account the latest feedback about renaming linker to illink.
For more information about these changes please refer to #75278.

…0227.1 (dotnet/linker#2664) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@396c37d

* Fix behavior of intrinsics with null or empty inputs Add tests for intrinsics receiving null or empty values, and tweak a few intrinsics to avoid warnings for these cases. This fixes some unnecessary warnings, and also fixes a crash in the linker. Some of the new shared intrinsics that don't produce type values need to have some tracked value. Presumably this should really be Unknown, but currently they fall back on the shared logic to track a value with annotations (which for these intrinsics will be None). A few unnecessary warnings are left as-is to avoid changing the linker behavior. Commit migrated from dotnet/linker@6aa9837

…r#2666) Commit migrated from dotnet/linker@a8f9532

Shares two more intrinsics, with supporting infra. Fixed a bug in the intrinsics - passing null to the name of a property/field will throw at runtime, so no need to validate anything. Modifies the existing tests to add warnings, since that is the only verifyable behavior for the analyzer. Commit migrated from dotnet/linker@8c0df91

Commit migrated from dotnet/linker@b10e1bc

…otnet/linker#2672) Commit migrated from dotnet/linker@a709836

Commit migrated from dotnet/linker@75235a5

…304.3 (dotnet/linker#2676) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@14b3d62

…0307.1 (dotnet/linker#2677) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@26e0c5c

Commit migrated from dotnet/linker@e003f2c

Commit migrated from dotnet/linker@5baeb5e

* Use attributes on lambdas in tests * Fix formatting * Unindent attributes Commit migrated from dotnet/linker@93eac59

Commit migrated from dotnet/linker@98c81d5

* Warn on DAM mismatch between overrides Commit migrated from dotnet/linker@ac6cfb3

…307.6 (dotnet/linker#2684) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@cbcfcfc

…0313.2 (dotnet/linker#2685) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@ae1f280

* Remove SuppressionContextMember Instead of computing the suppression context member when we push to the scope stack, it is now computed on demand when we need to know whether a warning is suppressed. The suppression context should be entirely determined by the static scopes, so there isn't any need to track it dynamically. Commit migrated from dotnet/linker@2303da0

…tnet/linker#2675) Add intrinsic support for Nullable.GetUnderlyingType and support for MakeGenericType with Nullables Adds ArrayCreationOperation visitors to create ArrayValue's in the analyzer, and adds start of dataflow analysis for array values. Adds tests to validate dataflow in Arrays. Co-authored-by: vitek-karas <[email protected]> Commit migrated from dotnet/linker@ed8b22a

Commit migrated from dotnet/linker@1d77412

Commit migrated from dotnet/linker@947f66a

…net/linker#2688) Commit migrated from dotnet/linker@9f2b304

* Add test which mimics what is done in Type.ImplementInterface helper in runtime There's nothing new in this test, but it's better to have coverage for the pattern as it's used in runtime. * Add tests for properties * Simplify Co-authored-by: vitek-karas <[email protected]> Commit migrated from dotnet/linker@5929598

…318.2 (dotnet/linker#2697) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@9f890c6

…0321.1 (dotnet/linker#2698) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@64ad0ff

There seems to be a race condition on Cecil while trying to get GenericParameters, if a multithreaded test asks simultaneously about GenericParameters on the same type Cecil answers could vary creating test failures. Fixes dotnet/linker#2693 Commit migrated from dotnet/linker@5d8277a

* Fix array dataflow test for passing ref to element Commit migrated from dotnet/linker@04c49c9

* Don't let RUC on cctor silence warnings Commit migrated from dotnet/linker@7ef273a

…321.2 (dotnet/linker#2709) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@88746f7

…0328.1 (dotnet/linker#2710) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@da3c743

This adds support for RequiresUnreferencedCode and UnconditionalSuppressMessage on lambdas and local functions, relying on heuristics and knowledge of compiler implementation details to detect lambdas and local functions. This approach scans the code for IL references to lambdas and local functions, which has some limitations. - Unused local functions aren't referenced by the containing method, so warnings from these will not be suppressed by suppressions on the containing method. Lambdas don't seem to have this problem, because they contain a reference to the generated method as part of the delegate conversion. - The IL doesn't in general contain enough information to determine the nesting of the scopes of lambdas and local functions, so we make no attempt to do this. We only try to determine to which user method a lambda or local function belongs. So suppressions on a lambda or local function will not silence warnings from a nested lambda or local function in the same scope. This also adds warnings for reflection access to compiler-generated state machine members, and to lambdas or local functions. For these, the analyzer makes no attempt to determine what the actual IL corresponding to the user code will be, so it produces fewer warnings. The linker will warn for what is actually in IL. Commit migrated from dotnet/linker@cb11422

ViktorHofer · 2022-11-10T08:13:36Z

src/tools/illink/eng/Analyzers.props

+<Project>
+  <ItemGroup Condition="'$(RunAnalyzers)' == 'true'">
+    <PackageReference Include="Microsoft.DotNet.CodeAnalysis" Version="$(MicrosoftDotNetCodeAnalysisVersion)" PrivateAssets="all" IsImplicitlyDefined="true" />
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeStyle" Version="$(MicrosoftCodeAnalysisCSharpCodeStyleVersion)" PrivateAssets="all" />
+    <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="$(MicrosoftCodeAnalysisNetAnalyzersVersion)" PrivateAssets="all" />
+    <!-- <PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.205" PrivateAssets="all" /> -->
+  </ItemGroup>
+
+  <Import Project="$(BootstrapBuildPath)/Microsoft.NET.ILLink.Analyzers.props" Condition="'$(BootstrapBuildPath)' != ''" />
+
+  <!-- Don't enable for Cecil, as they can't be suppressed -->
+  <PropertyGroup Condition="'$(BootstrapBuildPath)' != '' and '$(MSBuildProjectName)' != 'Mono.Cecil.Pdb'">
+    <EnableTrimAnalyzer>true</EnableTrimAnalyzer>
+  </PropertyGroup>
+
+  <ItemGroup Condition="'$(BootstrapBuildPath)' != ''">
+    <Analyzer Include="$(BootstrapBuildPath)/ILLink.RoslynAnalyzer.dll" />
+  </ItemGroup>
+
+</Project>


We already have infrastructure for Analyzers. See https://github.com/dotnet/runtime/blob/main/eng/Analyzers.props. Please use that instead (and consolidate if necessary).

ViktorHofer · 2022-11-10T08:14:07Z

src/tools/illink/eng/azure-pipelines.yml

@@ -0,0 +1,158 @@
+trigger:


Is this file used anywhere? All our pipelines reside under eng/pipelines.

ViktorHofer · 2022-11-10T08:15:45Z

src/tools/illink/eng/ilasm.ilproj

+  <!-- This project exists solely to restore ilasm for use in the test
+  project. -->


We want to use the live built ilasm/ildasm instead of a prebuilt for the tests.

ViktorHofer · 2022-11-10T08:18:17Z

src/tools/illink/test/Mono.Linker.Tests/Mono.Linker.Tests.csproj

+  <PropertyGroup>
+    <DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);DEBUG</DefineConstants>
+  </PropertyGroup>


nit: remove, as that constant is already set by the SDK.

Suggested change

<PropertyGroup>

<DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);DEBUG</DefineConstants>

</PropertyGroup>

ViktorHofer · 2022-11-10T08:19:27Z

src/tools/illink/test/Mono.Linker.Tests/Mono.Linker.Tests.csproj

+    <!-- This reference is purely so that the linker can resolve this
+         dependency of mscorlib. It is not actually required to build
+         the tests. -->
+    <PackageReference Include="System.Threading.AccessControl" Version="5.0.0" />


We centrally define all our versions in Versions.props. Please don't hardcode package versions here as that makes fixing component governance issues and vulnerabilities much harder.

ViktorHofer · 2022-11-10T08:21:28Z

src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="System.Security.Permissions" Version="5.0.0" />


Same here regarding not hardcoding versions outside of Versions.props. Also, this is a stale dependency. The latest one is 7.0.0.

ViktorHofer · 2022-11-10T08:25:42Z

src/tools/illink/src/analyzer/analyzer.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" PrivateAssets="All" Version="1.0.0" />


This is a stale package version. When targeting .NET Framework, reference assembly packages are already auto package referenced. What's the use case here?

I'm not familiar with the analyzer project, @radekdoulik could you elaborate on what is this used for?

ViktorHofer

Left some feedback. Naturally when merging a repository into another one, a lot of infrastructure comes along. There is a lot of duplication right now as we in runtime already handle most of the scenarios that linker had to handle itself.

What concerns me is that the linker under src/tools/linker doesn't use any of the repository infrastructure (as the repo root's Directory.Build.props and Directory.Build.targets msbuild files aren't imported). I would like to see that cleaned-up as otherwise we will get into a component governance hell with stale and vulnerable dependencies.

stephentoub · 2022-11-10T14:01:13Z

...external/corert/src/System.Private.CoreLib/shared/System/Reflection/AssemblyNameFormatter.cs

@@ -0,0 +1,156 @@
+// Licensed to the .NET Foundation under one or more agreements.


Why is all this corert code needed? What's the plan for getting rid of it? Why aren't the current version of these files for coreclr, mono, or nativeaot sufficient?

Agreed that we should try to use common reflection sources for this. cc @dotnet/area-system-reflection

Opened #77868 to track this work

stephentoub · 2022-11-10T14:03:46Z

src/tools/illink/PATENTS.TXT

@@ -0,0 +1,44 @@
+Microsoft Patent Promise for Mono


Is this needed? (The root of the repo has a different PATENTS.TXT.)

cc: @richlander

src/tools/illink/test/Mono.Linker.Tests.Cases/Symbols/Dependencies/LibraryWithMdb/compile.sh

ericstj · 2022-11-10T17:25:59Z

...external/corert/src/System.Private.CoreLib/shared/System/Reflection/AssemblyNameFormatter.cs

@@ -0,0 +1,156 @@
+// Licensed to the .NET Foundation under one or more agreements.


Agreed that we should try to use common reflection sources for this. cc @dotnet/area-system-reflection

ericstj

Agree with a lot of the things that others have said. In principle, the addition of the linker should be as a set of simple managed assemblies that build after the libraries subset. They should leverage props/targets defined by the libraries to help them build against the live framework. There should not be a need for custom infrastructure to solve problems that are already solved a different way in the repo.

agocke · 2022-11-10T19:14:36Z

Agreed on merging things with runtime -- but I don't want the initial commit to get bogged down in the improvements that come with sharing. This is a clean merge of the repo in its current state from dotnet/linker. Hooking things together and sharing files can and should be done in follow-up PRs.

jkoritzinsky · 2022-11-14T20:24:50Z

Agree with a lot of the things that others have said. In principle, the addition of the linker should be as a set of simple managed assemblies that build after the libraries subset. They should leverage props/targets defined by the libraries to help them build against the live framework. There should not be a need for custom infrastructure to solve problems that are already solved a different way in the repo.

Have we considered how we will consume the linker in System.Private.CoreLib's build that occurs before the libraries build if the linker references projects defined in the libraries build? Or is that not an eventual goal (and we'll always consume an LKG linker for the "library linking" step)?

* Merge and remove common files Remove arcade eng\common directory in src\tools\illink since now we will use the runtime arcade infra Remove build.cmd/build.sh and lint.cmd/lint.sh in src\tools\illink directory since now they will execute via a subset Remove/Merge common files from src\tools\illink root: - .gitattributes - .gitignore - .github - .gitmodules - after.illink.sln.targets - code_of_conduct.md - global.json - LICENSE.txt - NuGet.config - THIRD-PARTY-NOTICES.TXT Remove/Merge common files from src\tools\illink\eng: - Build.props - Publishing.props - Signing.props - SourceBuild.props - SourceBuildPrebuiltBaseline.xml - Tools.props - Version.Details.xml - Versions.props * Create subsets to be able to build illink Create a variable for the tools folder in runtime Add subsets tools.illink and tools.illinktests for building illink and unitest it Add Microsoft.DotNet.Cecil dependencies to runtime and to illink projects Some workarounds to be able to build illink Delete some cecil information from the external folder since now its a package * Refactorings to make test work Test projects use to have relative paths based on the current working directory to know where to find stuff, now that the project is in a different place things are not found, this commit changes to instead use MSBuild variables to calculate where things are Add the cecil package to tests Change a cecil test that verify the official package name to only care about the important pieces * Enable pipeline Add a variable to recognize when illink contains a change, and set an exclusion of the src/tools/* for other repos Reuse the dotnet-linker-tests pipeline file to also run illink unitests every time there are illink changes * Fix Markdown lint * Remove checked-in binaries * Use nunit for linker tests and fix cecil version test

tlakollo · 2022-11-15T20:46:50Z

Sven added an issue in runtime tracking most of the issues will remain open after this PR is merged #78334. I squashed the feedback that was included for this PR into a single commit from a previous PR that included all fixes made to be able to build and test the illink repo

agocke · 2022-11-15T20:47:30Z

Agree with a lot of the things that others have said. In principle, the addition of the linker should be as a set of simple managed assemblies that build after the libraries subset. They should leverage props/targets defined by the libraries to help them build against the live framework. There should not be a need for custom infrastructure to solve problems that are already solved a different way in the repo.

Have we considered how we will consume the linker in System.Private.CoreLib's build that occurs before the libraries build if the linker references projects defined in the libraries build? Or is that not an eventual goal (and we'll always consume an LKG linker for the "library linking" step)?

At the moment, I think consuming an LKG of one of those two items is the right thing to do. Certainly, consuming a linker LKG is what we do today, so the product is no worse for it. I would do that for now.

sbomer

LGTM as long as we address the remaining feedback in a follow-up. @tlakollo @agocke is this good to merge?

agocke

LGTM

dotnet-maestro bot and others added 30 commits February 28, 2022 13:39

Update dependencies from https://github.com/dotnet/runtime build 2022…

0191b0a

…0227.1 (dotnet/linker#2664) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@396c37d

Remove mention of PreserveDependencyAttribute from docs (dotnet/linke…

2b08bd5

…r#2666) Commit migrated from dotnet/linker@a8f9532

Remove special logic for TypeConverterAttribute (dotnet/linker#2659)

f9f184f

Commit migrated from dotnet/linker@b10e1bc

Don't dispose of linked assembly in VerifyLinkingOfOtherAssemblies (d…

d03f17d

…otnet/linker#2672) Commit migrated from dotnet/linker@a709836

Add testcase for intrinsic with override (dotnet/linker#2674)

39f46f9

Commit migrated from dotnet/linker@75235a5

Update dependencies from https://github.com/dotnet/arcade build 20220…

3dc0bd0

…304.3 (dotnet/linker#2676) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@14b3d62

Update dependencies from https://github.com/dotnet/runtime build 2022…

c4e8e97

…0307.1 (dotnet/linker#2677) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@26e0c5c

Remove fixing-warnings.md and link to dotnet/docs (dotnet/linker#2645)

501ebec

Commit migrated from dotnet/linker@e003f2c

Expose GetTypePreserve (dotnet/linker#2679)

62d06f3

Commit migrated from dotnet/linker@5baeb5e

Use attributes on lambdas in tests (dotnet/linker#2682)

9a8964f

* Use attributes on lambdas in tests * Fix formatting * Unindent attributes Commit migrated from dotnet/linker@93eac59

Share Type.Base intrinsic (dotnet/linker#2681)

b6a4a1c

Commit migrated from dotnet/linker@98c81d5

Warn on DAM mismatch between overrides (dotnet/linker#2656)

611a613

* Warn on DAM mismatch between overrides Commit migrated from dotnet/linker@ac6cfb3

Update dependencies from https://github.com/dotnet/arcade build 20220…

9198978

…307.6 (dotnet/linker#2684) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@cbcfcfc

Update dependencies from https://github.com/dotnet/runtime build 2022…

3e8ae0b

…0313.2 (dotnet/linker#2685) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@ae1f280

Create shared WellKnownTypes enum (dotnet/linker#2692)

40f5127

Commit migrated from dotnet/linker@1d77412

Fix empty/null handling for Type.BaseType intrinsic (dotnet/linker#2694)

d4a383e

Commit migrated from dotnet/linker@947f66a

Add tests for analyzer null references in byte array COM interop (dot…

56212d6

…net/linker#2688) Commit migrated from dotnet/linker@9f2b304

Update dependencies from https://github.com/dotnet/arcade build 20220…

93b34e3

…318.2 (dotnet/linker#2697) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@9f890c6

Update dependencies from https://github.com/dotnet/runtime build 2022…

1bc28d5

…0321.1 (dotnet/linker#2698) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@64ad0ff

Update array data flow test (dotnet/linker#2704)

80c6c04

* Fix array dataflow test for passing ref to element Commit migrated from dotnet/linker@04c49c9

Don't let RUC on cctor silence warnings (dotnet/linker#2705)

c948f43

* Don't let RUC on cctor silence warnings Commit migrated from dotnet/linker@7ef273a

Update dependencies from https://github.com/dotnet/arcade build 20220…

599bc37

…321.2 (dotnet/linker#2709) [main] Update dependencies from dotnet/arcade Commit migrated from dotnet/linker@88746f7

Update dependencies from https://github.com/dotnet/runtime build 2022…

6240ad3

…0328.1 (dotnet/linker#2710) [main] Update dependencies from dotnet/runtime Commit migrated from dotnet/linker@da3c743

ViktorHofer reviewed Nov 10, 2022

View reviewed changes

stephentoub reviewed Nov 10, 2022

View reviewed changes

ericstj reviewed Nov 10, 2022

View reviewed changes

runfoapp bot mentioned this pull request Nov 14, 2022

Infra improvements for Helix #68176

Closed

This was referenced Nov 14, 2022

Use runtime build infrastructure for tools.illink subset #78334

Closed

Use common reflection sources for illink codebase #78335

Closed

sbomer mentioned this pull request Nov 14, 2022

Re-enable illink tests which rely on checked-in binaries #78344

Open

build-analysis bot mentioned this pull request Nov 15, 2022

System.IO.Pipes.Tests.NamedPipeTest_Specific.ClientConnectAsync_Cancel_With_InfiniteTimeout failing on Libraries Test Run release coreclr windows x86 Release #69101

Open

tlakollo force-pushed the LinkerIntoRuntime2 branch from 80a23bc to 1f2c6cc Compare November 15, 2022 20:43

sbomer approved these changes Nov 16, 2022

View reviewed changes

agocke approved these changes Nov 16, 2022

View reviewed changes

agocke merged commit 0fe8bd3 into main Nov 16, 2022

agocke deleted the LinkerIntoRuntime2 branch November 16, 2022 22:12

tlakollo mentioned this pull request Nov 17, 2022

Add cecil as a dependency of the runtime #77029

Closed

am11 removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 17, 2022

ghost locked as resolved and limited conversation to collaborators Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linker into runtime2 #78077

Linker into runtime2 #78077

tlakollo commented Nov 9, 2022

ViktorHofer Nov 10, 2022

ViktorHofer Nov 10, 2022

ViktorHofer Nov 10, 2022 •

edited

Loading

ViktorHofer Nov 10, 2022

ViktorHofer Nov 10, 2022

ViktorHofer Nov 10, 2022 •

edited

Loading

ViktorHofer Nov 10, 2022 •

edited

Loading

tlakollo Dec 14, 2022

ViktorHofer left a comment

stephentoub Nov 10, 2022

ericstj Nov 10, 2022

tlakollo Nov 15, 2022

stephentoub Nov 10, 2022

ericstj Nov 10, 2022

ericstj left a comment

agocke commented Nov 10, 2022

jkoritzinsky commented Nov 14, 2022

tlakollo commented Nov 15, 2022

agocke commented Nov 15, 2022

sbomer left a comment

agocke left a comment

		<!-- This project exists solely to restore ilasm for use in the test
		project. -->

	<PropertyGroup>
	<DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);DEBUG</DefineConstants>
	</PropertyGroup>

		@@ -0,0 +1,156 @@
		// Licensed to the .NET Foundation under one or more agreements.

Linker into runtime2 #78077

Linker into runtime2 #78077

Conversation

tlakollo commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

ViktorHofer Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

agocke commented Nov 10, 2022

jkoritzinsky commented Nov 14, 2022

tlakollo commented Nov 15, 2022

agocke commented Nov 15, 2022

sbomer left a comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

ViktorHofer Nov 10, 2022 •

edited

Loading

ViktorHofer Nov 10, 2022 •

edited

Loading

ViktorHofer Nov 10, 2022 •

edited

Loading