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

Add N_NO_DEAD_STRIP flag to section symbols #106444

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Aug 15, 2024

Extracted from #106224.

PR #103039 added N_NO_DEAD_STRIP flag to all symbols emitted by ILC and enabled the dead code stripping in the native linker.

It failed to handle one specific edge case that is luckily not happening in the wild. If the first node emitted into a section has a symbol with non-zero offset NN the first N_NO_DEAD_STRIP symbol is not pointing at the start of the section. The native linker then splits up the section into atom and the first atom from offset 0 to offset NN is never referenced and becomes eligible for dead code stripping. Since we emit a symbol for each section start (for use in section-relative relocations) we can just mark the symbol with N_NO_DEAD_STRIP to resolve the issue.

…e beginning of the section are not stripped if there's no other N_NO_DEAD_STRIP symbol referencing them
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 15, 2024
Copy link
Contributor

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

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop, runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop, runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@MichalStrehovsky
Copy link
Member

Cc @ivanpovazan @dotnet/jit-contrib @dotnet/ilc-contrib for Apple-specific ObjWriter change. Looks reasonable to me but don't know much about Apple stuff.

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

@filipnavara I assume that we do not need to backport this fix to .NET 9. Correct?

@filipnavara
Copy link
Member Author

I assume that we do not need to backport this fix to .NET 9. Correct?

I am on a verge on this one. On one hand it's not something that I saw in practice so it may not meet the bar. On the other hand, if it ever happens it will be incredibly difficult to trace it back to the root cause. Note the -dead_strip on Apple linker was only re-enabled in .NET 9 and hence this is a code path that didn't see any substantial usage in the wild yet.

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

The change looks good to me.
I am curious though, how did you find out the edge case that the previous PR did not cover?

@filipnavara
Copy link
Member Author

I am curious though, how did you find out the edge case that the previous PR did not cover?

It was result of the discussion in #106224. Michal pointed out that MethodTables are emitted with prepended GCDesc data in the front and the actual symbol is defined with non-zero offset in relation to the data. If this was emitted as the first thing in the data section we would hit this edge case. Luckily, it seems that in most cases we get some other data emitted in front with zero-offset symbol, and so the GCDesc part becomes part of the preceding linker atom which is already marked as N_NO_DEAD_STRIP.

I tested the PR #106224 with both the old ld64 linker and the new ld-prime linker (which is still disabled by default in .NET), and I actually hit a case that exposed this bug. That said, it was most likely an artifact of other (local) changes and not reproducible in the wild.

@MichalStrehovsky MichalStrehovsky merged commit 0c90519 into dotnet:main Aug 19, 2024
130 of 142 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
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.

4 participants