Page MenuHomePhabricator

[libunwind][DWARF] Fix end of .eh_frame calculation
ClosedPublic

Authored by rprichard on Sep 16 2020, 2:04 AM.

Details

Summary
  • When .eh_frame is located using .eh_frame_hdr (PT_GNU_EH_FRAME), the start of .eh_frame is known, but not the size. In this case, the unwinder must rely on a terminator present at the end of .eh_frame. Set dwarf_section_length to UINTPTR_MAX to indicate this.
  • Add a new field, dso_length, that the FrameHeaderCache uses to track to size of the PT_LOAD segment indicated by dso_base.
  • Compute ehSectionEnd by adding sectionLength to ehSectionStart, never to fdeHint.

Fixes PR46829.

Diff Detail

Event Timeline

rprichard created this revision.Sep 16 2020, 2:04 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 16 2020, 2:04 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard requested review of this revision.Sep 16 2020, 2:04 AM

An alternative to D86256.

It adds another field to UnwindInfoSections, so the cache will use more memory:

  • This can obviously be avoided, because dwarf_section_length is now always initialized to UINTPTR_MAX when _LIBUNWIND_USE_DL_ITERATE_PHDR and _LIBUNWIND_SUPPORT_DWARF_INDEX are defined.
  • Even the dwarf_section field is unnecessary if we're willing to decode the .eh_frame_hdr on a cache hit. (The unwinder already decodes it again, later, because most of the fields aren't stored in UnwindInfoSections. I think decoding is fairly cheap.)
  • The dwarf_index_section_length field isn't really used much, currently. It's used to bounds-check fields if they use LEB encodings. (They probably don't). It *could* be used to do more bounds checking, though.
  • If this matters, I think it can be fixed later.

After disentangling dso_length from dwarf_section_length, it should be easy to combine the DWARF and EHABI versions of findUnwindSectionsByPhdr in AddressSpace.hpp. Only the PT_GNU_EH_FRAME vs PT_ARM_EXIDX bits should be different. I assume that could be a later patch.

libunwind/src/AddressSpace.hpp
465

FWIW: None of the UnwindInfoSections fields are used if LocalAddressSpace::findUnwindSections returns false. I removed this line because it's inconsistent to clear only this field, and only for the dl_iterate_phdr codepath.

rprichard updated this revision to Diff 292154.Sep 16 2020, 2:26 AM

Stylistic change.

rprichard edited the summary of this revision. (Show Details)Sep 16 2020, 2:26 AM

Adding @saugustine -- this patch makes the frame header cache one word larger per entry.

compnerd requested changes to this revision.Sep 16 2020, 7:46 AM

I think that tracking the segment length should be okay. It’s a total overhead of 8 bytes, which is only allocated once, however, the naming here is egregiously bad. This is the segment length and not the DSO length nor the DSO mapped length (which may actually be larger or smaller based on the load segment layout).

libunwind/src/AddressSpace.hpp
417

This seems like a bad name. This is the segment length, not the DSO length. There can be multiple PT_LOAD segments.

libunwind/test/frameheadercache_test.pass.cpp
19

This also shouldn’t be called DSO Length,

This revision now requires changes to proceed.Sep 16 2020, 7:46 AM
rprichard updated this revision to Diff 292373.Sep 16 2020, 4:29 PM

Rename dso_length to text_segment_length.

compnerd accepted this revision.Sep 16 2020, 4:32 PM

Thanks!

This revision is now accepted and ready to land.Sep 16 2020, 4:32 PM

This seems like a bad name.

Good point. How about text_segment_length?

I think that tracking the segment length should be okay. It’s a total overhead of 8 bytes, which is only allocated once, however, the naming here is egregiously bad. This is the segment length and not the DSO length nor the DSO mapped length (which may actually be larger or smaller based on the load segment layout).

It's 8 bytes, per cache entry (8 entries), and with the Android NDK, each shared object would have its own copy of the cache. Happily, the Android platform itself now has only a single copy of the unwinder in libc.so, which mitigates the waste. I'd still prefer to refactor the waste away (e.g. maybe by removing UnwindInfoSections from FrameHeaderCache::CacheEntry and inlining just what's needed).

The DwarfFDECache waste is a worse. In practice, it will always be empty, and it's adding 2KiB of .bss in every DSO. That's easy to fix by lowering the number of elements in _initialBuffer[64].

This revision was landed with ongoing or failed builds.Sep 16 2020, 7:02 PM
This revision was automatically updated to reflect the committed changes.