[libunwind] Replace chain-of-ifdefs for dl_iterate_phdr

[rprichard]

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

[saugustine]

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]

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

libunwind/src/AddressSpace.hpp

 
 #elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
 
 // When statically linked on bare-metal, the symbols for the EH table are looked
 // up without going through the dynamic loader.
 extern char __exidx_start;
 extern char __exidx_end;
 
-#elif defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
 
-// ELF-based systems may use dl_iterate_phdr() to access sections
-// containing unwinding information. The ElfW() macro for pointer-size
-// independent ELF header traversal is not provided by <link.h> on some
-// systems (e.g., FreeBSD). On these systems the data structures are
-// just called Elf_XXX. Define ElfW() locally.
-#ifndef _WIN32
-#include <link.h>
-#else
 #include <windows.h>
 #include <psapi.h>
-#endif
-#if !defined(ElfW)
-#define ElfW(type) Elf_##type
-#endif
+
+#elif defined(_LIBUNWIND_USE_DL_ITERATE_PHDR) ||                               \
+      defined(_LIBUNWIND_USE_DL_UNWIND_FIND_EXIDX)
+
+#include <link.h>
 
 #endif
 
 namespace libunwind {
 
 /// Used by findUnwindSections() to return info about needed sections.
 struct UnwindInfoSections {
 #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) || defined(_LIBUNWIND_SUPPORT_DWARF_INDEX) ||       \
LocalAddressSpace::getEncodedP(pint_t &addr, pint_t end, uint8_t encoding,
   }
 
   if (encoding & DW_EH_PE_indirect)
     result = getP(result);
 
   return result;
 }
 
-#ifdef __APPLE__
-#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
-#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
-#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
-#elif defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && defined(_WIN32)
-#elif defined(_LIBUNWIND_ARM_EHABI) && defined(__BIONIC__)
-// Code inside findUnwindSections handles all these cases.
-//
-// Although the above ifdef chain is ugly, there doesn't seem to be a cleaner
-// way to handle it. The generalized boolean expression is:
-//
-//  A OR (B AND C) OR (D AND C) OR (B AND E) OR (F AND E) OR (D AND G)
-//
-// Running it through various boolean expression simplifiers gives expressions
-// that don't help at all.
-#elif defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+#if defined(_LIBUNWIND_USE_DL_ITERATE_PHDR)
 
+// The ElfW() macro for pointer-size independent ELF header traversal is not
+// provided by <link.h> on some systems (e.g., FreeBSD). On these systems the
+// data structures are just called Elf_XXX. Define ElfW() locally.
+#if !defined(ElfW)
+  #define ElfW(type) Elf_##type
+#endif
 #if !defined(Elf_Half)
   typedef ElfW(Half) Elf_Half;
 #endif
 #if !defined(Elf_Phdr)
   typedef ElfW(Phdr) Elf_Phdr;
 #endif
 #if !defined(Elf_Addr)
   typedef ElfW(Addr) Elf_Addr;

 #endif
       return 1;
     }
   }
   cbdata->sects->dwarf_section_length = 0;
   return 0;
 }
 
-#else  // defined(LIBUNWIND_SUPPORT_DWARF_UNWIND)
-// Given all the #ifdef's above, the code here is for
-// defined(LIBUNWIND_ARM_EHABI)
+#elif defined(_LIBUNWIND_ARM_EHABI)
 
 static int findUnwindSectionsByPhdr(struct dl_phdr_info *pinfo, size_t,
                                     void *data) {
   auto *cbdata = static_cast<dl_iterate_cb_data *>(data);
   bool found_obj = false;
   bool found_hdr = false;
 
   assert(cbdata);
if (phdr->p_type == PT_LOAD) {
       uintptr_t exidx_start = image_base + phdr->p_vaddr;
       cbdata->sects->arm_section = exidx_start;
       cbdata->sects->arm_section_length = phdr->p_memsz;
       found_hdr = true;
     }
   }
   return found_obj && found_hdr;
 }
