Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CoreCLR/NativeAOT assembly compile with .subsections_via_symbols on Apple platforms #92520

Merged
merged 2 commits into from
Sep 23, 2023

Conversation

filipnavara
Copy link
Member

Related to #92297 #92491

The new Xcode 15 linker mishandles object files without the MH_SUBSECTIONS_VIA_SYMBOLS flag (.subsections_via_symbols assembly code). It ends up treating the whole object file as one function and produces unwind info only for the first function in the file.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 23, 2023
@ghost
Copy link

ghost commented Sep 23, 2023

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

Issue Details

Related to #92297 #92491

The new Xcode 15 linker mishandles object files without the MH_SUBSECTIONS_VIA_SYMBOLS flag (.subsections_via_symbols assembly code). It ends up treating the whole object file as one function and produces unwind info only for the first function in the file.

Author: filipnavara
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr

Milestone: -


NotInHeap:
b C_FUNC(RhpAssignRefArm64)
Copy link
Member Author

@filipnavara filipnavara Sep 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional jump to non-local labels are not allowed in subsections-via-symbols mode. I updated 3-4 places to use non-conditional branches instead, it's mostly in the "rare" code paths.

Most of the diff is just decorating local labels with LOCAL_LABEL.

@filipnavara filipnavara marked this pull request as ready for review September 23, 2023 11:24
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@jkotas jkotas merged commit 5dfd805 into dotnet:main Sep 23, 2023
109 checks passed
@jkotas
Copy link
Member

jkotas commented Sep 23, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6286618371

@jkotas
Copy link
Member

jkotas commented Sep 24, 2023

We depend on assembly code to stay together here:

LEAF_ENTRY JIT_PatchedCodeStart, _TEXT
ret
LEAF_END JIT_PatchedCodeStart, _TEXT

Can .subsections_via_symbols break this guarantee by reordering methods inside the file?

@filipnavara
Copy link
Member Author

Can .subsections_via_symbols break this guarantee by reordering methods inside the file?

In theory it probably can. I'll check what the linker happens to do in practice. I don't think it does reordering, and we don't use "dead strip" or "identical code folding" options that could make this a real problem.

(good catch!)

@filipnavara
Copy link
Member Author

filipnavara commented Sep 24, 2023

I am blocked by unrelated build breakage (related to PR #92301 and discussed there). I don't feel overly confident in the results of the analysis (with and without -dead_strip) that I gathered so far, so I will have to resolve the build breakage first to proceed further.

Meanwhile, I checked that some other platforms (eg. arm32) split of this code into patchedcode.S file. I suppose we don't depend on unwind information for these particular functions, especially since they are patched at runtime. Would it make sense to do the split for arm64 and amd64 too and turn off .subsections_via_symbols for them? That way we would get the original ordering/preservation guarantees and only lose the unwind information for this specific code [with the Xcode 15 linker].

@filipnavara
Copy link
Member Author

filipnavara commented Sep 24, 2023

Checked output on arm64 so far, both in the default configuration and with added -dead_strip, with the Xcode 15 default linker and the legacy one (-ld_classic switch). In all cases the code was preserved and didn't get reordered. So, from strictly practical point of view, the generated output is correct.

From the theoretical point of view, I think the linker is allowed to do some reordering. The comments in the old linker specifically mention "Atoms in the same section from the same .o file should be contiguous and sequenced in the same order they were in the .o file," but it's not obvious how hard the guarantee is and whether it applies only to the initial order or whether it can be affected by some subsequent optimization. The only two reordering optimizations that I am aware of are a) reordering initialiser/terminator functions to the start/end of code section, and b) reordering DATA section to group together data modified by the dyld dynamic linker. Neither of those optimizations applies here.

That leaves us with several options:

  1. Do nothing. The downside is that a future linker update may start doing some optimization that breaks the code.
  2. Split the patchable code into separate patchedcode.S file and disable .subsections_via_symbols for it individually. The new linker would lose the unwind info for this specific code but we may not need it anyway. I'd need someone more knowledgable to confirm or deny that assumption. The upside of that approach is that we would still communicate the correct semantics to the linker so it's less likely to be accidentally broken. The downside is that we would still hit the Xcode 15 linker bug but only for a very limited part of code where it may not have consequences.
  3. Use the -order_file linker switch and explicitly enforce the order of the symbols inside the patchable section. This is potentially an unchartered territory where we may hit another linker bug minefield. It also introduces a maintenance burden where the order file needs to be kept up-to-date with the assembly code.
  4. Detect Xcode version and force the use of classic linker with -ld_classic switch if Xcode 15 is used. This complicates the build process and it would only buy us some limited amount of time. Apple publicly stated that they intend to remove the old linker in future Xcode update.

Interestingly, -dead_strip does strip about 120Kb of code on debug libcoreclr.dylib build, so it might be worth checking out separately as a size optimization.

@filipnavara
Copy link
Member Author

One more thing, JIT_UpdateWriteBarrierState is inside the patched section for .asm (Windows) and outside for .S (non-Windows). Is that intentional?

@jkotas
Copy link
Member

jkotas commented Sep 26, 2023

One more thing, JIT_UpdateWriteBarrierState is inside the patched section for .asm (Windows) and outside for .S (non-Windows). Is that intentional?

I do not see a good reason for it. I think it should be outside the patched section in both cases.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants