Skip to content

Conversation

@filipnavara
Copy link
Member

@filipnavara filipnavara commented Jan 10, 2024

Ref: #95876 (comment)
Ref: https://github.com/dotnet/llvm-project/blob/d8bacb4031b1d1e290ab2792e8378560419ee0de/llvm/lib/MC/MCDwarf.cpp#L1759-L1764

The DWARF specification requires the CIE/FDE entries in the .eh_frame sections to be aligned to pointer size. However, I incorrectly implemented it as the platform target pointer size, while it's supposed to be the DWARF pointer size specified in the pointer encoding field. Since we use 4-byte relative pointers on all platforms this means the alignment is supposed to be on the 4-byte boundary, not on the 8-byte boundary.

Tested on https://github.com/aspnet/Benchmarks/tree/main/src/BenchmarksApps/BasicMinimalApi where we now produce the same size of .eh_frame section as the legacy ObjWriter. Prior to the fix the additional alignment contributed 53,736 bytes to the section size (out of roughly ~1MiB).

@ghost ghost added area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member labels Jan 10, 2024
@ghost
Copy link

ghost commented Jan 10, 2024

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

Issue Details

Ref: #95876 (comment)
Ref: https://github.com/dotnet/llvm-project/blob/d8bacb4031b1d1e290ab2792e8378560419ee0de/llvm/lib/MC/MCDwarf.cpp#L1759-L1764

The DWARF specification requires the CIE/FDE entries in the .eh_frame sections to be aligned to pointer size. However, I incorrectly implemented it as the platform target pointer size, while it's supposed to be the DWARF pointer size specified in the pointer encoding field. Since we use 4-byte relative pointers on all platforms this means the alignment is supposed to be on the 4-byte boundary, and not on the 8-byte boundary.

Author: filipnavara
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@filipnavara filipnavara changed the title Fix over-alignment of .eh_frame entries [NativeAOT] ObjWriter: Fix over-alignment of .eh_frame entries Jan 10, 2024
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thanks!

@MichalStrehovsky MichalStrehovsky merged commit 81c6235 into dotnet:main Jan 23, 2024
@filipnavara filipnavara deleted the eh-frame-align branch January 23, 2024 10:26
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@am11
Copy link
Member

am11 commented Jan 24, 2024

Unfortunately this missed preview 1 by few hours (#97336), @carlossanlop any chance we can include this change in P1? ilc-in-c# is a new feature, it will be tested more thoroughly by masses once P1 is announced.

@carlossanlop
Copy link
Contributor

carlossanlop commented Jan 24, 2024

Not sure if we still have time, let me ask Tactics what the build status is.

@MichalStrehovsky @agocke while we confirm there's still runway, can you please take a look and let me know if you'd like to backport this change to release/9.0-preview1?

@jkotas
Copy link
Member

jkotas commented Jan 24, 2024

This is a fix for relatively minor perf regression. I do not think it is needed in Preview 1.

@am11
Copy link
Member

am11 commented Jan 24, 2024

Agreed with @jkotas, it was a non-critical suggestion (if we still had time). :)

@carlossanlop
Copy link
Contributor

Since we are still taking some more changes, we have time to get this backported. Let's do it.

@carlossanlop
Copy link
Contributor

/backport to release/9.0-preview1

@github-actions
Copy link
Contributor

Started backporting to release/9.0-preview1: https://github.com/dotnet/runtime/actions/runs/7643752342

@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 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.

6 participants