-#endif  // defined(LIBUNWIND_SUPPORT_DWARF_UNWIND)
-#endif  // defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+
+#endif
+#endif  // defined(_LIBUNWIND_USE_DL_ITERATE_PHDR)
 
 
 inline bool LocalAddressSpace::findUnwindSections(pint_t targetAddr,
                                                   UnwindInfoSections &info) {
 #ifdef __APPLE__
   dyld_unwind_sections dyldInfo;
   if (_dyld_find_unwind_sections((void *)targetAddr, &dyldInfo)) {
     info.dso_base                      = (uintptr_t)dyldInfo.mh;
#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
   }
   return false;
 #elif defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && defined(_WIN32)
   // Don't even bother, since Windows has functions that do all this stuff
   // for us.
   (void)targetAddr;
   (void)info;
   return true;
-#elif defined(_LIBUNWIND_ARM_EHABI) && defined(__BIONIC__)
-  // For ARM EHABI, Bionic didn't implement dl_iterate_phdr until API 21. After
-  // API 21, dl_iterate_phdr exists, but dl_unwind_find_exidx is much faster.
+#elif defined(_LIBUNWIND_USE_DL_UNWIND_FIND_EXIDX)
   int length = 0;
   info.arm_section =
       (uintptr_t)dl_unwind_find_exidx((_Unwind_Ptr)targetAddr, &length);
   info.arm_section_length = (uintptr_t)length * sizeof(EHABIIndexEntry);
   if (info.arm_section && info.arm_section_length)
     return true;
-#elif defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+#elif defined(_LIBUNWIND_USE_DL_ITERATE_PHDR)
   dl_iterate_cb_data cb_data = {this, &info, targetAddr};
   int found = dl_iterate_phdr(findUnwindSectionsByPhdr, &cb_data);
   return static_cast<bool>(found);
 #endif
 
   return false;
 }
 

libunwind/src/config.h

#else
     #define _LIBUNWIND_SUPPORT_DWARF_UNWIND 1
   #endif
 #elif defined(_WIN32)
   #ifdef __SEH__
     #define _LIBUNWIND_SUPPORT_SEH_UNWIND 1
   #else
     #define _LIBUNWIND_SUPPORT_DWARF_UNWIND 1
   #endif
+#elif defined(_LIBUNWIND_IS_BAREMETAL)
+  #if !defined(_LIBUNWIND_ARM_EHABI)
+    #define _LIBUNWIND_SUPPORT_DWARF_UNWIND 1
+    #define _LIBUNWIND_SUPPORT_DWARF_INDEX 1
+  #endif
+#elif defined(__BIONIC__) && defined(_LIBUNWIND_ARM_EHABI)
+  // For ARM EHABI, Bionic didn't implement dl_iterate_phdr until API 21. After
+  // API 21, dl_iterate_phdr exists, but dl_unwind_find_exidx is much faster.
+  #define _LIBUNWIND_USE_DL_UNWIND_FIND_EXIDX 1
 #else
+  // Assume an ELF system with a dl_iterate_phdr function.
+  #define _LIBUNWIND_USE_DL_ITERATE_PHDR 1
   #if !defined(_LIBUNWIND_ARM_EHABI)
     #define _LIBUNWIND_SUPPORT_DWARF_UNWIND 1
     #define _LIBUNWIND_SUPPORT_DWARF_INDEX 1
   #endif
 #endif
 
 #if defined(_LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS)
   #define _LIBUNWIND_EXPORT

libunwind/test/frameheadercache_test.pass.cpp

 // The other libunwind tests don't test internal interfaces, so the include path
 // is a little wonky.
 #include "../src/config.h"
 
 // Only run this test under supported configurations.
-// The frame header cache should work fine for other architectures,
-// but the #ifdefs end up being even more complicated than this.
 
-#if defined(__x86_64__) && defined(_LIBUNWIND_USE_FRAME_HEADER_CACHE)
-
-// This #if chain is ugly, but see the comments in AddressSpace.hpp for
-// the reasoning.
-
-#ifdef __APPLE__
-int main() { return 0; }
-#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
-int main() { return 0; }
-#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
-int main() { return 0; }
-#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
-int main() { return 0; }
-#elif defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && defined(_WIN32)
-int main() { return 0; }
-#elif defined(_LIBUNWIND_ARM_EHABI) && defined(__BIONIC__)
-int main() { return 0; }
-#elif defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+#if defined(_LIBUNWIND_USE_DL_ITERATE_PHDR) &&                                 \
+    defined(_LIBUNWIND_SUPPORT_DWARF_INDEX) &&                                 \
+    defined(_LIBUNWIND_USE_FRAME_HEADER_CACHE)
 
 #include <link.h>
 #include <stdio.h>
 
 // This file defines several of the data structures needed here,
 // and includes FrameHeaderCache.hpp as well.
 #include "../src/AddressSpace.hpp"
 
for (int i = 0; i < 9; i++) {
     FHC.add(&UIS);
   }
   CBData.targetAddr = kBaseAddr;
   // Should have been evicted.
   if (FHC.find(&PInfo, 0, &CBData))
     abort();
   return 0;
 }
-#else
-int main() { return 0; }
-#endif
+
 #else
 int main() { return 0;}
 #endif