Page MenuHomePhabricator

[libunwind] Replace chain-of-ifdefs for dl_iterate_phdr
ClosedPublic

Authored by rprichard on Aug 28 2020, 12:52 AM.

Details

Summary

Define a _LIBUNWIND_USE_DL_ITERATE_PHDR macro in config.h when there is
no other unwind info lookup method. Also define a
_LIBUNWIND_USE_DL_UNWIND_FIND_EXIDX macro to factor out
(BIONIC and _LIBUNWIND_ARM_EHABI).

Diff Detail

Event Timeline

rprichard created this revision.Aug 28 2020, 12:52 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 28 2020, 12:52 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard requested review of this revision.Aug 28 2020, 12:52 AM
saugustine accepted this revision.Sep 2 2020, 5:03 PM

I am going to accept this, since no one else seems to be able to review it. But the problem here is that no one really knows what a simplified ifdef chain with the same functionality looks like. I tried a couple of times and failed miserably.

So if this works, great! If not, we will need to revert it quickly.

rprichard updated this revision to Diff 289634.Sep 2 2020, 10:13 PM

I am going to accept this, since no one else seems to be able to review it. But the problem here is that no one really knows what a simplified ifdef chain with the same functionality looks like. I tried a couple of times and failed miserably.

So if this works, great! If not, we will need to revert it quickly.

Sounds good to me.

I looked over the change myself and noticed that I had changed the behavior for the combination of:

defined(__BIONIC__) && defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)

I wonder if bare-metal mode is supported when an operating-system macro is also defined (e.g. __APPLE__, _WIN32, __BIONIC__). Previously, that combination did nothing for __APPLE__, but maybe it could be used with _WIN32 or __BIONIC__. I decided to move the _LIBUNWIND_IS_BAREMETAL clause up above the Bionic EHABI special-case, which I believe preserves the previous behavior.

I also had changed AddressSpace.hpp to include the two Windows headers for SEH-mode, whereas previously they weren't included. I decided to stop including them unless DWARF-mode is enabled.

I went through this change for each possible configuration, and I believe this change preserves behavior:

  • If __APPLE__ is defined, nothing changes.
  • Otherwise, if _WIN32 is defined, then either _LIBUNWIND_SUPPORT_SEH_UNWIND or _LIBUNWIND_SUPPORT_DWARF_UNWIND is defined.
    • AddressSpace.hpp won't define ElfW anymore, but it isn't needed.
    • The defined(_WIN32) && defined(_LIBUNWIND_ARM_EHABI) configuration stops including windows.h and pslink.h, but they aren't needed. I audited other _WIN32 uses in libunwind, and I believe that windows.h is included where necessary. (I suspect the EHABI macro shouldn't be enabled for arm32 Windows, but perhaps it's possible with Clang, when it uses SEH.)
  • Otherwise, if _LIBUNWIND_IS_BAREMETAL is defined, then there is no change in config.h.
    • Either one of these is defined:
      • _LIBUNWIND_ARM_EHABI, or
      • _LIBUNWIND_SUPPORT_DWARF_UNWIND and _LIBUNWIND_SUPPORT_DWARF_INDEX
    • AddressSpace.hpp stops including link.h, and stops defining ElfW, but they aren't needed, so it's OK.
  • Otherwise, if __BIONIC__ and _LIBUNWIND_ARM_EHABI are defined:
    • Now _LIBUNWIND_USE_DL_UNWIND_FIND_EXIDX is defined in config.h.
    • In AddressSpace.hpp, link.h is still included, but ElfW isn't defined (and isn't needed).
  • Otherwise:
    • Now _LIBUNWIND_USE_DL_ITERATE_PHDR is defined.
    • As before, either one of these is defined:
      • _LIBUNWIND_ARM_EHABI, or
      • _LIBUNWIND_SUPPORT_DWARF_UNWIND and _LIBUNWIND_SUPPORT_DWARF_INDEX
    • The definition of ElfW moves down, but nothing else changes in AddressSpace.hpp.
This revision was not accepted when it landed; it landed in state Needs Review.Sep 8 2020, 3:50 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.