Avoid copy action for root assemblies #96363

sbomer · 2023-12-30T00:22:25Z

Changes TrimmerRootAssembly (and the underlying option -a) to root all members of the assembly, without setting the action to copy. This allows us to perform branch removal and attribute removal for the assembly.

Fixes #95429

ghost · 2023-12-30T00:22:36Z

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	sbomer
Assignees:	-
Labels:	`linkable-framework`, `area-Tools-ILLink`
Milestone:	-

Preserve existing marking behavior by letting MarkStep do the same work that it does for copy assemblies, including marking attributes.

With the change to MarkEntireType that calls MarkType instead of Annotations.Mark, we are now reaching MarkType for the dependency from the copy assembly to the attribute it contains, producing an unexpected IL2045. Silence this by checking the DependencyKind.

BitVector32.CreateMask(int.MinValue) was incorrectly being treated as returning constant 1. The IL from the beginning of that method: IL_0000: ldarg.0 IL_0001: brtrue.s IL_0005 IL_0003: ldc.i4.1 IL_0004: ret The brtrue.s branch was being analyzed as not taken because the argument was not 1. The branch should be analyzed as taken for any non-zero argument.

And don't set ForceParse for rooted methods. ForceParse was preventing the XML substitutions from applying. This change may make it possible for substitutions in copy assemblies to be honored...

lambdageek · 2024-01-08T17:31:16Z

The ApplyUpdateTests are running for mono wasm in Release mode, where DebuggerSupport was set to false by default. This illink change results in DebuggableAttribute being removed, causing test failures, so I set DebuggerSupport to true for these tests to preserve the attribute. Now I am getting failures which I don't know how to diagnose, and I'm not sure whether they're expected. For example, this assert is hitting:

runtime/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs

Line 226 in 89af7af

Assert.NotNull(mi1);

@lambdageek Could you please advise? Would it be better to simply disable these tests in Release mode, or are these failures worth investigating?

edit: It seems those failures aren't reproing in CI - maybe I was doing something incorrectly locally.

@sbomer The EnC tests depend at runtime on the assemblies built from the projects in the ApplyUpdate subdirectory. Those assemblies need to be compiled by Roslyn with optimizations off and with emitted debug information see https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/Directory.Build.props

(specifically when a runtime that supports hot reload tries to apply an update to a particular assembly, that assembly must have [assembly:System.Diagnostics.DebuggableAttribute(DebuggingModes.DisableOptimizations)]).

