[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGbab39816085d: [libunwind] Add an interface for dynamic .eh_frame registration (authored by housel). Repository: rG

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. There doesn't seem to be any objection to this approach, so I think you're free to land @housel. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111863/new/ https://reviews.llvm.org/D111863

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. I've chatted about this with Joerg on a side thread and wanted to summarize my understanding of the situation. The problem is that we have incompatible behaviors for `__register_frame` and `__deregister_frame` between unwinders: - libgcc_s and the FreeBSD port of

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. @joerg, @emaste -- I finally got a chance to experiment with a Linux docker container and confirmed that libgcc_s's `__register_frame` can handle individual frames. Unfortunately that does not help us on FreeBSD. If we ignore FreeBSD for a moment we could imagine switch

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3071356 , @steven_wu wrote: > In D111863#3071328 , @housel wrote: > >> In D111863#3069279 , @lhames wrote: >> >>> I think the ORC

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

emaste added a comment. In D111863#3100654 , @lhames wrote: > @joerg @dim @emaste Any further comments on this? I have no objection. 2016 (when I patched FreeBSD's in-tree libunwind) is long enough ago that I don't recall any of the context but indeed

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames accepted this revision. lhames added a comment. @joerg @dim @emaste Any further comments on this? I'd like @housel to be able to land this this week if possible. It will significantly improve the usability of libunwind for JIT clients by giving us an API with behavior that is consistent

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel updated this revision to Diff 382884. housel added a comment. Added additional comments to `CFI_Parser::decodeFDE`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111863/new/ https://reviews.llvm.org/D111863 Files:

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. @joerg Any further thoughts on this? I'm happy for it to go in as-is -- we can always make the implementation of `__unw_add_dynamic_eh_frame_section` / `__unw_remove_dynamic_eh_frame_section` later, but the API looks good to me, and I think this is a strict improvement

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3074404 , @housel wrote: > To be clear, this new code parses exactly as much of each FDE as the existing > `__register_frame`/`__unw_add_dynamic_fde` does, including doing the same > work to compute the record length.

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel added a comment. To be clear, this new code parses exactly as much of each FDE as the existing `__register_frame`/`__unw_add_dynamic_fde` does, including doing the same work to compute the record length. Neither needs to parse the instructions at registration time. Repository: rG

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3072827 , @joerg wrote: > `__register_frame` requires parsing the CIE header, but not the whole FDE > program. E.g. that's the `findPCRange` logic. After that, the FDE is just > added to the internal block list.

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel added a comment. In D111863#3072829 , @joerg wrote: > Are you mixing up of `__register_frame` and `__register_frame_info`? No? FreeBSD doesn't patch `__register_frame_info`, and so (like base libunwind) it does nothing in the code I linked.

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

joerg added a comment. In D111863#3072023 , @housel wrote: > It's also worth noting that FreeBSD's version of libgcc exception handling is > actually based on the libunwind code, with a local patch >

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

joerg added a comment. In D111863#3071651 , @lhames wrote: > In D111863#3071353 , @joerg wrote: > >> I would strongly prefer if ORCv2 doesn't depend on this. It essentially >> forces libunwind to parse the whole

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

dim added a subscriber: emaste. dim added a comment. In D111863#3072023 , @housel wrote: > It's also worth noting that FreeBSD's version of libgcc exception handling is > actually based on the libunwind code, with a local patch >

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

MaskRay added a subscriber: dim. MaskRay added a comment. In D111863#3072023 , @housel wrote: > It's also worth noting that FreeBSD's version of libgcc exception handling is > actually based on the libunwind code, with a local patch >

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel added a comment. It's also worth noting that FreeBSD's version of libgcc exception handling is actually based on the libunwind code, with a local patch that implements compatibility with

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3071353 , @joerg wrote: > I would strongly prefer if ORCv2 doesn't depend on this. It essentially > forces libunwind to parse the whole section just to find the delimiters of > the FDEs. That's a lot of unnecessary

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

steven_wu added a comment. In D111863#3071328 , @housel wrote: > In D111863#3069279 , @lhames wrote: > >> I think the ORC runtime provides a much more natural way to test this. Did >> you manage to come up with

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

joerg added a comment. I would strongly prefer if ORCv2 doesn't depend on this. It essentially forces libunwind to parse the whole section just to find the delimiters of the FDEs. That's a lot of unnecessary work as JIT use normally allows registering functions individually. Repository: rG

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel added a comment. In D111863#3069279 , @lhames wrote: > I think the ORC runtime provides a much more natural way to test this. Did > you manage to come up with some ORC-runtime based tests in the end? My current plan is to automate what I've been

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

steven_wu added a comment. I don't know enough about Dwarf unwinding but the implementation looks generally good. Can you please add a testcase indicating how ORCJIT is planning to use it? Comment at: libunwind/src/DwarfParser.hpp:158 + FDE_Info

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames accepted this revision. lhames added a comment. This looks good to me, but we should get a libunwind contributor to weigh in too. I've been trying to think of a good way to test this, but it's awkward. The best strategy that I've come up with, at least for testing within libunwind

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel added a comment. In D111863#3065992 , @MaskRay wrote: > I looked at the libgcc mechanism at one time. I remember that in most cases > it just uses `PT_GNU_EH_FRAME` and these eh_frame boundary registry functions > are not needed. > Can ORC just

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

MaskRay added a comment. I looked at the libgcc mechanism at one time. I remember that most cases just use `PT_GNU_EH_FRAME` and these eh_frame boundary registry functions are not needed. Can ORC just use `PT_GNU_EH_FRAME`? Clang passes `--eh-frame-hdr` to ld even in `-static` mode, unlike

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

housel created this revision. housel added a reviewer: cfe-commits. Herald added a project: libunwind. Herald added a subscriber: libcxx-commits. Herald added a reviewer: libunwind. housel requested review of this revision. Herald added a project: LLVM. Herald added a subscriber: llvm-commits.