[wasm] Fix trimming errors for library tests, to help building with AOT #50885

radical · 2021-04-07T21:06:44Z

No description provided.

ghost · 2021-04-07T21:06:50Z

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	radical
Assignees:	-
Labels:	`area-Infrastructure-libraries`
Milestone:	-

eerhardt · 2021-04-13T19:51:46Z

eng/testing/ildescriptors/ILLink.Descriptor.Castle.xml

@@ -0,0 +1,13 @@
+<linker>


All these files should have Descriptors in the name following the naming pattern set by the linker.

See the docs here:
https://github.com/mono/linker/blob/55bd0ebd00ddbf2cd817314b1cf6824003cfb463/docs/data-formats.md#descriptor-format

eerhardt · 2021-04-13T19:52:24Z

eng/testing/ildescriptors/ILLink.Descriptor.Serialization.xml

@@ -0,0 +1,10 @@
+<linker>
+  <assembly fullname="System.Private.DataContractSerialization">
+    <type fullname="System.Runtime.Serialization.KeyValue`2" />


cc @joperezr - is your change going to make it so this is no longer necessary?

In general, I don't think we should be adding ILLink.Descriptors files for our own assemblies. Instead we should be fixing our assemblies to be trimmable.

eerhardt · 2021-04-13T19:56:59Z

eng/testing/ildescriptors/ILLink.Descriptor.Serialization.xml

+    <type fullname="System.Runtime.Serialization.KeyValuePairAdapter`2" />
+  </assembly>
+  <assembly fullname="System.Private.Xml.Linq">
+    <type fullname="System.Xml.Linq.XElement" />


Why is this necessary? It shouldn't be.

eerhardt · 2021-04-13T19:58:00Z

src/libraries/System.Runtime.Serialization.Xml/tests/ILLink.Descriptor.xml

+<linker>
+  <assembly fullname="System.Data.Common">
+    <type fullname="System.Data.DataColumn">
+      <method signature="System.Void .ctor()" />


cc @krwq - is your change going to fix this?

eerhardt · 2021-04-13T19:58:48Z

src/libraries/System.Reflection/tests/ILLink.Descriptor.xml

@@ -0,0 +1,7 @@
+<linker>
+  <assembly fullname="System.Private.CoreLib">
+      <type fullname="System.Collections.ArrayList" />


Why is this necessary? It shouldn't be.

I was seeing this as:

02:19:20] fail: [FAIL] System.Reflection.Tests.TypeInfoTests.IsAssignable(type: typeof(System.Collections.IList), c: typeof(System.Collections.ArrayList), expected: True) [02:19:20] info: Assert.Equal() Failure [02:19:20] info: Expected: True [02:19:20] info: Actual: False [02:19:20] info: at System.Reflection.Tests.TypeInfoTests.IsAssignable(Type type, Type c, Boolean expected) [02:19:20] info: at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

radical@3cdf08a

This PR is an attempt to remove the trimming errors, to enable AOT tests to be built. I didn't delve into why the linker wasn't being able to pick up the usage. We could look into that, when I make a clean PR based on main.

eerhardt · 2021-04-13T20:00:16Z

src/libraries/Microsoft.Extensions.Logging/tests/Common/ILLink.Descriptor.xml

+<linker>
+  <assembly fullname="System.Diagnostics.TraceSource">
+    <type fullname="System.Diagnostics.TraceListener">
+        <method signature="System.Void .ctor()" />


Do you know why this is necessary? It shouldn't be.

….Tests

marek-safar · 2021-04-22T07:32:57Z

src/libraries/Microsoft.Extensions.Logging/tests/Common/ILLink.Descriptors.xml

@@ -0,0 +1,7 @@
+<linker>
+  <assembly fullname="System.Diagnostics.TraceSource">


This one is a bit unexpected, do you know where is it used?

Agreed - this shouldn't be necessary because all the ILLink warnings are fixed in this assembly. This might be a product bug somewhere.

eerhardt · 2021-04-22T14:37:17Z

src/libraries/System.Reflection.MetadataLoadContext/tests/ILLink.Descriptors.xml

@@ -0,0 +1,3 @@
+<linker>
+  <assembly fullname="mscorlib" />


Is this still necessary with the latest linker? We had a few fixes that went in to preserve facade assemblies.

eerhardt · 2021-04-22T14:38:15Z

src/libraries/System.Reflection/tests/CoreCLR/System.Reflection.CoreCLR.Tests.csproj

@@ -14,5 +14,7 @@
  </ItemGroup>
  <ItemGroup>
    <ProjectReference Include="..\TestExe\System.Reflection.TestExe.csproj" />
+
+    <TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)..\ILLink.Descriptors.CoreCLR.xml" />


Can this ILLink.Descriptors.CoreCLR.xml file move next to the .csproj that needs it?

