[main] Update dependencies from dotnet/linker

[dotnet-maestro]

This pull request updates the following dependencies

From https://github.com/dotnet/linker

• Subscription: 0d6d6ae4-f71f-4395-53e6-08d8e409d87d
• Build: 20220420.4
• Date Produced: April 21, 2022 12:19:25 AM UTC
• Commit: 6374217e191b8cef0c5a3d862f4291583eb959f4
• Branch: refs/heads/main

• Updates:
  • Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22205.1 to 7.0.100-1.22220.4

[vitek-karas]

I'll be looking into the linker warning failure.

[lewing]

@vitek-karas latest failures look real

[vitek-karas]

Thanks @lewing - I know. The fix is relatively simple, but I would rather figure out "why" first.

/cc @marek-safar as this is likely related to the constant propagation change (I don't know for sure though)

The failure is on Mono only.
What happens is:

• We build System.Private.CoreLib and it includes ResourceReader.InitializeBinaryFormatter method
• We trim System.Private.CoreLib in library mode
  • This removed the InitializeBinaryFormatter method
  • Note that the method is private and doesn't implement any interface method
• We then run the "framework" (sfx) run of linker on the output (all libraries at once to gather warning)
  • The ILLinkSubstitutions.Shared.xml in System.Private.CoreLib has a line which substitutes the InitializeBinaryFormatter method with a false
  • This produces a warning since the method doesn't exist in the input

Note: It's intentional that linker warns if there are methods in substitution XML which don't resolve. This is to protect against typos, as it would be really easy to make a typo, no failure happens anywhere, but the substitution didn't work.

This probably means that the method is not used on Mono and the new linker can remove the InitializeBinaryFormatter method in the Mono build now, while older version couldn't do it.

CoreCLR probably has a hard reference to the method and thus linker has to keep it.

What we need to do:

• Figure out if the above theory is correct
• Figure out what change in the linker caused the removal of the method - and if it's actually correct
• If the above comes out OK, the fix would be to move the substitution for InitializeBinaryFormatter into the CoreCLR specific part of substitutions for CoreLib.

[vitek-karas]

/cc @sbomer as my backup as I'm feeling a bit ill and it might take a while for me to figure this out.

[sbomer]

I'll take a look to see if I can confirm the theory.

[sbomer]

@vitek-karas your theory is pretty much correct. The missing detail was that mono-wasm has a substitution that stubs out InitializeBinaryFormatter:

runtime/src/mono/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.wasm.xml

Line 13 in bd29496

<method signature="System.Boolean InitializeBinaryFormatter()" body="stub" value="false" />

So that gets inlined when building SPC for wasm and the method isn't marked.

Maybe a way to fix this is to remove the wasm-specific substitution, and instead stub it out by passing a feature setting to the linker (EnableUnsafeBinaryFormatterSerialization = false) - since the substitution is effectively saying that the feature is unsupported on wasm, and it seems redundant to declare the stub in two places. But this would need to be set for the shared fx link and also when linking in the SDK.

Or we could ensure that the shared fx substitutions for wasm don't contain the feature switch definition, by moving it to a separate XML file for non-wasm targets only - probably the quickest solution.

Generally I think the feature switches need a mechanism to "remember" that a feature setting got baked in. We are doing optimizations based on a baked-in value but are still shipping with an XML that declares that corelib supports the feature switch - and now that we are doing more optimizations based on the setting it's causing problems. @marek-safar curious what you think.

[marek-safar]

@sbomer / @vitek-karas I think it's ok to remove this "outliner" from ILLink.Substitutions.wasm.xml. The SDK already set EnableUnsafeBinaryFormatterSerialization to false by default for every other project anyway.

Another option could be to not apply substitutions during library build but I'm not convinced that's the right fix for this case.

[ViktorHofer]

@marek-safar @sbomer @vitek-karas kindly asking for an update on the PR as my change depends on the newer linker version and is currently blocked.

[sbomer]

@marek-safar the icu test is still failing on some platforms:

Assert.NotNull() Failure
   at System.Globalization.Tests.IcuTests.IcuShouldBeUsedByDefault() in /_/src/libraries/System.Globalization/tests/IcuTests.cs:line 38

IcuGetCultureDataFromRegionName is probably getting inlined causing your new assert to fail:

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Icu.cs

Line 212 in 47bcd1a

private static CultureData? IcuGetCultureDataFromRegionName() => null;

[marek-safar]

Failures are unrelated

[ViktorHofer]

@marek-safar The tar issues were already fixed but unsure about the wasm failure. I filed an issue for the new runtime failure so that the area owners know about flakiness in that test: #68376.

[AndyAyersMS]

Possible regression (though no clue why this change would lead to these tests regressing):

• [Perf] Changes at 4/22/2022 8:14:55 AM perf-autofiling-issues#4833