Do not suppress trim analysis warnings when NativeAOT is enabled #20767

simonrozsival · 2024-06-24T09:36:15Z

We noticed we weren't seeing trim analysis warnings in VS Code when PublishAot was set to true. There was a recent change that correctly disabled the suppressions when TrimMode is full. We need to make sure that we're also getting the trim analysis warnings in dotnet build with PublishAot but suppress them when publishing (in that case the warnings will come later from ILC). This PR aligns the behavior of PublishAot=true and TrimMode=true in debug builds.

/cc @ivanpovazan @rolfbjarne @jonathanpeppers

simonrozsival · 2024-06-24T09:45:25Z

@rolfbjarne do you think this change (if approved) should go into main or the net9.0 branch?

simonrozsival · 2024-06-24T12:44:20Z

/azp run

azure-pipelines · 2024-06-24T12:44:47Z

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s).

mandel-macaque · 2024-06-24T15:58:16Z

/azp run

mandel-macaque · 2024-06-24T15:58:40Z

Had to re-trigger, something funny was going on with VSTS.

azure-pipelines · 2024-06-24T15:58:45Z

Azure Pipelines successfully started running 2 pipeline(s), but failed to run 1 pipeline(s).

rolfbjarne · 2024-06-25T13:52:31Z

dotnet/targets/Xamarin.Shared.Sdk.props

@@ -150,13 +150,13 @@
 	</PropertyGroup>
 	<PropertyGroup Condition="'$(TrimMode)' == ''">
 		<!-- Linking is always on for all assemblies when using NativeAOT - this is because we need to modify all assemblies in the linker for them to be compatible with NativeAOT -->
-		<_DefaultLinkMode Condition="'$(_UseNativeAot)' == 'true'">Full</_DefaultLinkMode>
+		<_DefaultLinkMode Condition="'$(PublishAot)' == 'true'">Full</_DefaultLinkMode>


This will slow down debug/simulator builds significantly.

How does this work in desktop/console apps? Will setting PublishAot=true enable the trimmer for debug builds as well?

We wanted to align projects with <PublishAot>true</PublishAot> with projects that have <TrimMode>full</TrimMode>. Now that you pointed this out, I'm realizing that we're telling devs to only enable TrimMode=full in Release builds and not in Debug builds and this change is incorrect.

The goal of this PR is to make sure we don't suppress trimming warnings in Debug builds so I'll make sure to achieve that without changing the trim mode.

/cc @ivanpovazan

How does this work in desktop/console apps? Will setting PublishAot=true enable the trimmer for debug builds as well?

Yes, when we set PublishAot=true debug builds will enable trim analyzers:

ivanpovazan@EONE-MAC-ARM64 ~/tmp/abcd $ dotnet build -c Debug -p:PublishAot=true Restore complete (0.7s) You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy abcd succeeded with 1 warning(s) (0.4s) → bin/Debug/net9.0/abcd.dll /Users/ivan/tmp/abcd/Program.cs(9,21): warning IL2075: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperties()'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. Build succeeded with 1 warning(s) in 1.3s Workload updates are available. Run `dotnet workload list` for more information. ivanpovazan@EONE-MAC-ARM64 ~/tmp/abcd $ rm -rf bin obj ivanpovazan@EONE-MAC-ARM64 ~/tmp/abcd $ dotnet build -c Debug -p:PublishAot=false Restore complete (0.5s) You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy abcd succeeded (0.4s) → bin/Debug/net9.0/abcd.dll Build succeeded in 1.1s

ivanpovazan · 2024-07-03T06:35:53Z

dotnet/targets/Xamarin.Shared.Sdk.targets

+				In the case of NativeAOT builds, the trim warnings will be produced either by the ILLink roslyn analyzer in Debug builds
+				or by the ILC when publishing. We want to suppress the ILLink warnings in all build configurations to avoid duplicates.
+			-->
+			<SuppressTrimAnalysisWarnings Condition="'$(PublishAot)' == 'true'">true</SuppressTrimAnalysisWarnings>


I think this should be:

Suggested change

<SuppressTrimAnalysisWarnings Condition="'$(PublishAot)' == 'true'">true</SuppressTrimAnalysisWarnings>

<SuppressTrimAnalysisWarnings Condition="'$(PublishAot)' == 'true' And '$(_IsPublishing)' != 'true'">true</SuppressTrimAnalysisWarnings>

As we could have a project file with both '$(TrimMode)' == 'full' and '$(PublishAot)' == 'true'.
If that is the case and we do dotnet build, ILC will not run, so we want warnings from ILLink.
When both PublishAot and _IsPublishing are true aka _UseNativeAot=true we already suppress the warnings in props, so we got that covered.

FWIW, to avoid confusion, I think we should rename _UseNativeAot to something like _PublishWithNativeAot or something like that.

Changing this condition won't make any difference, but it might make it a bit more confusing and harder to follow the values of SuppressTrimAnalysisWarnings. "Why shouldn't this be set to true when publishing?" I don't have a strong preference but I would lean towards keeping the condition simple.

I agree that _UseNativeAot is a bit confusing. It's a combination of PublishAot and _IsPublishing so I suggest _IsPublishingUsingNativeAot or _IsPublishingWithNativeAot.

Changing this condition won't make any difference

