Bundle assemblies at 4K for linux arm64 #41809

sbomer · 2020-09-03T16:41:29Z

On ARM64 Linux, single-file apps were segfaulting because of an incorrect address computed in R2R code. See #41832 for details about the issue.

This is a temporary fix that uses 4K offsets within the bundle. This will preserve the page alignment when we mmap assemblies, working around the lack of fixups for adrp; add instruction sequences.

I've confirmed that the fix, together with these SDK changes, produces a functional single-file app on Linux ARM64: sbomer/sdk@19998e6. I will make the SDK changes in the rc2 branch after this fix is ported and flows to the SDK.

Dotnet-GitSync-Bot · 2020-09-03T16:41:31Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2020-09-03T19:40:27Z

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

swaroop-sridhar · 2020-09-03T19:58:16Z

src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs

+
+            if (IsLinux && Arch == Architecture.Arm64)
+            {
+                // On ARM64, we use adrp; add instructions to encode pointers in the instruction stream.


I think this comment needs a little more explanation; that:

R2R assemblies from bundle are loaded on Linux via direct mmap() + offset fixups

R2R code sections will be individually mapped, adjusted for offset within the bundle + offset within the assembly and explicitly fixed up at load time.

Only on ARM64 the fixup logic is special and uses ardp ...

Only Linux ARM64 R2R assemblies need to be aligned, but here we align all assemblies for simplicity

Removing the alignment requirement is tracked by [Please file an issue in 6.0].

Please add the information to the design doc too, in this section. Add a Linux-ARM64 limitation, in addition to Windows limitations.

Thanks! I filed #41832, updated the comment, and am updating the docs at dotnet/designs#156.

Thank you very much, looks good.

To fix ARM64 Linux behavior. See dotnet/runtime#41809.

agocke · 2020-09-04T06:18:31Z

src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs

@@ -29,13 +29,29 @@ public class Bundler
        readonly TargetInfo Target;
        readonly BundleOptions Options;

-        // Assemblies are 16 bytes aligned, so that their sections can be memory-mapped cache aligned.
-        public const int AssemblyAlignment = 16;
+        // Temporary overload to avoid breaking the SDK build.


Don't we want to break the SDK build? That is, don't we want the insertion that brings this into the SDK to be accompanied with our change on the SDK side?

It would block dependency flow from runtime -> SDK until that change went in, which might be nice to avoid in case there are other fixes going through. If temporarily blocking it is ok, I'll remove this.

~~It will only block ingestion of 6.0 bits right?~~ That seems fine to me. We just have to make sure to have the change ready on the SDK side and move somewhat quickly.

Removed this as discussed. The plan is to make the SDK change quickly so that the dependency flow is not blocked for long.

We also need the change migrated to 5.0 branches in both repos, isn't it? Can we briefly take breaking changes there too?
In SDK, it is typical to only checkin to the 5.0 branch release, and let the change flow into master branch.

The SDK's invocation of the bundler needs to pass in the architecture argument, but it can only do so when this change flows into the SDK through HostModel version update. Given the position of Architecture argument, this looks like it'll break the SDK integration job; Isn't it?. So, we have the following options:

Make the update as part of SDK integration commit

Make the update in two steps -- once SDK gets a chance to update.

In this case, the change is small, and its OK to follow the former approach. But, we've followed the latter approach so far for HostModel/SDK interdependencies (originally per Nick's recommendation).

I'm not sure why he suggested (2), but I rather prefer (1), as it removes the possibility of accidentally dropping the corresponding change somewhere.

Nick suggested (2) because the SDK parts of the change are typically non-trivial, and they could be independently reviewed in a normal PR, rather than in the automated integration job, and because they can be independently reverted if there's a problem without affecting the other parts of integration.

But in this case, the change on the SDK side is very small, so I'm fine with (1).

That makes sense. For larger changes (2) could definitely be appropriate, especially if we're concerned that CI wouldn't fully validate the change.

Just to be clear - I do plan to get the change reviewed in a normal PR, not in the automated job. I'll include a manual dependency update in my PR, but the automated flow will be blocked until my PR is merged.

agocke · 2020-09-04T06:22:43Z

src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs

        public readonly Version FrameworkVersion;
        public readonly uint BundleVersion;
        public readonly BundleOptions DefaultOptions;
+        public readonly int AssemblyAlignment;


Do we need to store the AssemblyAlignment in a field? It looks like it could be a calculated property based on the Arch. Moreover, do we need to store it on TargetInfo at all? It's not really a property of the target info. It was previously stored in Bundler.cs, which seems like the only place it's needed.

I made it a field for consistency with DefaultOptions and BundleVersion, but don't have a strong preference. To me it seems like it belongs with TargetInfo because it depends on the alignment requirements of the target platform - for example if the bundler accepted an option to change the alignment, we would still only allow multiples of this value (similar to DefaultOptions).

belongs with TargetInfo because it depends on the alignment requirements of the target platform

