[libcxx-commits] [PATCH] D86768: [libunwind] Replace chain-of-ifdefs for dl_iterate_phdr

[Ryan Prichard via Phabricator via libcxx-commits]

rprichard updated this revision to Diff 289634.
rprichard added a comment.

In D86768#2253572 <https://reviews.llvm.org/D86768#2253572>, @saugustine wrote:

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86768/new/

https://reviews.llvm.org/D86768

Files:
  libunwind/src/AddressSpace.hpp
  libunwind/src/config.h
  libunwind/test/frameheadercache_test.pass.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86768.289634.patch
Type: text/x-patch
Size: 6453 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20200903/3bdf3ff1/attachment.bin>