Yes, you are right, my suggestion was not correct, and it should include check on TrimMode as well.

With the current state of the condition, users who have their project files configured like:

<TrimMode>full</TrimMode> <PublishAot>true</PublishAot>

and do dotnet build will not see any warnings:

from ILLink, because we suppress warnings here based on just PublishAot

from ILC, because they are running build instead of publish

tests/dotnet/UnitTests/ProjectTest.cs

rolfbjarne · 2024-07-03T08:03:44Z

dotnet/targets/Xamarin.Shared.Sdk.targets

 			<!-- Suppress trimmer warnings unless we're trimming all assemblies -->
 			<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == '' And '$(TrimMode)' == 'full'">false</SuppressTrimAnalysisWarnings>
 			<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">true</SuppressTrimAnalysisWarnings>


Yes, it looks like all the SuppressTrimAnalysisWarnings property setters here are no-ops, because SuppressTrimAnalysisWarnings shouldn't be empty at this point.

Then below, add the _ExtraTrimmerArgs like this:

<_ExtraTrimmerArgs Condition="'$(SuppressTrimAnalysisWarnings)' == 'true' Or '$(_UseNativeAot)' == 'true'">$(_ExtraTrimmerArgs) --notrimwarn</_ExtraTrimmerArgs>

This way we'll:

Suppress ILLink's trimmer warnings when using NativeAOT

Honor SuppressTrimAnalysisWarnings otherwise.

rolfbjarne · 2024-07-03T08:10:15Z

Btw the test now fails with:

Xamarin.Tests.DotNetProjectTest.BuildMyNativeAotAppWithTrimAnalysisWarning(iOS,"iossimulator-arm64"): Warning count
Expected: 1
But was: 2

simonrozsival · 2024-07-03T12:26:53Z

@rolfbjarne with the changes in the last comment, the new tests are now passing (finally). There are just failing Windows tests but I saw this on other PRs and the error message hints that this is an infra issue and not related to this PR:

error VSX1002: The .NET runtime 8.0.1 or newer could not be found in the remote host under '/Users/builder/Library/Caches/Xamarin/XMA/SDKs/dotnet/dotnet'. Please install it or try setting a different path using the MSBuild property "DotNetRuntimePath".

ivanpovazan · 2024-07-03T13:53:46Z

dotnet/targets/Xamarin.Shared.Sdk.targets

+			<!--
+				In the case of NativeAOT builds, the trim warnings will be produced either by the ILLink roslyn analyzer in Debug builds
+				or by the ILC when publishing. We want to suppress the ILLink warnings in all build configurations to avoid duplicates.
+				We choose not to suppress the ILLink warnings when TrimMode is set to 'full' and we're not publishing. In this scenario
+				ILC will not run at a later stage of the build.
+			-->
+			<SuppressTrimAnalysisWarnings Condition="'$(PublishAot)' == 'true' And '$(_IsPublishing)' != 'true' And '$(TrimMode)' != 'full'">true</SuppressTrimAnalysisWarnings>


@jonathanpeppers regarding your changes for Android:

We will need to add something like this in: https://github.com/dotnet/android/blob/77f5fdd1d32b6ff2a2b36008f966cb4abffb8231/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets#L12

and additionally the changes from lines 659-661:
https://github.com/xamarin/xamarin-macios/pull/20767/files#diff-2e24f1b6f44df7580244d00f106fa1a9e2257536fbca9d7bbdc74571549324a9R659-R661

the only difference would be that we would exchange PublishAot with IsAotCompatible

vs-mobiletools-engineering-service2 · 2024-07-03T14:31:17Z

📚 [CI Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-07-03T14:43:52Z

💻 [CI Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: 190e0b53394ab28a4884408de42d358b2a9c3c92 [PR build]

vs-mobiletools-engineering-service2 · 2024-07-03T14:48:16Z

❌ [CI Build] Windows Integration Tests failed ❌

❌ Failed ❌

Pipeline on Agent
Hash: 190e0b53394ab28a4884408de42d358b2a9c3c92 [PR build]

vs-mobiletools-engineering-service2 · 2024-07-03T14:48:17Z

💻 [CI Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: 190e0b53394ab28a4884408de42d358b2a9c3c92 [PR build]

vs-mobiletools-engineering-service2 · 2024-07-03T14:49:39Z

💻 [CI Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: 190e0b53394ab28a4884408de42d358b2a9c3c92 [PR build]

vs-mobiletools-engineering-service2 · 2024-07-03T16:17:07Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 170 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 6 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ install-source: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 7 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac-binding-project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 6 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (watchOS): All 4 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 190e0b53394ab28a4884408de42d358b2a9c3c92 [PR build]

rolfbjarne · 2024-07-03T16:22:21Z

The windows test failure is a known issue.

simonrozsival requested a review from rolfbjarne as a code owner June 24, 2024 09:36

simonrozsival mentioned this pull request Jun 24, 2024

Do not suppress trim analysis warnings when NativeAOT is enabled #20763

Closed

simonrozsival force-pushed the dev/simonrozsival/do-not-suppress-trim-analysis-warnings-when-nativeaot-is-enabled branch from f220e23 to d44d2c4 Compare June 24, 2024 09:43

simonrozsival changed the base branch from net9.0 to main June 24, 2024 09:43