Fix default link mode for NativeAOT #18530

filipnavara · 2023-07-01T14:25:14Z

No description provided.

dalexsoto · 2023-07-02T20:57:22Z

/azp run

azure-pipelines · 2023-07-02T20:57:45Z

Azure Pipelines successfully started running 1 pipeline(s).

vs-mobiletools-engineering-service2 · 2023-07-02T22:02:19Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: f49d290a0139837c4d91121000c3d680f9420021 [PR build]

vs-mobiletools-engineering-service2 · 2023-07-02T22:05:45Z

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-07-02T22:07:03Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-07-02T22:07:50Z

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

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

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-07-02T22:26:55Z

✅ API diff for current PR / commit

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: f49d290a0139837c4d91121000c3d680f9420021 [PR build]

vs-mobiletools-engineering-service2 · 2023-07-03T00:18:32Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 92 tests passed 🎉

Tests counts

⚠️ bcl: No tests selected. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ framework: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 4 tests passed. Html Report (VSDrops) Download
⚠️ install_source: No tests selected. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 40 tests passed. Html Report (VSDrops) Download
⚠️ mac_binding_project: No tests selected. Html Report (VSDrops) Download
⚠️ mmp: No tests selected. Html Report (VSDrops) Download
⚠️ mononative: No tests selected. Html Report (VSDrops) Download
✅ monotouch: All 26 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
⚠️ mtouch: No tests selected. Html Report (VSDrops) Download
⚠️ xammac: No tests selected. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: f49d290a0139837c4d91121000c3d680f9420021 [PR build]

ivanpovazan · 2023-07-03T09:04:26Z

dotnet/targets/Xamarin.Shared.Sdk.targets

@@ -1583,7 +1583,7 @@
 			<_DefaultLinkMode>TrimMode</_DefaultLinkMode>
 		</PropertyGroup>
 		<PropertyGroup Condition="'$(TrimMode)' == ''">
-			<_DefaultLinkMode Condition="'$(_UseNativeAot)' != 'true'">Full</_DefaultLinkMode> <!-- 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> <!-- 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 -->


I agree that the comment does not match how _DefaultLinkMode is set, but I think there is more to this and that the introduced change only works because we explicitly pass:

xamarin-macios/dotnet/targets/Xamarin.Shared.Sdk.targets

Line 1215 in 7ed8c08

<IlcArg Include="--defaultrooting" />

Without this fix _DefaultLinkMode is not set. Then you get to the _ComputeLinkMode target here:

xamarin-macios/msbuild/Xamarin.Shared/Xamarin.Shared.targets

Lines 2263 to 2271 in 05502d1

<Target Name="_ComputeLinkMode" DependsOnTargets="$(_ComputeLinkModeDependsOn)">

<PropertyGroup>

<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' == 'macOS'">$(LinkMode)</_LinkMode>

<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' != 'macOS'">$(MtouchLink)</_LinkMode>

<_LinkMode Condition="'$(_LinkMode)' == ''">$(_DefaultLinkMode)</_LinkMode> 

<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' == 'macOS'">None</_LinkMode> 

<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' != 'macOS'">SdkOnly</_LinkMode> 

</PropertyGroup>

</Target>

On iOS-like platform you end up in the SdkOnly code path which translates to partial TrimMode iirc and somehow ends up doing mostly the right thing. On macOS, however, you get the None mode in the fallback. This results in copy TrimMode which does no linking at all and then passes all the assemblies as --root:<assembly name> to ILCompiler. That in turns results in compilation of the whole world, and in Debug version of ILC it asserts on trying to compile Array<T>. That's definitely wrong.

This is all irrespective of --defaultrooting which is already filed as issue #18482.

What I meant is (considering just iOS):

if _DefaultLinkMode=Full -> _LinkMode will become Full

if _LinkMode=Full -> TrimMode will become full