In general the assemblies in the ApplyUpdate directory must not be modified by the trimmer at all - otherwise the hot reload deltas will not match the assembly contents and the runtime will do something nonsensical. (in real world usage the deltas are generated by VS; for the tests it's the *.dll.1.dmeta and *.dll.1.dil files that are generated during the test build ahead of time). You can think of the hot reload deltas as some kind of log that says "add a new row N to table X with contents ABCD; modify row M of table Y with contents SPQR; etc" - if the trimmer disturbs the baseline assembly at all - things will go wrong.

Would it be better to simply disable these tests in Release mode

I would prefer not to do that, if possible. Using a Release runtime with a Release BCL with debuggable untrimmed "user code" is much closer to the actual scenario that users will be running.

FIxing the checkRemainingErrors behavior results in too many spurious failures. Use LogDoesNotContainAttribute for now.

src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj

MichalStrehovsky

Looks reasonable to me although I'm not an expert on this codebase.

MichalStrehovsky · 2024-01-12T07:43:51Z

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Attributes.cs

@@ -327,6 +328,8 @@ public static class GetCustomAttribute
    {

        [Fact]
+        [ActiveIssue("https://github.com/dotnet/linker/issues/2078", typeof(PlatformDetection), nameof(PlatformDetection.IsTrimmedWithILLink))


Wondered why this wasn't hit with naot and found out illinker has this as a hardcoded list. Weird, but okay I guess. Would be nice to move this to the same plan as all the other attributes at least. I'm not convinced anyone will want to work on 2078 anytime soon.

Filed #96907

MichalStrehovsky · 2024-01-12T07:46:32Z

src/tools/illink/src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

@@ -1583,7 +1583,7 @@ public bool Analyze (in CalleePayload callee, Stack<MethodDefinition> callStack)
 								return false;

 							if (operand is int oint) {
-								if (oint == 1)
+								if (oint != 0)


Why was this needed? It's a good change, but surprised to see this in this PR.

Maybe I should've separated it out, but I thought this change was small enough to include - allowing branch removal caused this test to fail, due to this bug. More detail in the commit message for aadf946.

Changes TrimmerRootAssembly (and the underlying option -a) to root all members of the assembly, without setting the action to copy. This allows us to perform branch removal and attribute removal for the assembly. This includes changes to MarkStep to ensure that rooted assemblies are marked visible to reflection. This for example ensures that parameter names don't get removed. With the change to MarkEntireType that calls MarkType instead of Annotations.Mark, we are now reaching MarkType for the dependency from the copy assembly to the attribute it contains, producing an unexpected IL2045. Silence this by checking the DependencyKind. Also fixes a bug in constant method analysis that this uncovered: BitVector32.CreateMask(int.MinValue) was incorrectly being treated as returning constant 1. The IL from the beginning of that method: IL_0000: ldarg.0 IL_0001: brtrue.s IL_0005 IL_0003: ldc.i4.1 IL_0004: ret The brtrue.s branch was being analyzed as not taken because the argument was not 1. The branch should be analyzed as taken for any non-zero argument.

Avoid copy action for root assemblies

bff0b2c

dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Dec 30, 2023

ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Dec 30, 2023

ghost assigned sbomer Dec 30, 2023

build-analysis bot mentioned this pull request Dec 30, 2023

System.Text.Json failing some large file tests #59678

Closed

sbomer added 3 commits January 2, 2024 19:55

Ensure attributes get marked too

9e4804b

Preserve existing marking behavior by letting MarkStep do the same work that it does for copy assemblies, including marking attributes.

Fix wasm test failures

d20cd80

Fix ApiCompat errors

e281fd0

This was referenced Jan 3, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Tracking issue for "WORKLOAD TIMED OUT" #90309

Closed

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

sbomer added 3 commits January 4, 2024 00:06

Fix issue, suppress for trimming but not AOT

411ca2d

Mark root assemblies visible to reflection

1c948d4

Fix unexpected IL2045

e5dbcb2

With the change to MarkEntireType that calls MarkType instead of Annotations.Mark, we are now reaching MarkType for the dependency from the copy assembly to the attribute it contains, producing an unexpected IL2045. Silence this by checking the DependencyKind.

This was referenced Jan 4, 2024

[wasm][debugger] Debugger tests getting randomly aborted on CI #89409

Open

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

sbomer added 2 commits January 4, 2024 18:30

Disable another test

c9621a2

Merge remote-tracking branch 'origin/main' into avoidCopyForRootAssembly

b356586

This was referenced Jan 4, 2024

System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783

Open

[wasm] Failing test System.Runtime.InteropServices.JavaScript.Tests.WebWorkerTest.ManagedDelay_ContinueWith #96493

Closed

Preserve DebuggableAttribute for ApplyUpdateTest

5df751b

This comment was marked as resolved.

Sign in to view

Set DebuggerSupport for System.Runtime.Tests

93942b8

build-analysis bot mentioned this pull request Jan 5, 2024

MSBuild crashing in the build #92290

Open

sbomer added 3 commits January 5, 2024 18:42

Add testcase for root behaviors

f7dfe0b

And don't set ForceParse for rooted methods. ForceParse was preventing the XML substitutions from applying. This change may make it possible for substitutions in copy assemblies to be honored...

Check warning behavior

61ff65e

Preserve substitution behavior for 'copy' action

d447447

Use LogDoesNotContain instead

4c2ef7f

FIxing the checkRemainingErrors behavior results in too many spurious failures. Use LogDoesNotContainAttribute for now.

build-analysis bot mentioned this pull request Jan 8, 2024

System.Net.Http.Functional.Tests.HttpMetricsTest_Http11_Async_HttpMessageInvoker Test failure #96407

Closed

sbomer added 4 commits January 8, 2024 23:17

Ignore testcase for nativeaot

51d9146

Preserve EnC assemblies for System.Runtime.Loader.Tests

b833a19

Add KeptAttributeAttribute

944bbeb

Merge remote-tracking branch 'origin/main' into avoidCopyForRootAssembly

2a072e8

sbomer marked this pull request as ready for review January 9, 2024 17:40

sbomer requested a review from marek-safar as a code owner January 9, 2024 17:40

sbomer requested review from vitek-karas and MichalStrehovsky January 9, 2024 17:41

MichalStrehovsky reviewed Jan 10, 2024

View reviewed changes

src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj Outdated Show resolved Hide resolved

Remove unnecessary DebuggerSupport setting

150a67d

MichalStrehovsky approved these changes Jan 12, 2024

View reviewed changes

sbomer merged commit d92db39 into dotnet:main Jan 12, 2024
133 of 135 checks passed

github-actions bot locked and limited conversation to collaborators Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid copy action for root assemblies #96363

Avoid copy action for root assemblies #96363

sbomer commented Dec 30, 2023 •

edited

Loading

ghost commented Dec 30, 2023

This comment was marked as resolved.

lambdageek commented Jan 8, 2024 •

edited

Loading

MichalStrehovsky left a comment

MichalStrehovsky Jan 12, 2024

sbomer Jan 12, 2024

MichalStrehovsky Jan 12, 2024

sbomer Jan 12, 2024

Avoid copy action for root assemblies #96363

Avoid copy action for root assemblies #96363

Conversation

sbomer commented Dec 30, 2023 • edited Loading

ghost commented Dec 30, 2023

This comment was marked as resolved.

lambdageek commented Jan 8, 2024 • edited Loading

MichalStrehovsky left a comment

Choose a reason for hiding this comment

MichalStrehovsky Jan 12, 2024

Choose a reason for hiding this comment

sbomer Jan 12, 2024

Choose a reason for hiding this comment

MichalStrehovsky Jan 12, 2024

Choose a reason for hiding this comment

sbomer Jan 12, 2024

Choose a reason for hiding this comment

sbomer commented Dec 30, 2023 •

edited

Loading

lambdageek commented Jan 8, 2024 •

edited

Loading