eerhardt · 2021-04-22T14:40:09Z

eng/testing/tests.props

@@ -6,6 +6,7 @@
    <TestDependsOn Condition="'$(TestNoBuild)' != 'true' and '$(BuildAllProjects)' != 'true'">Build</TestDependsOn>
    <TestDependsOn>$(TestDependsOn);GenerateRunScript;RunTests</TestDependsOn>
    <VSTestNoLogo>true</VSTestNoLogo>
+    <ILDescriptorsPath>$(MSBuildThisFileDirectory)ildescriptors\</ILDescriptorsPath>


This needs a better name. How about?

<ILLinkDescriptorsPath>$(MSBuildThisFileDirectory)ILLinkDescriptors\</ILLinkDescriptorsPath>

eerhardt · 2021-04-22T14:40:39Z

This file should go in the ILLinkDescriptors folder.

Refers to: eng/testing/ILLink.Descriptors.TestUtilities.xml:1 in e8cb433. [](commit_id = e8cb433, deletion_comment = False)

eerhardt · 2021-04-22T14:47:38Z

eng/testing/ildescriptors/ILLink.Descriptors.Serialization.xml

@@ -0,0 +1,10 @@
+<linker>
+  <assembly fullname="System.Private.DataContractSerialization">
+    <type fullname="System.Runtime.Serialization.KeyValue`2" />


Are these still necessary on the latest main? @joperezr addressed the ILLink warnings in this assembly, so these might be fixed.

If they aren't fixed, we should open a bug to make sure we don't have a product bug here.

DCS is annotated now but most of it was annotated as unsafe. It would be good to understand who actually needs this so that we can check why is the type not being preserved and to find out if it is expected or not

eerhardt · 2021-04-22T14:48:30Z

src/libraries/System.Reflection/tests/ILLink.Descriptors.xml

@@ -0,0 +1,7 @@
+<linker>
+  <assembly fullname="System.Private.CoreLib">
+      <type fullname="System.Collections.ArrayList" />


This is suprising. Why is this necessary?

eerhardt · 2021-04-22T14:49:22Z

src/libraries/System.Runtime.Serialization.Xml/tests/ILLink.Descriptors.xml

+<linker>
+  <assembly fullname="System.Data.Common">
+    <type fullname="System.Data.DataColumn">
+      <method signature="System.Void .ctor()" />


fyi @krwq again here

radical · 2021-04-22T15:59:44Z

@eerhardt This PR was based on another big PR which got merged last night. And I had been working on this over the last few weeks. So, it's definitely possible that some of the changes aren't needed anymore, as you suggested.

I'm going to enable all the tests on separate temporary PR, and then apply the fixes that are still useful, and we can have the current error messages etc. Then I can reset the history here.

marek-safar · 2021-04-23T09:36:20Z

I think it'd be better to enable trimming analyzer warnings for this than trying to guess where stuff can break. We could exclude the 3rd party libs to make it less noisy.

ghost · 2021-04-23T09:40:40Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	radical
Assignees:	-
Labels:	`arch-wasm`, `linkable-framework`
Milestone:	-

eerhardt · 2021-04-23T15:58:08Z

be better to enable trimming analyzer warnings

For our test assemblies? I'm unsure if that would be worth the effort.

marek-safar · 2021-04-23T16:38:56Z

I'm unsure if that would be worth the effort.

Why? Isn't that what this and probably more upcoming PRs will do?

eerhardt · 2021-04-23T16:47:38Z

Addressing ILLink warnings isn't trivial. It takes quite a bit of effort.
The maintenance cost of keeping ILLink warnings out of our tests would be an on-going tax for developers.
It doesn't completely solve the problem, because of 3rd party libraries, as noted above.

marek-safar · 2021-04-23T16:56:38Z

The maintenance cost of keeping ILLink warnings out of our tests would be an on-going tax for developers.

Once this PR goes it that will be the case, every PR touching libraries will have to ensure the tests are not failing with the linker running for the browser's test

radical · 2021-04-23T17:58:08Z

Once this PR goes it that will be the case, every PR touching libraries will have to ensure the tests are not failing with the linker running for the browser's test

It's already enabled on main, see https://dev.azure.com/dnceng/public/_build/results?buildId=1104194&view=logs&j=fddee5b2-3265-5a91-40c1-750d81e69f3f .

…test

radical · 2021-04-26T22:40:09Z

I have re-tested, and applied all the relevant changes with clean commits in #51697 . Some changes weren't needed with the latest main.

Closing this in favor of that PR.

dotnet-issue-labeler bot added the area-Infrastructure-libraries label Apr 7, 2021

radical force-pushed the build-wasm-aot-helix-test branch from d01b28b to 8b18e5c Compare April 7, 2021 21:07

