Page MenuHomePhabricator

[libunwind] Minor fixes in libunwind
ClosedPublic

Authored by rprichard on Aug 19 2020, 5:10 PM.

Details

Summary
  • When _LIBUNWIND_SUPPORT_COMPACT_UNWIND is defined in config.h, define it to "1" like the other macros. These macros are still checked using "#if defined(...)", however.
  • Include libunwind.h in AddressSpace.hpp before using _LIBUNWIND_ARM_EHABI.
  • Rename ProcessFrameHeaderCache to TheFrameHeaderCache, because some configurations (e.g. Android / hermetic static libraries) can have one cache per shared object in the process. (When there are more copies, it's more important not to waste memory in the cache.)
  • Add 3 missing header files to LIBUNWIND_HEADERS.

Diff Detail

Event Timeline

rprichard created this revision.Aug 19 2020, 5:10 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 19 2020, 5:10 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard requested review of this revision.Aug 19 2020, 5:10 PM
rprichard updated this revision to Diff 286690.Aug 19 2020, 5:18 PM

Revert edit that crept in from a later patch: SECTION -> UNWIND.

saugustine accepted this revision.Aug 19 2020, 5:19 PM

Looks good from the FrameHeaderCache side.

Can you please verify that findUnwindSectionsByPhdr was not exposed previously? This seems like it could be an ABI break.

There is a libunwind::findUnwindSectionsByPhdr as of 11.x. When I build 11.x libunwind.so for glibc (I tested arm and x86_64), the symbol is exposed:

$ readelf --dyn-syms unwind-arm/lib/libunwind.so.1.0 | grep findUnwind
    35: 00000f24   204 FUNC    GLOBAL DEFAULT    11 _ZN9libunwind24findUnwindSectionsByPhdrEP12dl_phdr_infojPv

$ readelf --dyn-syms unwind-x86_64/lib/libunwind.so.1.0 | grep findUnwind
    46: 0000000000002c30   854 FUNC    GLOBAL DEFAULT    12 _ZN9libunwind24findUnwindSectionsByPhdrEP12dl_phdr_infomPv

In previous releases, the function was a lambda instead, and the lambda was a hidden symbol (because LocalAddressSpace is marked with _LIBUNWIND_HIDDEN, which is __attribute__((visibility("hidden"))).)

$ readelf -s unwind-arm/src/CMakeFiles/unwind_shared.dir/libunwind.cpp.o | grep findUnwind
     5: 00000000     0 SECTION LOCAL  DEFAULT    23 .text._ZZN9libunwind17LocalAddressSpace18findUnwindSectionsEjRNS_18UnwindInfoSectionsEENUlP12dl_phdr_infojPvE_4_FUNES4_jS5_
     7: 00000000     0 SECTION LOCAL  DEFAULT    24 .ARM.extab.text._ZZN9libunwind17LocalAddressSpace18findUnwindSectionsEjRNS_18UnwindInfoSectionsEENUlP12dl_phdr_infojPvE_4_FUNES4_jS5_
     8: 00000000     0 SECTION LOCAL  DEFAULT    25 .ARM.exidx.text._ZZN9libunwind17LocalAddressSpace18findUnwindSectionsEjRNS_18UnwindInfoSectionsEENUlP12dl_phdr_infojPvE_4_FUNES4_jS5_
   265: 00000000   204 FUNC    WEAK   HIDDEN     23 _ZZN9libunwind17LocalAddressSpace18findUnwindSectionsEjRNS_18UnwindInfoSectionsEENUlP12dl_phdr_infojPvE_4_FUNES4_jS5_

$ readelf -s unwind-x86_64/src/CMakeFiles/unwind_shared.dir/libunwind.cpp.o | grep findUnwind
    38: 0000000000000000     0 SECTION LOCAL  DEFAULT    80 .text._ZZN9libunwind17LocalAddressSpace18findUnwindSectionsEmRNS_18UnwindInfoSectionsEENUlP12dl_phdr_infomPvE_4_FUNES4_mS5_
   249: 0000000000000000   313 FUNC    WEAK   HIDDEN     80 _ZZN9libunwind17LocalAddressSpace18findUnwindSectionsEmRNS_18UnwindInfoSectionsEENUlP12dl_phdr_infomPvE_4_FUNES4_mS5_

The hidden symbol doesn't make it into the shared object's .dynsym table.

11.0.0-final isn't released yet, so maybe this can be fixed still?

I suspect this function is currently only ever defined in libunwind.cpp.

compnerd accepted this revision.Aug 21 2020, 8:50 AM
compnerd added a subscriber: hans.

Please ensure that the fix goes into 11 before release (CC: @hans), but the remainder of the change is fine.

This revision is now accepted and ready to land.Aug 21 2020, 8:50 AM
rprichard updated this revision to Diff 287109.Aug 21 2020, 2:28 PM

Break the "Make findUnwindSectionsByPhdr static" change into its own patch.

rprichard updated this revision to Diff 287111.Aug 21 2020, 2:31 PM
rprichard edited the summary of this revision. (Show Details)

Update summary.

hans added a comment.Aug 24 2020, 11:00 AM

Please ensure that the fix goes into 11 before release (CC: @hans), but the remainder of the change is fine.

Just to make sure I get it right, the fix need for 11 is https://reviews.llvm.org/D86372, right?

Please ensure that the fix goes into 11 before release (CC: @hans), but the remainder of the change is fine.

Just to make sure I get it right, the fix need for 11 is https://reviews.llvm.org/D86372, right?

Correct. Only D86372, containing the findUnwindSectionsByPhdr change, is needed for 11. After I broke that change out, this differential revision (D86254) isn't needed for 11 anymore.

This revision was landed with ongoing or failed builds.Aug 26 2020, 6:24 PM
This revision was automatically updated to reflect the committed changes.