I would disagree -- TargetInfo is a collection of data about the target, while alignment is a piece of data about the bundler, which is dependent on TargetInfo. My general strategy would be for TargetInfo to carry only the essentials of the target, because that makes it more reusable and lighter weight for dependent components.

However I think this is all pretty academic because all of this code is pretty small, so it doesn't particularly matter to me either way 😄

Point taken - I'll leave it this way for consistency, but we could consider removing bundler-related decisions from it.

agocke

Aside from overload LGTM

src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs

sbomer · 2020-09-05T00:08:43Z

/backport to release/5.0-rc2

github-actions · 2020-09-05T00:09:45Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/240210475

Backport of #41809 to release/5.0-rc2 This fixes single-file deployments on Linux ARM64, which were segfaulting due to an incorrect computed address in R2R code. See #41832 for details about the issue.

To fix ARM64 Linux behavior. See dotnet/runtime#41809.

* Pass architecture to bundler To fix ARM64 Linux behavior. See dotnet/runtime#41809. * Fix how we get architecture from RIDs * Update dependencies from runtime * Revert incorrect targetOS change We fall back to linux so that RIDs like ubuntu* are supported.

Bundle assemblies at 4K for linux arm64

d53cf1e

sbomer force-pushed the bundleFix branch from a8e015c to d53cf1e Compare September 3, 2020 16:43

sbomer requested a review from swaroop-sridhar September 3, 2020 16:44

jkotas added the area-Single-File label Sep 3, 2020

swaroop-sridhar approved these changes Sep 3, 2020

View reviewed changes

sbomer added 2 commits September 3, 2020 21:54

Add arch to bundler trace output

3bedb89

PR feedback

bf5db17

sbomer mentioned this pull request Sep 3, 2020

Update single file docs dotnet/designs#156

Merged

sbomer added a commit to sbomer/sdk that referenced this pull request Sep 3, 2020

Pass architecture to bundler

19998e6

To fix ARM64 Linux behavior. See dotnet/runtime#41809.

sbomer requested a review from vitek-karas September 3, 2020 22:43

agocke reviewed Sep 4, 2020

View reviewed changes

sbomer mentioned this pull request Sep 4, 2020

Pass architecture to bundler dotnet/sdk#13402

Merged

agocke approved these changes Sep 4, 2020

View reviewed changes

sbomer added 2 commits September 4, 2020 17:56

Remove extra overload

e23df80

Update comment

586fd94

vitek-karas approved these changes Sep 4, 2020

View reviewed changes

src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs Outdated Show resolved Hide resolved

src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs Outdated Show resolved Hide resolved

Fix RID computation

13d46bd

vitek-karas approved these changes Sep 4, 2020

View reviewed changes

sbomer merged commit 9e22b9f into dotnet:master Sep 5, 2020

github-actions bot mentioned this pull request Sep 5, 2020

[release/5.0-rc2] Bundle assemblies at 4K for linux arm64 #41907

Merged

sbomer added a commit to sbomer/sdk that referenced this pull request Sep 8, 2020

Pass architecture to bundler

75e73ee

To fix ARM64 Linux behavior. See dotnet/runtime#41809.

sbomer mentioned this pull request Sep 9, 2020

Pass architecture to bundler (#13402) dotnet/sdk#13466

Merged

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

sbomer deleted the bundleFix branch November 3, 2023 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle assemblies at 4K for linux arm64 #41809

Bundle assemblies at 4K for linux arm64 #41809

sbomer commented Sep 3, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Sep 3, 2020

ghost commented Sep 3, 2020

swaroop-sridhar Sep 3, 2020

swaroop-sridhar Sep 3, 2020

sbomer Sep 3, 2020

swaroop-sridhar Sep 3, 2020

agocke Sep 4, 2020

sbomer Sep 4, 2020

agocke Sep 4, 2020 •

edited

Loading

sbomer Sep 4, 2020

swaroop-sridhar Sep 4, 2020

swaroop-sridhar Sep 4, 2020

agocke Sep 4, 2020

swaroop-sridhar Sep 4, 2020

agocke Sep 4, 2020

sbomer Sep 4, 2020

agocke Sep 4, 2020

sbomer Sep 4, 2020

agocke Sep 4, 2020

sbomer Sep 4, 2020

agocke left a comment

sbomer commented Sep 5, 2020

github-actions bot commented Sep 5, 2020

Bundle assemblies at 4K for linux arm64 #41809

Bundle assemblies at 4K for linux arm64 #41809

Conversation

sbomer commented Sep 3, 2020 • edited Loading

Dotnet-GitSync-Bot commented Sep 3, 2020

ghost commented Sep 3, 2020

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

agocke Sep 4, 2020 • edited Loading

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

agocke left a comment

Choose a reason for hiding this comment

sbomer commented Sep 5, 2020

github-actions bot commented Sep 5, 2020

sbomer commented Sep 3, 2020 •

edited

Loading

agocke Sep 4, 2020 •

edited

Loading