This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Support __register_frame with a full .eh_frame section
AbandonedPublic

Authored by mstorsjo on Mar 14 2018, 1:36 PM.

Details

Reviewers
rnk
compnerd
Summary

Previously, the __register_frame function supported registering an FDE for one individual function at a time. The function could also be used registering a full .eh_frame section of a module (which is how libgcc sets up unwinding in some configurations).

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 14 2018, 1:36 PM
rnk added inline comments.Mar 14 2018, 1:51 PM
src/UnwindCursor.hpp
52

I'm concerned that std::function might introduce runtime dependencies from libc++abi back to libc++. I think we get away with the <algorithm> include above because it's templated for the most part. Since this stuff is all in-header anyway, we could just take a non-type template parameter callable and call it directly.

mstorsjo added inline comments.Mar 14 2018, 2:14 PM
src/UnwindCursor.hpp
52

Ok, will change.

mstorsjo updated this revision to Diff 138451.Mar 14 2018, 2:15 PM

Avoid using the C++ <functional> header.

compnerd requested changes to this revision.Mar 28 2018, 1:39 PM

I really don't like this approach. I think that we should introduce a different entry point for this behavior rather than saying that we go through the existing interface. Having a reference to the .eh_frame_hdr seems better, as it would be better to make use of that optimization and we already have handling for that in libunwind.

This revision now requires changes to proceed.Mar 28 2018, 1:39 PM

I really don't like this approach. I think that we should introduce a different entry point for this behavior rather than saying that we go through the existing interface. Having a reference to the .eh_frame_hdr seems better, as it would be better to make use of that optimization and we already have handling for that in libunwind.

Well, this is just a plain .eh_frame, not an .eh_frame_hdr, since nothing for MinGW actually produces such a section for COFF (yet), GNU binutils doesn't either. Adding support for an indexed .eh_frame_hdr, while good from a performance PoV, feels like an orthogonal issue from this.

This else clause isn't about .eh_frame vs .eh_frame_hdr, but about registering a single FDE (which libunwind's __register_frame currently does) vs registering a full .eh_frame section (which libgcc's __register_frame does).

This else clause isn't about .eh_frame vs .eh_frame_hdr, but about registering a single FDE (which libunwind's __register_frame currently does) vs registering a full .eh_frame section (which libgcc's __register_frame does).

However, this function actually just is _unw_add_dynamic_fde, while __register_frame in UnwindLevel1-gcc-ext.c just calls this function. So we should probably check there instead, whether it's an FDE (which libgcc doesn't support via that entry point) or a full .eh_frame section.

This else clause isn't about .eh_frame vs .eh_frame_hdr, but about registering a single FDE (which libunwind's __register_frame currently does) vs registering a full .eh_frame section (which libgcc's __register_frame does).

However, this function actually just is _unw_add_dynamic_fde, while __register_frame in UnwindLevel1-gcc-ext.c just calls this function. So we should probably check there instead, whether it's an FDE (which libgcc doesn't support via that entry point) or a full .eh_frame section.

In this case, it triggers the "FDE is really a CIE" case in decodeFDE, so we could do a quick check in __register_frame and see if it is a CIE or an FDE, and then call the right underlying functions based on that, instead of adding it in the error handling else statement like this.

mstorsjo added a subscriber: joerg.Mar 28 2018, 2:53 PM

Also, relating to this, @joerg mentioned on irc that one should rather use __register_frame_info_bases instead of __register_frame. That function is a bit tricky to call though, since it uses a libgcc internal struct struct object, only available in libgcc headers and in Unwind_AppleExtras.cpp (as struct libgcc_object, with the comment "undocumented libgcc struct object").

mstorsjo updated this revision to Diff 140410.Mar 30 2018, 5:46 AM
mstorsjo edited the summary of this revision. (Show Details)

Added and using a helper function isCIE to avoid using the error return path from decodeFDE.

Technically, the decision between a full section and a plain FDE probably should be outside of _unw_add_dynamic_fde, in __register_frame in UnwindLevel1-gcc-ext.c. That'd require adding a C wrapper for isCIE in libunwind.cpp though.

The "struct object" is an implementation detail of the unwind implementation. You are guaranteed historically to get at least 8 longs / 8 pointers for internal use statically allocated in each object. What is stored inside is up to the unwind implementation.

mstorsjo abandoned this revision.Jul 11 2018, 11:47 AM

FWIW, I'm not going to put any more effort into this one at the moment, I managed to fix my use case in a much simpler way by providing a Windows Store compatible version of EnumProcessModules, see https://sourceforge.net/p/mingw-w64/mingw-w64/ci/0e1f41358845706a1adafcf4f9d27db0319dd3d9/.