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

Move libunwind directories to src/native/external #64043

Merged
merged 8 commits into from
Mar 21, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Jan 20, 2022

  • src/coreclr/pal/src/libunwind -> src/native/external/libunwind
  • src/coreclr/pal/src/libunwind_mac -> src/native/external/libunwind_extras/mac
  • src/coreclr/pal/src/libunwind/libunwind-version.txt -> src/native/external/libunwind-version.txt
  • src/coreclr/pal/src/libunwind/src/oop -> src/native/external/libunwind_extras/oop
    • so extra files that we compile as part of libunwind project are in same place (currently they are in two places: libunwind_mac and oop directories)
  • Extracted cmake configs in src/native/external/libunwind.cmake and reverted cmake files in vendor directory to upstream's original state (same structure is used for other external libs).

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

ghost commented Jan 20, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details
  • src/coreclr/pal/src/libunwind -> src/native/external/libunwind
  • src/coreclr/pal/src/libunwind_mac -> src/native/external/libunwind_extras/mac
  • src/coreclr/pal/src/libunwind/libunwind-version.txt -> src/native/external/libunwind-version.txt
  • src/coreclr/pal/src/libunwind/src/oop -> src/native/external/libunwind_extras/oop
    • so extra files that we compile as part of libunwind project are in same place (currently they are in two places: libunwind_mac and oop directories)
  • Extracted cmake configs in src/native/external/libunwind.cmake and reverted cmake files in vendor directory to upstream's original state (same structure is used for other external libs).
Author: am11
Assignees: -
Labels:

area-System.Reflection.Metadata, community-contribution

Milestone: -

@am11 am11 force-pushed the feature/consolidations branch 5 times, most recently from 9ce16a4 to 12aa0a2 Compare January 20, 2022 17:07
@am11
Copy link
Member Author

am11 commented Jan 21, 2022

cc @jkotas, @janvorli, this is a continuation of #62673. I haven't moved llvm-libunwind (https://github.com/dotnet/runtime/tree/f3e4e768d10dc502b0a7e9c417ae269dcf75dbc7/src/coreclr/nativeaot/libunwind) to external yet, as its tree is diverged from llvm-project's release/9.x branch (those patches were never upstreamed). Since now everything is in single repo; if it doesn't regress in size and functionality, should we try to switch NativeAOT to also use this (HP) libunwind (which supports more architectures -- like s390x -- compared to llvm libunwind)?

@jkotas
Copy link
Member

jkotas commented Jan 21, 2022

if it doesn't regress in size and functionality, should we try to switch NativeAOT to also use this (HP) libunwind (which supports more architectures -- like s390x -- compared to llvm libunwind)?

Yes, it would be nice to unify it if possible. IIRC, macOS pushed us to use llvm libunwind. HP libunwind did not work on macOS and the macOS-provided libunwind did not work either. I do not remember the details. Does HP libunwind work on macOS?

@am11
Copy link
Member Author

am11 commented Jan 21, 2022

Ah, that's still the case. We are only using remote unwind feature of HP libunwind on macOS (and FreeBSD) in CoreCLR, for out-of-context unwinding. For the rest of functionality, we use OS provided libunwind (which Apple contributed as llvm-libunwind to LLVM project in 2013). We could use same mechanism in NativeAOT since both libunwind libraries have matching headers. I haven't looked at NativeAOT usage of libunwind in depth, but from a quick search, @janvorli implemented unw_get_save_loc for memory-locations (HP libunwind provides this functionality for both register and memory locations), which we are using in nativeaot/Runtime/unix/UnixContext.cpp.

ps - Mono uses it's own implementation https://github.com/dotnet/runtime/blob/b85a435/src/mono/mono/utils/mono-stack-unwinding.h / https://github.com/dotnet/runtime/blob/b85a435/src/mono/mono/utils/mono-context.h. It might be possible to switch NativeAOT to Mono's implementation and get rid of llvm-libunwind dependency that way.

@janvorli
Copy link
Member

That's funny, I have completely forgotten that I have added the unw_get_save_loc to our local copy of that libunwind. Maybe we could actually merge that upstream (I'd look into implement the unw_get_save_loc support for location in registers too to make it fully implemented) and then using the LLVM libunwind as the unified version might be also an option.
The only issue would be the missing support for the s390x and the longsoonarch64 that is now being enabled by a 3rd party. But maybe adding support for that would not be difficult. The LLVM libunwind code is almost completely arch agnostic.

