Promote nameless lambda used by dl_iterate_phdr to named function to clean up control flow inside findUnwindSections. Also, expose the data structureto allow use by a future replacment function.

[saugustine]

Summary

[Refactor] Promote nameless lambda to fully named function, allowing easy replacement in following patch.

[echristo]

LGTM.

[MaskRay]

So findUnwindSectionByPhdr was the lambda. Makes sense.

[mstorsjo]

This broke building for windows, and I would be pretty sure it also broke things for apple platforms as well (as _LIBUNWIND_SUPPORT_DWARF_UNWIND is defined on both of them, at least on certain architectures).

There's a long chain of ifdefs in LocalAddressSpace::findUnwindSections, like this:

#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__)
#elif defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
// Current ELF code being refactored
#endif

Now after this change, you've placed a separate #if defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) standalone above all of it - while _LIBUNWIND_SUPPORT_DWARF_UNWIND is defined in a lot of other cases as well (both on apple targets and for some architectures on win32), that are handled by other more specialised OS specific ifdefs in LocalAddressSpace::findUnwindSections.

Not sure what the cleanest solution would be though. The initial thought would be to add && !defined(__APPLE__) && !defined(_WIN32) && !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(__BIONIC__) to the ifdef surrounding findUnwindSectionByPhdr, but that doesn't quite exactly replicate the effect of the long series of ifdefs either. The safest (but less neat) solution would be to add the exact same series of ifdefs around findUnwindSectionByPhdr, with empty ifdef bodies for all the other cases.

[miyuki]

This also broke the build for baremetal configurations: before the patch the lambda was ifdef-ed out for cases like

#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
...
#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
...

After the patch the code in the findUnwindSectionByPhdr function causes compile-time errors because of undefined symbols (e.g. ElfW) and accesses to the incomplete type dl_phdr_info. As the previous comment suggested, findUnwindSectionByPhdr should be ifdef-ed out when _LIBUNWIND_IS_BAREMETAL is defined.

[saugustine]

Sorry for the trouble. Trying again in D75637. There really doesn't seem to be a clean way of #ifdeffing this.

libunwind/src/AddressSpace.hpp

LocalAddressSpace::getEncodedP(pint_t &addr, pint_t end, uint8_t encoding,
   }
 
   if (encoding & DW_EH_PE_indirect)
     result = getP(result);
 
   return result;
 }
 
-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;
- #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
-    info.dwarf_section                 = (uintptr_t)dyldInfo.dwarf_section;
-    info.dwarf_section_length          = dyldInfo.dwarf_section_length;
- #endif
-    info.compact_unwind_section        = (uintptr_t)dyldInfo.compact_unwind_section;
-    info.compact_unwind_section_length = dyldInfo.compact_unwind_section_length;
-    return true;
-  }
-#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
-  // Bare metal is statically linked, so no need to ask the dynamic loader
-  info.dwarf_section_length = (uintptr_t)(&__eh_frame_end - &__eh_frame_start);
-  info.dwarf_section =        (uintptr_t)(&__eh_frame_start);
-  _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: section %p length %p",
-                             (void *)info.dwarf_section, (void *)info.dwarf_section_length);
-#if defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
-  info.dwarf_index_section =        (uintptr_t)(&__eh_frame_hdr_start);
-  info.dwarf_index_section_length = (uintptr_t)(&__eh_frame_hdr_end - &__eh_frame_hdr_start);
-  _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: index section %p length %p",
-                             (void *)info.dwarf_index_section, (void *)info.dwarf_index_section_length);
-#endif
-  if (info.dwarf_section_length)
-    return true;
-#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
-  // Bare metal is statically linked, so no need to ask the dynamic loader
-  info.arm_section =        (uintptr_t)(&__exidx_start);
-  info.arm_section_length = (uintptr_t)(&__exidx_end - &__exidx_start);
-  _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: section %p length %p",
-                             (void *)info.arm_section, (void *)info.arm_section_length);
-  if (info.arm_section && info.arm_section_length)
-    return true;
-#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
-  HMODULE mods[1024];
-  HANDLE process = GetCurrentProcess();
-  DWORD needed;
-
-  if (!EnumProcessModules(process, mods, sizeof(mods), &needed)) {
-    DWORD err = GetLastError();
-    _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: EnumProcessModules failed, "
-                               "returned error %d", (int)err);
-    return false;
-  }
-
-  for (unsigned i = 0; i < (needed / sizeof(HMODULE)); i++) {
-    PIMAGE_DOS_HEADER pidh = (PIMAGE_DOS_HEADER)mods[i];
-    PIMAGE_NT_HEADERS pinh = (PIMAGE_NT_HEADERS)((BYTE *)pidh + pidh->e_lfanew);
-    PIMAGE_FILE_HEADER pifh = (PIMAGE_FILE_HEADER)&pinh->FileHeader;
-    PIMAGE_SECTION_HEADER pish = IMAGE_FIRST_SECTION(pinh);
-    bool found_obj = false;
-    bool found_hdr = false;
-
-    info.dso_base = (uintptr_t)mods[i];
-    for (unsigned j = 0; j < pifh->NumberOfSections; j++, pish++) {
-      uintptr_t begin = pish->VirtualAddress + (uintptr_t)mods[i];
-      uintptr_t end = begin + pish->Misc.VirtualSize;
-      if (!strncmp((const char *)pish->Name, ".text",
-                   IMAGE_SIZEOF_SHORT_NAME)) {
-        if (targetAddr >= begin && targetAddr < end)
-          found_obj = true;
-      } else if (!strncmp((const char *)pish->Name, ".eh_frame",
-                          IMAGE_SIZEOF_SHORT_NAME)) {
-        info.dwarf_section = begin;
-        info.dwarf_section_length = pish->Misc.VirtualSize;
-        found_hdr = true;
-      }
-      if (found_obj && found_hdr)
-        return true;
-    }
-  }
-  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.
-  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)
-  struct dl_iterate_cb_data {
-    LocalAddressSpace *addressSpace;
-    UnwindInfoSections *sects;
-    uintptr_t targetAddr;
-  };
+#if defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+struct _LIBUNWIND_HIDDEN dl_iterate_cb_data {
 