xamarin-macios/dotnet/targets/Xamarin.Shared.Sdk.targets

Line 522 in 7ed8c08

<TrimMode Condition="'$(_LinkMode)' == 'Full'">full</TrimMode>

if TrimMode=full -> ILC will not use --defaultrooting
https://github.com/dotnet/runtime/blob/f25bc7b5b859935192e4eddc5b71efe71a60a4de/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L270

If --defaultrooting is not passed to ILC -> ILC will start trimming (which will break the app)

So I just wanted to draw the attention that the above change only works in the current setup, as we are explicitly passing --defaultrooting to ILC

Additionally, this means that for iOS/tvOS/watchOS (in case of NativeAOT) the default linking mode becomes Full which is not how ILLink is configured in the noNativeAOT mode for these platforms: https://github.com/xamarin/xamarin-macios/pull/18530/files#diff-2e24f1b6f44df7580244d00f106fa1a9e2257536fbca9d7bbdc74571549324a9R1591

This could start breaking the apps with NativeAOT that work normally in noNativeAOT mode, as the change enables ILLink to trim more than what is the current default mode.

So, what do you propose the default to be?

I just think that we should not change ILLink default trimming modes just because NativeAOT compilation later fails.

Also, just in case it's not clear, this is currently changing the default ONLY when PublishAot=true AND you are running dotnet publish...

Maybe the right fix would be to "intercept" the build and set the right properties for NativeAOT, after ILLink has finished its work and before ILC compilation starts.

... so the end effect is not really any different than this. It doesn't matter whether ILLink trims it first, or NativeAOT trims it later. In the end you still need the trimming to take place.

Also, just in case it's not clear, this is currently changing the default ONLY when PublishAot=true AND you are running dotnet publish...

Thanks, I did understand this exactly as you explained, but again I just want to emphasise that with this change we will start trimming the user code which was not marked with IsTrimmable=true (at least in case of iOS-like platforms) and the reasons for potential regressions that this could bring will not be necessarily related to the fact that we are using NativeAOT as a deployment toolchain-runtime, but rather to the fact that we started trimming something that was never trimmed before - meaning it is not something NativeAOT trimmer/compiler brought in, but how we configured the whole toolchain

I don't have a problem setting a different default as long as some default is set and it's not None (which is the underlying problem for me).

I, however, feel that defaulting to partial (current default for iOS-like platforms without this PR) is weird when .NET 7+ default for other platforms is full and it's documented that you can change it, and how.

That said, either partial (SdkOnly) or full would work to solve the macOS issue.

I, however, feel that defaulting to partial (current default for iOS-like platforms without this PR) is weird when .NET 7+ default for other platforms is full and it's documented that you can change it, and how.

That is something that should be checked with @rolfbjarne

As for the NativeAOT story regarding macOS, I believe something like:

<_DefaultLinkMode Condition="'$(_UseNativeAot)' == 'true' And '$(_PlatformName)' == 'macOS'">Full</_DefaultLinkMode> <_DefaultLinkMode Condition="'$(_UseNativeAot)' == 'true' And '$(_PlatformName)' != 'macOS'">SdkOnly</_DefaultLinkMode>

and

<IlcArg Condition="''$(_PlatformName)' != 'macOS'" Include="--defaultrooting" />

should be tested, as in the current set up NativeAOT trimming is disabled, due to:

ILLink removing IsTrimmable attributes from assemblies

NativeAOT operating in --defaultrooting mode

Alternative to #18530, see #18530 (comment) Contributes to #18482

filipnavara · 2023-07-20T15:12:52Z

Superceded by #18560

Fix default link mode for NativeAOT

f49d290

This was referenced Jul 1, 2023

Add support for NativeAOT on macOS #18524

Closed

NativeAOT: Asking for Array<T> MethodTable dotnet/runtime#88287

Closed

ivanpovazan reviewed Jul 3, 2023

View reviewed changes