@am11
Copy link
Member Author

am11 commented Jan 21, 2022

These are all the llvm-libunwind patches (based on releases/9.x branch), in our NativeAOT tree: llvm/llvm-project@release/9.x...am11:dotnet-runtime-patches. On their main branch, RISC-V and MIPS64 arch support was added since 9.x.

@kant2002
Copy link
Contributor

I’m not sure if I’m qualified to speak with authority, but LLVM libunwind for ARM a bit undeveloped. Overall code looks like it is half done. I was able only fix gross issue like this llvm/llvm-project@08a5ac3 In EHABI handling. For me this code is a bit scary

@am11
Copy link
Member Author

am11 commented Jan 22, 2022

Support status related to architectures we are supporting in .NET runtime (and the upcoming ones, like loongarch64, riscv64):

Mono unwinder - all archs (except loongarch64? shouldn't be hard to implement since both MIPS and RISC-V are supported)
HP libunwind - all archs
LLVM libunwind - all archs except s390x and loongarch64 (llvm does not dogfood its libunwind)
GCC unwinder - all archs except loongarch64 (PR is up for loongarch64; gcc does dogfood its unwinder)

If mono unwinder (currently supporting: xarch, armarch, s390x, mips, riscv etc.) suffice the stack unwinding requirements of coreclr and nativeaot (i.e. something equivalent to unw_get_save_loc is supported), we can move it to src/native/stack-unwinder and delete both HP and LLVM libunwinds directories from the repo. IMHO, that would be the best case scenario and we won't need to wait for next release of external libraries for next architecture support (rust, too, has its own stack unwinder implementation like mono).

@jkotas
Copy link
Member

jkotas commented Jan 22, 2022

Mono unwinder - all archs (except loongarch64? shouldn't be hard to implement since both MIPS and RISC-V are supported)

I have discussed this with the Mono folks in the past. Mono unwinder is incomplete - it only handles the unwind sequences that Mono emits, it is not a general purpose unwinder.

@am11
Copy link
Member Author

am11 commented Jan 22, 2022

  1. This document has the current status of llvm-libunwind https://github.com/llvm/llvm-project/blob/main/libunwind/docs/index.rst#current-status, which notes:

    Remote unwinding remains as future work.

    IIUC, in cross DAC implementation of CoreCLR, we rely on remote (or out-of-context) unwinding (for Windows->Linux and macOS->Linux diagnostics). This is in addition to unw_get_save_loc shortcomings. Adding support for both of these missing features is a non-trivial work. Therefore, removing HP libunwind dependency is not as feasible at this point.

  2. HP libunwind, OOTH, does not support formats beyond DWARF; whereas, LLVM libunwind supports COFF/XCOFF, DWARF and CodeView formats. Adding macOS MachO/XCOFF support is a non-trivial work. Therefore, removing LLVM libunwind from NativeAOT is also not feasible at this point.

  3. Mono unwinder is incomplete - it only handles the unwind sequences that Mono emits, it is not a general purpose unwinder.

    Therefore reliance on Mono unwinder in CoreCLR and NativeAOT (in order to remove both LLVM and HP libunwind dependencies) is also not feasible right now.

I think then this PR is ready for review as-is. We can move llvm-libunwind tree under src/native/external separately (ideally after upstreaming the local changes).

@jkotas jkotas requested a review from janvorli February 8, 2022 12:36
@am11
Copy link
Member Author

am11 commented Mar 17, 2022

Resolved merge conflicts, green CI. 🌴

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, just few missing licensing headers in files that are now internal to our repo.

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!

@am11
Copy link
Member Author

am11 commented Mar 21, 2022

@jkotas, @janvorli, could this be merged before #66898? I would like to rebase PR #66898 against main branch, rather than this one. :)

@janvorli janvorli merged commit 9c9ebb9 into dotnet:main Mar 21, 2022
@am11 am11 deleted the feature/consolidations branch March 21, 2022 17:52
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
* Move libunwind directories to src/native/external

* Move extra additions to single directory

* Extract coreclr configs in libunwind.cmake

* Reapply upstream patches

* Fixups
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-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