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

[NativeAOT] Clean up some of the ARM32 bit rotten code #96583

Merged
merged 4 commits into from
Jan 7, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Jan 6, 2024

I don't intend to get the ARM32 target working but I used it to test the 32-bit code path for the ELF ObjWriter and fixed part of the compilation while I was at it.

The GC hijacking code is still missing. This likely wouldn't be too hard to restore since a version of the code exists in the Windows .asm version.

EHABI exception handling didn't compile after the last few refactorings. That would need to be reimplemented and it likely would not be prohibitively hard. I'm not sure if the EHABI was ever emitted by ObjWriter though, so perhaps it would still need to work in tandem with the DWARF EH ABI for the ILC emitted code.

ARMEmitter.EmitMOV emits code that's incompatible with position independent executables. Should be reasonably easy to fix by using PC-relative relocation and emitting add <dest>, <dest>, PC (+ immediate offset?) instruction. I am too lazy to figure out how to encode that.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 6, 2024
@ghost
Copy link

ghost commented Jan 6, 2024

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

Issue Details

I don't intend to get the ARM32 target working but I used it to test the 32-bit code path for the ELF ObjWriter and fixed part of the compilation while I was at it.

The GC hijacking code is still missing. They likely wouldn't be too hard to restore since a version of the code exists in the Windows .asm version.

EHABI exception handling didn't compile after the last few refactorings. That would need to be reimplemented and it likely would not be prohibitively hard. I'm not sure if the EHABI was ever emitted by ObjWriter though, so perhaps it would still need to work in tandem with the DWARF EH ABI for the ILC emitted code.

ARMEmitter.EmitMOV emits code that's incompatible with position independent executables. Should be reasonably easy to fix by using PC-relative reloction and emitting add <dest>, <dest>, PC (+ immediate offset?) instruction. I am too lazy to figure out how to encode that.

Author: filipnavara
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr

Milestone: -

Copy link
Member

Choose a reason for hiding this comment

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

Theoretical question: We are only building subset of llvm-libunwind in nativeaot (3 out of 18 cpp files). Given the DWARF manipulation impl in objwriter3 branch, what would it take to get rid of llvm-libunwind dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Emitting the DWARF EH subset is easy, interpreting the same subset is also relatively easy, but the full scope of DWARF EH is quite complex. In order to unwind over code that we didn't generate we'd need to implement the full expression language (AFAIK).

Unlike the ObjWriter work, there are no obvious performance or memory usage wins here.

Copy link
Member

@am11 am11 Jan 6, 2024

Choose a reason for hiding this comment

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

Thanks! As to the benefits: it mainly concerns maintainability and portability (if nothing else). Within the llvm toolchain, llvm-libunwind porting is "optional", i.e. none of the essential components of toolchain depend on it. So s390x, ppc64le etc. had to go out of their way to add support there for dotnet runtime not too long ago. Then upgrading is also rough; we have to cherry-pick all the patches dating back to 2016 because they weren't upstreamed (#72655). Local unwinder implementation will bring this overhead down to exactly what's needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that there's a maintenance annoyance with keeping the llvm-libunwind copy in sync. Unlike ObjWriter it's not spread across two different repositories which was a major annoyance for me when trying to update the code in sync.

I'm not entirely opposed to the idea of writing the DWARF unwinder but I expect the testing requirement for that would be massive. So far my cost/benefit analysis was on the side of not spending the time on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Thank you!

@jkotas jkotas merged commit c33557d into dotnet:main Jan 7, 2024
172 of 177 checks passed
@filipnavara filipnavara deleted the arm-bitrot branch January 7, 2024 18:20
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm32 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