Enable verbose linker output & add baseline warning suppressions for library builds #39133

layomia · 2020-07-11T00:47:37Z

layomia · 2020-07-11T00:58:07Z

eng/illink.targets

+           as we need both linker annotations and xml files
+           suppress "error IL2047: All overridden members must have the same DynamicallyAccessedMembersAttribute usage",
+           as we don't add suppression files for ref projects -->
+      <ILLinkArgs>$(ILLinkArgs) --nowarn IL2025;IL2047</ILLinkArgs>


IL2047

This is a workaround for dotnet/linker#1348.

All the 2047 should be fixed within the runtime. I know @MichalStrehovsky planned to look into it. I think we should have an issue for fixing these in the runtime - or add them to the baselines as well.

dotnet/linker#1348 might help make the baselines "smaller" if we link against implementation assemlies.

mateoatr · 2020-07-11T00:59:17Z

/cc @sbomer @vitek-karas

marek-safar · 2020-07-12T07:40:23Z

Should there be tracking issue to actually fix the warnings?

marek-safar · 2020-07-12T07:42:04Z

src/coreclr/src/System.Private.CoreLib/ILLinker/ILLink.Suppressions.cs

+
+using System.Diagnostics.CodeAnalysis;
+
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2006", Scope = "member", Target = "F:System.LazyDebugView`1._lazy")]


I'd make sense to have the warnings for shared SPC in the shared location (there might not be many in runtime specific code)

jkotas · 2020-07-13T03:34:29Z

src/libraries/System.ComponentModel.Annotations/src/MatchingRefApiCompatBaseline.txt

@@ -0,0 +1,2 @@
+# Exposed publicly in the implementation to suppress linker warnings.
+TypesMustExist : Type 'System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute' does not exist in the reference but it does exist in the implementation.


Should the type be public in CoreLib only, and be internal everywhere else to make this unnecessary?

It is what we do in other similar cases. For example:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/DynamicallyAccessedMembersAttribute.cs

Line 28 in 6072e4d

#if SYSTEM_PRIVATE_CORELIB

vitek-karas · 2020-07-13T08:33:47Z

src/coreclr/src/System.Private.CoreLib/ILLinker/ILLink.Suppressions.cs

+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2047", Scope = "member", Target = "M:System.Reflection.Emit.SymbolType.GetProperties(System.Reflection.BindingFlags)")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2047", Scope = "member", Target = "M:System.Reflection.Emit.SymbolType.GetPropertyImpl(System.String,System.Reflection.BindingFlags,System.Reflection.Binder,System.Type,System.Type[],System.Reflection.ParameterModifier[])")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2047", Scope = "member", Target = "M:System.Reflection.Emit.SymbolType.InvokeMember(System.String,System.Reflection.BindingFlags,System.Reflection.Binder,System.Object,System.Object[],System.Reflection.ParameterModifier[],System.Globalization.CultureInfo,System.String[])")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2047", Scope = "member", Target = "M:System.Reflection.Emit.TypeBuilder.GetConstructorImpl(System.Reflection.BindingFlags,System.Reflection.Binder,System.Reflection.CallingConventions,System.Type[],System.Reflection.ParameterModifier[])")]


Above you seem to disable this warning all up - we should either disable it, or add the baselines for it... not both.

joperezr · 2020-07-13T18:24:54Z

eng/illink.targets

@@ -192,6 +192,13 @@
      <ILLinkArgs>$(ILLinkArgs) --disable-opt unusedinterfaces</ILLinkArgs>
      <!-- keep DynamicDependencyAttribute unless a project explicitly disables it -->
      <ILLinkArgs Condition="'$(ILLinkKeepDepAttributes)' != 'false'">$(ILLinkArgs) --keep-dep-attributes true</ILLinkArgs>
+      <!-- enable verbose output -->
+      <ILLinkArgs>$(ILLinkArgs) --verbose</ILLinkArgs>


In case these ever need to be conditioned out for a specific reason, can we add a property that individiual projects can override for this?

I believe this is going to be removed as soon as the linker enables warnings by default. So there is probably no reason to add a property here - it is just going to be deleted.

joperezr · 2020-07-13T18:26:17Z

eng/illink.targets

+      <ILLinkArgs>$(ILLinkArgs) --verbose</ILLinkArgs>
+      <!-- suppress "error IL2025: Duplicate preserve of" warnings from verbose output,
+           as we need both linker annotations and xml files
+           suppress "error IL2047: All overridden members must have the same DynamicallyAccessedMembersAttribute usage",
+           as we don't add suppression files for ref projects -->
+      <ILLinkArgs>$(ILLinkArgs) --nowarn IL2025;IL2047</ILLinkArgs>


Suggested change

<ILLinkArgs>$(ILLinkArgs) --verbose</ILLinkArgs>



<ILLinkArgs>$(ILLinkArgs) --nowarn IL2025;IL2047</ILLinkArgs>

<ILLinkArgs Condition="'$(DisableILLinkWarnings)' != 'true'">$(ILLinkArgs) --verbose</ILLinkArgs>



<ILLinkArgs Condition="'$(DisableILLinkWarnings)' != 'true'">$(ILLinkArgs) --nowarn IL2025;IL2047</ILLinkArgs>

so basically something like this is what I mean.

joperezr · 2020-07-13T18:38:42Z

src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj

@@ -270,6 +270,7 @@
    <Compile Include="$(CommonPath)Interop\Windows\OleAut32\Interop.SysAllocStringByteLen.cs">
      <Link>Common\Interop\Windows\OleAut32\Interop.SysAllocStringByteLen.cs</Link>
    </Compile>
+    <Compile Include="ILLinker\ILLink.Suppressions.cs" />


NIT: Consider adding this on the eng\illink.targets file so that we automatically add this to the compile in case it exists. This is what we do in other baseline cases for example: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L31-L32

That way adding a new supression doesn't mean you have to go author the .csproj which might be harder for folks to do if they are not familiar with msbuild.

I think this is more than a NIT. 😄 We should pick it up automatically.

eerhardt · 2020-07-13T21:33:42Z

src/coreclr/src/System.Private.CoreLib/ILLinker/ILLink.Suppressions.cs

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


The existing folder name in mono is ILLInk not ILLinker. Can we name the folder consistently throughout the repo?

eerhardt · 2020-07-13T21:40:26Z

src/libraries/System.Private.CoreLib/src/System/ComponentModel/DefaultValueAttribute.cs

@@ -57,38 +57,38 @@ public DefaultValueAttribute(Type type, string? value)
                {
                    _value = Convert.ChangeType(value, type, CultureInfo.InvariantCulture);
                }
+            }


What was this change for? Is there a bug somewhere?

eerhardt · 2020-07-13T21:42:33Z

src/libraries/System.Reflection.DispatchProxy/src/System.Reflection.DispatchProxy.csproj

@@ -11,6 +11,9 @@
  <PropertyGroup>
    <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetFramework)' == 'netstandard2.0'">SR.PlatformNotSupported_ReflectionDispatchProxy</GeneratePlatformNotSupportedAssemblyMessage>
  </PropertyGroup>
+  <ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">


We aren't doing this elsewhere, are we? If this is reporting linker warnings, we will probably need the attributes in this project so we might as well add the UnconditionalSuppressMessageAttribute now.

eerhardt · 2020-07-13T21:43:49Z

...s/System.Security.Cryptography.Algorithms/src/System.Security.Cryptography.Algorithms.csproj

@@ -587,6 +587,9 @@
    <Compile Include="$(CommonPath)Interop\Unix\Interop.Libraries.cs"
             Link="Common\Interop\Unix\Interop.Libraries.cs" />
  </ItemGroup>
+  <ItemGroup Condition="$(TargetFramework.StartsWith('$(NetCoreAppCurrent)'))">


Similar comment here - we should add the internal attribute we need so we don't need to #if in the code when we apply them.

eerhardt · 2020-07-13T21:47:46Z

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.xml

@@ -665,9 +643,7 @@

 		<!-- domain.c: mono_defaults.thread_class -->
 		<!-- FIXME: -->


@marek-safar @akoeplinger @vargaz - do you know what this FIXME is here for? Are we fixing it with this change?

@vargaz any recollection why this was added?

eerhardt · 2020-07-13T21:49:00Z

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.cs

+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2046", Scope = "member", Target = "M:System.Reflection.RuntimeModule.ResolveSignature(System.Int32)")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2046", Scope = "member", Target = "M:System.Reflection.RuntimeModule.ResolveString(System.Int32)")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2046", Scope = "member", Target = "M:System.Reflection.RuntimeModule.ResolveType(System.Int32,System.Type[],System.Type[])")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2047", Scope = "member", Target = "M:System.Reflection.Emit.EnumBuilder.GetConstructorImpl(System.Reflection.BindingFlags,System.Reflection.Binder,System.Reflection.CallingConventions,System.Type[],System.Reflection.ParameterModifier[])")]


Aren't we disabling IL2047 all together? Do we need these here as well?

MichalStrehovsky · 2020-07-14T13:02:33Z

src/coreclr/src/System.Private.CoreLib/ILLinker/ILLink.Suppressions.cs

+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2006", Scope = "member", Target = "M:Internal.Runtime.InteropServices.ComActivator.BasicClassFactory.CreateInstance(System.Object,System.Guid@,System.Object@)")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2006", Scope = "member", Target = "M:Internal.Runtime.InteropServices.ComActivator.ClassRegistrationScenarioForType(Internal.Runtime.InteropServices.ComActivationContext,System.Boolean)")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2006", Scope = "member", Target = "M:Internal.Runtime.InteropServices.ComponentActivator.InternalGetFunctionPointer(System.Runtime.Loader.AssemblyLoadContext,System.String,System.String,System.IntPtr)")]
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2006", Scope = "member", Target = "M:Internal.Runtime.InteropServices.LicenseInteropProxy.#ctor")]


How are we running linker on this (what's the command line)? Linker should be able to analyze this. I've seen this warning show up when linker isn't passed a reference to System.ComponentMode.TypeConverter assembly and can't analyze the reflection as a result.

(There's multiple instances of the same problem within this suppression file.)

I think these are the arguments that are passed when linking CoreLib:

-reference "D:\source\repos\runtime\artifacts\obj\coreclr\System.Private.CoreLib\x64\Debug\PreTrim/System.Private.CoreLib.dll" -out "D:\source\repos\runtime\artifacts\obj\coreclr\System.Private.CoreLib\x64\Debug" -r System.Private.CoreLib -c skip -u skip -p link System.Private.CoreLib -t -b true -v true --strip-descriptors false --strip-substitutions false --skip-unresolved true --disable-opt unusedinterfaces --keep-dep-attributes true

Is there any chance we can have linker see the whole framework when trimming the individual libraries? (I think this comes back to an old topic...)

Without that, we're going to see warnings for all Type.GetType("Foo, X").GetMethod("Bar") patterns because even though linker could know what Foo refers to, it won't be able to resolve that and has to treat it as "don't know". We'll never know if that reflection is actually safe (linker will be able to reason about it for actual app build) or not (linker can no longer reason about it when seeing the whole framework either because e.g. someone refactored things).

Is there any chance we can have linker see the whole framework when trimming the individual libraries?

This approach was rejected in #31712

Do we need to revisit that? What are the options to avoid the bogus warnings like this one?

Do we need to revisit that?

In my mind, this is no different than the advantages already outlined in that issue - #31712 (comment). Specifically:

The linker can better understand the code when it sees the entire "distributable".

This is just another case of the linker not being able to reason about everything at once.

What are the options to avoid the bogus warnings like this one?

One option is to keep doing what we are doing in this PR. It suppresses the warning.

One option is to pass in suppress messages separately to the linker that are only for the "Library Build", just like we do with the ILLinkTrim_LibraryBuild.xml files.

Lastly, we could revisit Run illink task with at the end of build #31712 and change the way we run the linker during the build to do the whole shared fx/runtimepack at once.

A variation on this approach is to only run the validation against the whole shared fx/runtimepack at the of the build, and throw away the outputted binaries. We still run the linker during the individual library build, and suppress all warnings like we currently are. The warnings only come at the end, during the full build.

This addresses the main concerns in Run illink task with at the end of build #31712 that the inner dev loop produces the same binaries as the full build.

A variation on this approach is to only run the validation against the whole shared fx/runtimepack at the of the build, and throw away the outputted binaries

That sounds most reasonable option to me.

Yep, if we want to do the precise analysis we have to run against the whole framework and every runtime pack. Doing it the way as it's today will rely on manual information copying which is very error-prone in our scale.

The first two options (suppress warnings unconditionally or suppress warning during library build) run the risk that if something happens and linker stops recognizing the pattern we either don't see the warning at all, or only customers see the warnings. That's quite undesirable for things as fragile as "app-specific customization of the framework" (which is what trimming+feature switches really is - no two frameworks will be the same and it's hard to prove correctness without automation help).

I like option 3 the best too even though it's probably more work.

As much as I don't like running the same tool twice, it sounds as the best option.

ericstj · 2020-07-22T16:21:02Z

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.cs

+
+using System.Diagnostics.CodeAnalysis;
+
+[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2001", Scope = "type", Target = "T:Mono.ValueTuple")]


This seems pretty excessive to ship inside the shipping assembly if the goal is just to result in no warnings. Does the linker provide a way that we could list granular suppressions for the purpose of our build, but then a single assembly-wide suppression in the shipping assembly?

I would have probably assumed that the linker would actually take these into consideration for verbose warnings, but then linked them out, is that not the case?

For assembly-wide suppression we can do this:

[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2001")] [assembly: UnconditionalSuppressMessage ("ILLinker", "IL2002")] ...

But if we do assembly-wide suppressions, we won't get new warnings when writing new code in the assembly.

I would have probably assumed that the linker would actually take these into consideration for verbose warnings, but then linked them out, is that not the case?

My understanding is that they remain in the final binaries in the runtime-pack and shared framework, since today those are the same and need to be considered as input to customer app when linking.

if we do assembly-wide suppressions, we won't get new warnings when writing new code in the assembly.

Can we still have granular suppressions though some mechanism when we're building? Re-reading your original issue I see the goal is not to "always suppress" these in framework code, but only to "temporarily suppress" them until we can properly annotate everything. I guess in that case this bloat is a point-in-time problem.

I discussed this with @eerhardt - there was a disconnect on our end... anyway:

Hardcoding the suppressions into the library source code feels like a too much of a final solution (it means there's no way to "reenable" these warnings without rebuilding the library)

We want the baselines to exist for the runtime repo build, but we don't necessarily want/need them for the application build

Linker/SDK will have a solution to suppress all of these warnings by default anyway (Introduce option to enable trim analysis warnings sdk#12388) - so we don't NEED to have a solution for this in the runtime repo

We would like to have the freedom to tweak the user experience (as in how much warnings we show to the user) later on - this means the libraries themselves should not hide anything - if we need to we will hide it in higher layers (linker or SDK)

So the technical solution we thought would be best is to:

Encode the suppressions in linker attribute XML files (basically just like the CS, but encoded in XML - https://github.com/mono/linker/blob/master/docs/data-formats.md#custom-attributes-annotations-format)

We will change the linker to be able to generate the suppressions in the XML format automatically - Add an option to generate the suppressions in the linker attribute XML format linker#1380

We would then pass these XMLs only on the command line to the linker - so they take effect during the runtime repo build, but they don't get baked into the product.

layomia · 2020-07-22T21:21:03Z

Thanks all for the review feedback & discussion. Closing this PR based on the comments in #39133 (comment). Will revisit it when the needed support in the linker (generation of suppression XML files) is added.

cc @eerhardt, @vitek-karas, @mateoatr, @sbomer

layomia added the area-Meta label Jul 11, 2020

layomia added this to the 5.0.0 milestone Jul 11, 2020

layomia requested review from akoeplinger, EgorBo, jozkee, marek-safar, steveharter, steveisok and vargaz as code owners July 11, 2020 00:47

layomia self-assigned this Jul 11, 2020

layomia requested review from eerhardt, joperezr and mateoatr July 11, 2020 00:50

layomia mentioned this pull request Jul 11, 2020

Provide option to specify which assemblies to emit linker warnings for dotnet/linker#1348

Closed

layomia commented Jul 11, 2020

View reviewed changes

marek-safar reviewed Jul 12, 2020

View reviewed changes

jkotas reviewed Jul 13, 2020

View reviewed changes

jkotas mentioned this pull request Jul 13, 2020

Preserve DebuggerTypeProxyAttribute classes #39126

Merged

vitek-karas reviewed Jul 13, 2020

View reviewed changes

joperezr reviewed Jul 13, 2020

View reviewed changes

eerhardt reviewed Jul 13, 2020

View reviewed changes

MichalStrehovsky reviewed Jul 14, 2020

View reviewed changes

sbomer mentioned this pull request Jul 17, 2020

Support --warn <version_int> dotnet/linker#1359

Closed

mateoatr mentioned this pull request Jul 21, 2020

Warn by default dotnet/linker#1372

Merged

Add baseline linker warning suppressions for library builds

367bb8d

layomia force-pushed the baselines branch from c868f25 to 399c6ea Compare July 21, 2020 20:32

Fix up warnings for more configurations

b3d396b

layomia force-pushed the baselines branch from 399c6ea to b3d396b Compare July 21, 2020 20:43

ericstj reviewed Jul 22, 2020

View reviewed changes

vitek-karas mentioned this pull request Jul 22, 2020

Add an option to generate the suppressions in the linker attribute XML format dotnet/linker#1380

Closed

layomia closed this Jul 22, 2020

eerhardt mentioned this pull request Aug 3, 2020

Add step to run linker on entire runtime pack during libraries build #40172

Merged

layomia deleted the baselines branch August 10, 2020 23:18

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020


		using System.Diagnostics.CodeAnalysis;

		[assembly: UnconditionalSuppressMessage ("ILLinker", "IL2006", Scope = "member", Target = "F:System.LazyDebugView`1._lazy")]

		@@ -0,0 +1,2 @@
		# Exposed publicly in the implementation to suppress linker warnings.
		TypesMustExist : Type 'System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute' does not exist in the reference but it does exist in the implementation.

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

		@@ -665,9 +643,7 @@

		<!-- domain.c: mono_defaults.thread_class -->
		<!-- FIXME: -->

Enable verbose linker output & add baseline warning suppressions for library builds #39133

Enable verbose linker output & add baseline warning suppressions for library builds #39133

Conversation

layomia commented Jul 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateoatr commented Jul 11, 2020

marek-safar commented Jul 12, 2020

marek-safar Jul 12, 2020 • edited Loading

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

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

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

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

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

Choose a reason for hiding this comment

layomia commented Jul 22, 2020

marek-safar Jul 12, 2020 •

edited

Loading