This was referenced Apr 8, 2021

[tests] System.Text.Json.Tests segfault, for Libraries Test Run release coreclr OSX x64 Release #47805

Closed

System.Text.Json.Tests failing on browser #50433

Closed

Helix DNS failures causing work item failures in CI #50750

Closed

radical force-pushed the build-wasm-aot-helix-test branch 5 times, most recently from f011bfb to 837548a Compare April 11, 2021 01:15

radical mentioned this pull request Apr 11, 2021

[wasm][tests][eat] Multiple test failures with EnableAggressiveTrimming #50724

Closed

7 tasks

radical force-pushed the build-wasm-aot-helix-test branch 2 times, most recently from 46e4653 to 3b817b1 Compare April 12, 2021 23:34

eerhardt reviewed Apr 13, 2021

View reviewed changes

radical force-pushed the build-wasm-aot-helix-test branch 3 times, most recently from 868d149 to 4ab2f77 Compare April 14, 2021 04:18

This was referenced Apr 14, 2021

[wasm][tests] Tracking issue for errors with EnableAggressiveTrimming, for building AOT tests #51249

Open

[wasm][tests] Tracking issue for test failures with AOT #51251

Closed

radical force-pushed the build-wasm-aot-helix-test branch from 4ab2f77 to a359117 Compare April 14, 2021 18:42

runfoapp bot mentioned this pull request Apr 15, 2021

Exception in test infrastructure - System.InvalidOperationException: Collection was modified after the enumerator was instantiated #11063

Closed

radical force-pushed the build-wasm-aot-helix-test branch 2 times, most recently from d0e003a to 10a6ba1 Compare April 16, 2021 00:19

radical added 2 commits April 22, 2021 01:04

[wasm] Disable failing test in System.Xml.Xsl.XslCompiledTransformApi…

875c07a

….Tests

Re-enable System.Xml.Xsl.XslCompiledTransformApi.Tests

e8cb433

radical mentioned this pull request Apr 22, 2021

[wasm][aot] RuntimeError: memory access out of bounds in System.Xml.XmlSchemaSet.Test #51040

Closed

marek-safar reviewed Apr 22, 2021

View reviewed changes

eerhardt reviewed Apr 22, 2021

View reviewed changes

marek-safar added linkable-framework Issues associated with delivering a linker friendly framework and removed area-Build-mono area-Infrastructure-libraries trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT labels Apr 23, 2021

Merge remote-tracking branch 'origin/main' into build-wasm-aot-helix-…

404ed1f

…test

radical closed this Apr 26, 2021

radical deleted the build-wasm-aot-helix-test branch April 26, 2021 22:40

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] Fix trimming errors for library tests, to help building with AOT #50885

[wasm] Fix trimming errors for library tests, to help building with AOT #50885

radical commented Apr 7, 2021

ghost commented Apr 7, 2021

eerhardt Apr 13, 2021

eerhardt Apr 13, 2021

eerhardt Apr 13, 2021

eerhardt Apr 13, 2021

eerhardt Apr 13, 2021

eerhardt Apr 13, 2021

radical Apr 14, 2021

eerhardt Apr 13, 2021

marek-safar Apr 22, 2021

eerhardt Apr 22, 2021

eerhardt Apr 22, 2021

eerhardt Apr 22, 2021

eerhardt Apr 22, 2021

eerhardt commented Apr 22, 2021

eerhardt Apr 22, 2021

joperezr Apr 23, 2021

eerhardt Apr 22, 2021

eerhardt Apr 22, 2021

radical commented Apr 22, 2021

marek-safar commented Apr 23, 2021

ghost commented Apr 23, 2021

eerhardt commented Apr 23, 2021

marek-safar commented Apr 23, 2021

eerhardt commented Apr 23, 2021

marek-safar commented Apr 23, 2021 •

edited

Loading

radical commented Apr 23, 2021

radical commented Apr 26, 2021

		@@ -0,0 +1,7 @@
		<linker>
		<assembly fullname="System.Diagnostics.TraceSource">

[wasm] Fix trimming errors for library tests, to help building with AOT #50885

[wasm] Fix trimming errors for library tests, to help building with AOT #50885

Conversation

radical commented Apr 7, 2021

ghost commented Apr 7, 2021

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

eerhardt commented Apr 22, 2021

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

radical commented Apr 22, 2021

marek-safar commented Apr 23, 2021

ghost commented Apr 23, 2021

eerhardt commented Apr 23, 2021

marek-safar commented Apr 23, 2021

eerhardt commented Apr 23, 2021

marek-safar commented Apr 23, 2021 • edited Loading

radical commented Apr 23, 2021

radical commented Apr 26, 2021

marek-safar commented Apr 23, 2021 •

edited

Loading