This is an archive of the discontinued LLVM Phabricator instance.

Drop the dependency on dl_unwind_find_exidx().
ClosedPublic

Authored by ed on Dec 23 2016, 2:31 PM.

Details

Summary

While porting libunwind over to CloudABI for ARMv6, I observed that this
source file doesn't build, as it depends on dl_unwind_find_exidx(),
which CloudABI's C library was lacking. After I added that function,
I still needed to patch up libunwind to define _Unwind_Ptr.

Taking a step back, I wonder why we need to make use of this function
anyway. The unwinder already has some nice code to use dl_iterate_phdr()
to scan for a PT_GNU_EH_FRAME header. The dl_unwind_find_exidx() does
the same thing, except matching PT_ARM_EXIDX instead. We could also do
that ourselves.

This change gets rid of the dl_unwind_find_exidx() call and extends the
dl_iterate_phdr() loop. In addition to making the code a bit shorter, it
has the advantage of getting rid of some of those OS-specific #ifdefs.

This now means that if an operating system only provides
dl_iterate_phdr(), it gets support for unwinding on all architectures.
There is no need to add more stuff, just to get ARMv6 support.

Diff Detail

Repository
rL LLVM

Event Timeline

ed updated this revision to Diff 82422.Dec 23 2016, 2:31 PM
ed retitled this revision from to Drop the dependency on dl_unwind_find_exidx()..
ed updated this object.
ed added reviewers: kledzik, phosek, emaste.
ed added a comment.Dec 23 2016, 2:39 PM

Interesting observation: dwarf_index_section_length stores the length of the section, whereas arm_section_length stores the number of entries in the section. This feels a bit unnatural in my opinion. Should this be tackled in this change as well, or do we want to do this separately?

emaste edited edge metadata.Dec 23 2016, 5:29 PM

This seems reasonable to me.

IMO I would propose the dwarf_index_section_length vs arm_section_length change in a separate review.

src/AddressSpace.hpp
430 ↗(On Diff #82422)

perhaps a comment on the #else indicating which case it's for?

ed updated this revision to Diff 82434.Dec 23 2016, 11:21 PM
ed edited edge metadata.

Add a comment after the 'else' to indicate this code applies to ARM.

ed marked an inline comment as done.Dec 23 2016, 11:21 PM
ed added a comment.Dec 24 2016, 5:57 AM

Also an interesting observation: _LIBUNWIND_IS_BAREMETAL seems to be used only in this single source file and only provides an implementation for ARMv6.

Given that we're not implementing support for this for any other hardware architectures (x86, ARM64), is running on bare metal configurations even something we're aiming at? Is anyone using this? If not, should we consider removing it?

ed added a comment.Feb 2 2017, 4:22 AM

Friendly ping! :-)

ed updated this revision to Diff 88162.Feb 13 2017, 12:31 AM

Sync to latest codebase.

ed updated this revision to Diff 89175.Feb 21 2017, 1:55 AM

Restore comment that was lost during sync to latest code.

ed added a comment.Feb 21 2017, 1:56 AM

Any more feedback before I commit this change to the trunk?

This revision was automatically updated to reflect the committed changes.