-  dl_iterate_cb_data cb_data = {this, &info, targetAddr};
-  int found = dl_iterate_phdr(
-      [](struct dl_phdr_info *pinfo, size_t, void *data) -> int {
-        auto cbdata = static_cast<dl_iterate_cb_data *>(data);
-        bool found_obj = false;
-        bool found_hdr = false;
+static int findUnwindSectionByPhdr(struct dl_phdr_info *pinfo, size_t, void *data) {
 
-        assert(cbdata);
-        assert(cbdata->sects);
 
-        if (cbdata->targetAddr < pinfo->dlpi_addr) {
-          return false;
-        }
 
 #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
 
-        Elf_Addr image_base = pinfo->dlpi_addr;
 
 #if defined(__ANDROID__) && __ANDROID_API__ < 18
-        if (image_base == 0) {
-          // Normally, an image base of 0 indicates a non-PIE executable. On
-          // versions of Android prior to API 18, the dynamic linker reported a
-          // dlpi_addr of 0 for PIE executables. Compute the true image base
-          // using the PT_PHDR segment.
-          // See https://github.com/android/ndk/issues/505.
-          for (Elf_Half i = 0; i < pinfo->dlpi_phnum; i++) {
-            const Elf_Phdr *phdr = &pinfo->dlpi_phdr[i];
-            if (phdr->p_type == PT_PHDR) {
-              image_base = reinterpret_cast<Elf_Addr>(pinfo->dlpi_phdr) -
-                  phdr->p_vaddr;
-              break;
-            }
-          }
-        }
 #endif
 
   #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
     #if !defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
      #error "_LIBUNWIND_SUPPORT_DWARF_UNWIND requires _LIBUNWIND_SUPPORT_DWARF_INDEX on this platform."
    #endif
-        size_t object_length;
 
-        for (Elf_Half i = 0; i < pinfo->dlpi_phnum; i++) {
-          const Elf_Phdr *phdr = &pinfo->dlpi_phdr[i];
-          if (phdr->p_type == PT_LOAD) {
-            uintptr_t begin = image_base + phdr->p_vaddr;
-            uintptr_t end = begin + phdr->p_memsz;
-            if (cbdata->targetAddr >= begin && cbdata->targetAddr < end) {
-              cbdata->sects->dso_base = begin;
-              object_length = phdr->p_memsz;
-              found_obj = true;
-            }
-          } else if (phdr->p_type == PT_GNU_EH_FRAME) {
-            EHHeaderParser<LocalAddressSpace>::EHHeaderInfo hdrInfo;
-            uintptr_t eh_frame_hdr_start = image_base + phdr->p_vaddr;
-            cbdata->sects->dwarf_index_section = eh_frame_hdr_start;
-            cbdata->sects->dwarf_index_section_length = phdr->p_memsz;
-            found_hdr = EHHeaderParser<LocalAddressSpace>::decodeEHHdr(
-                *cbdata->addressSpace, eh_frame_hdr_start, phdr->p_memsz,
-                hdrInfo);
-            if (found_hdr)
-              cbdata->sects->dwarf_section = hdrInfo.eh_frame_ptr;
-          }
-        }
 
-        if (found_obj && found_hdr) {
-          cbdata->sects->dwarf_section_length = object_length;
-          return true;
-        } else {
-          return false;
-        }
  #else // defined(_LIBUNWIND_ARM_EHABI)
-        for (Elf_Half i = 0; i < pinfo->dlpi_phnum; i++) {
-          const Elf_Phdr *phdr = &pinfo->dlpi_phdr[i];
-          if (phdr->p_type == PT_LOAD) {
-            uintptr_t begin = image_base + phdr->p_vaddr;
-            uintptr_t end = begin + phdr->p_memsz;
-            if (cbdata->targetAddr >= begin && cbdata->targetAddr < end)
-              found_obj = true;
-          } else if (phdr->p_type == PT_ARM_EXIDX) {
-            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
-      },
-      &cb_data);
+}
+
+#endif  // defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+
+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;
+ #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+    info.dwarf_section                 = (uintptr_t)dyldInfo.dwarf_section;
+    info.dwarf_section_length          = dyldInfo.dwarf_section_length;
+ #endif
+    info.compact_unwind_section        = (uintptr_t)dyldInfo.compact_unwind_section;
+    info.compact_unwind_section_length = dyldInfo.compact_unwind_section_length;
+    return true;
+  }
+#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
+  // Bare metal is statically linked, so no need to ask the dynamic loader
+  info.dwarf_section_length = (uintptr_t)(&__eh_frame_end - &__eh_frame_start);
+  info.dwarf_section =        (uintptr_t)(&__eh_frame_start);
+  _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: section %p length %p",
+                             (void *)info.dwarf_section, (void *)info.dwarf_section_length);
+#if defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
+  info.dwarf_index_section =        (uintptr_t)(&__eh_frame_hdr_start);
+  info.dwarf_index_section_length = (uintptr_t)(&__eh_frame_hdr_end - &__eh_frame_hdr_start);
+  _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: index section %p length %p",
+                             (void *)info.dwarf_index_section, (void *)info.dwarf_index_section_length);
+#endif
+  if (info.dwarf_section_length)
+    return true;
+#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
+  // Bare metal is statically linked, so no need to ask the dynamic loader
+  info.arm_section =        (uintptr_t)(&__exidx_start);
+  info.arm_section_length = (uintptr_t)(&__exidx_end - &__exidx_start);
+  _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: section %p length %p",
+                             (void *)info.arm_section, (void *)info.arm_section_length);
+  if (info.arm_section && info.arm_section_length)
+    return true;
+#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
+  HMODULE mods[1024];
+  HANDLE process = GetCurrentProcess();
+  DWORD needed;
+
+  if (!EnumProcessModules(process, mods, sizeof(mods), &needed)) {
+    DWORD err = GetLastError();
+    _LIBUNWIND_TRACE_UNWINDING("findUnwindSections: EnumProcessModules failed, "
+                               "returned error %d", (int)err);
+    return false;
+  }
+
+  for (unsigned i = 0; i < (needed / sizeof(HMODULE)); i++) {
+    PIMAGE_DOS_HEADER pidh = (PIMAGE_DOS_HEADER)mods[i];
+    PIMAGE_NT_HEADERS pinh = (PIMAGE_NT_HEADERS)((BYTE *)pidh + pidh->e_lfanew);
+    PIMAGE_FILE_HEADER pifh = (PIMAGE_FILE_HEADER)&pinh->FileHeader;
+    PIMAGE_SECTION_HEADER pish = IMAGE_FIRST_SECTION(pinh);
+    bool found_obj = false;
+    bool found_hdr = false;
+
+    info.dso_base = (uintptr_t)mods[i];
+    for (unsigned j = 0; j < pifh->NumberOfSections; j++, pish++) {
+      uintptr_t begin = pish->VirtualAddress + (uintptr_t)mods[i];
+      uintptr_t end = begin + pish->Misc.VirtualSize;
+      if (!strncmp((const char *)pish->Name, ".text",
+                   IMAGE_SIZEOF_SHORT_NAME)) {
+        if (targetAddr >= begin && targetAddr < end)
+          found_obj = true;
+      } else if (!strncmp((const char *)pish->Name, ".eh_frame",
+                          IMAGE_SIZEOF_SHORT_NAME)) {
+        info.dwarf_section = begin;
+        info.dwarf_section_length = pish->Misc.VirtualSize;
+        found_hdr = true;
+      }
+      if (found_obj && found_hdr)
+        return true;
+    }
+  }
+  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.
+  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)
+  dl_iterate_cb_data cb_data = {this, &info, targetAddr};
+  int found = dl_iterate_phdr(findUnwindSectionByPhdr, &cb_data);
   return static_cast<bool>(found);
 #endif
 
   return false;
 }
 
 
 inline bool LocalAddressSpace::findOtherFDE(pint_t targetAddr, pint_t &fde) {