mandel-macaque approved these changes Jul 7, 2023

View reviewed changes

filipnavara mentioned this pull request Jul 17, 2023

Fix _DefaultLinkMode for NativeAOT. #18560

Merged

dalexsoto pushed a commit that referenced this pull request Jul 20, 2023

Fix _DefaultLinkMode for NativeAOT. (#18560)

d635e6d

Alternative to #18530, see #18530 (comment) Contributes to #18482

filipnavara closed this Jul 20, 2023

filipnavara deleted the patch-10 branch July 20, 2023 15:13

rolfbjarne added the community Community contribution ❤ label Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default link mode for NativeAOT #18530

Fix default link mode for NativeAOT #18530

filipnavara commented Jul 1, 2023

dalexsoto commented Jul 2, 2023

azure-pipelines bot commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 3, 2023

ivanpovazan Jul 3, 2023

filipnavara Jul 3, 2023 •

edited

Loading

ivanpovazan Jul 3, 2023

ivanpovazan Jul 10, 2023

filipnavara Jul 10, 2023

filipnavara Jul 10, 2023

ivanpovazan Jul 10, 2023 •

edited

Loading

filipnavara Jul 10, 2023 •

edited

Loading

filipnavara Jul 10, 2023 •

edited

Loading

ivanpovazan Jul 10, 2023

filipnavara commented Jul 20, 2023

	<Target Name="_ComputeLinkMode" DependsOnTargets="$(_ComputeLinkModeDependsOn)">
	<PropertyGroup>
	<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' == 'macOS'">$(LinkMode)</_LinkMode>
	<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' != 'macOS'">$(MtouchLink)</_LinkMode>
	<_LinkMode Condition="'$(_LinkMode)' == ''">$(_DefaultLinkMode)</_LinkMode> <!-- Let the .NET targets chime in -->
	<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' == 'macOS'">None</_LinkMode> <!-- Linking is off by default for macOS apps -->
	<_LinkMode Condition="'$(_LinkMode)' == '' And '$(_PlatformName)' != 'macOS'">SdkOnly</_LinkMode> <!-- Default linking is SdkOnly for iOS/tvOS/watchOS apps -->
	</PropertyGroup>
	</Target>

Fix default link mode for NativeAOT #18530

Fix default link mode for NativeAOT #18530

Conversation

filipnavara commented Jul 1, 2023

dalexsoto commented Jul 2, 2023

azure-pipelines bot commented Jul 2, 2023

vs-mobiletools-engineering-service2 commented Jul 2, 2023

💻 [CI Build] Windows Integration Tests passed 💻

vs-mobiletools-engineering-service2 commented Jul 2, 2023

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

vs-mobiletools-engineering-service2 commented Jul 2, 2023

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Jul 2, 2023

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

vs-mobiletools-engineering-service2 commented Jul 2, 2023

✅ API diff for current PR / commit

✅ API diff vs stable

ℹ️ Generator diff

vs-mobiletools-engineering-service2 commented Jul 3, 2023

🚀 [CI Build] Test results 🚀

Test results

Tests counts

ivanpovazan Jul 3, 2023

Choose a reason for hiding this comment

filipnavara Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

ivanpovazan Jul 3, 2023

Choose a reason for hiding this comment

ivanpovazan Jul 10, 2023

Choose a reason for hiding this comment

filipnavara Jul 10, 2023

Choose a reason for hiding this comment

filipnavara Jul 10, 2023

Choose a reason for hiding this comment

ivanpovazan Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

filipnavara Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

filipnavara Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

ivanpovazan Jul 10, 2023

Choose a reason for hiding this comment

filipnavara commented Jul 20, 2023

filipnavara Jul 3, 2023 •

edited

Loading

ivanpovazan Jul 10, 2023 •

edited

Loading

filipnavara Jul 10, 2023 •

edited

Loading

filipnavara Jul 10, 2023 •

edited

Loading