Default to disabling the libunwind frameheader cache

[saugustine]

Summary

Although it works fine with glibc, as currently implemented the
frameheader cache is incompatible with certain platforms with
slightly different locking semantics inside dl_iterate_phdr.

Therefore only enable it when it is turned on explicitly with
a configure-time option.

[emaste]

Looks reasonable to me.

For reference here's a FreeBSD review for dl_iterate_phdr: provide exclusive locking for callback when statically linked.

[emaste]

Ah if you use _LIBUNWIND_USE_FRAME_HEADER_CACHE instead (i.e., a _ between FRAME and HEADER) it will match the workaround committed to FreeBSD.

[jgorbe]

LGTM

[saugustine]

Rework to match bsd workaround.

[saugustine]

Now just waiting on a libunwind group reviewer.

[mstorsjo]

So, this disables code that currently is enabled, unless it's explicitly requested to be enabled? Sounds good to me, but the subject line sounds to me like it allows enabling something that currently is disabled.

LGTM with the subject line rephrased.

[saugustine]

Update for comment.

[rprichard]

Should we cherry-pick this change to 11.x to help with https://bugs.llvm.org/show_bug.cgi?id=46743? The cache also relies on the dlpi_adds / dlpi_subs fields that are only added in Android R (the upcoming version).

[hans]

In D86163#2225711, @rprichard wrote:

Should we cherry-pick this change to 11.x to help with https://bugs.llvm.org/show_bug.cgi?id=46743? The cache also relies on the dlpi_adds / dlpi_subs fields that are only added in Android R (the upcoming version).

Yes, and also for https://bugs.llvm.org/show_bug.cgi?id=47181

Cherry-picked as 33c13cd8c5772871ec79f0b061e7f7a2fb287383.

libunwind/CMakeLists.txt

 option(LIBUNWIND_ENABLE_STATIC "Build libunwind as a static library." ON)
 option(LIBUNWIND_ENABLE_CROSS_UNWINDING "Enable cross-platform unwinding support." OFF)
 option(LIBUNWIND_ENABLE_ARM_WMMX "Enable unwinding support for ARM WMMX registers." OFF)
 option(LIBUNWIND_ENABLE_THREADS "Build libunwind with threading support." ON)
 option(LIBUNWIND_WEAK_PTHREAD_LIB "Use weak references to refer to pthread functions." OFF)
 option(LIBUNWIND_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 option(LIBUNWIND_INCLUDE_DOCS "Build the libunwind documentation." ${LLVM_INCLUDE_DOCS})
 option(LIBUNWIND_IS_BAREMETAL "Build libunwind for baremetal targets." OFF)
+option(LIBUNWIND_USE_FRAMEHEADER_CACHE "Cache frame headers for unwinding. Requires locking dl_iterate_phdr." OFF)
 
 set(LIBUNWIND_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
     "Define suffix of library directory name (32/64)")
 option(LIBUNWIND_INSTALL_LIBRARY "Install the libunwind library." ON)
 cmake_dependent_option(LIBUNWIND_INSTALL_STATIC_LIBRARY
   "Install the static libunwind library." ON
   "LIBUNWIND_ENABLE_STATIC;LIBUNWIND_INSTALL_LIBRARY" OFF)
 cmake_dependent_option(LIBUNWIND_INSTALL_SHARED_LIBRARY
if (LIBUNWIND_ENABLE_ARM_WMMX)
   # unwinder.
   add_compile_flags(-D__ARM_WMMX)
 endif()
 
 if(LIBUNWIND_IS_BAREMETAL)
   add_compile_definitions(_LIBUNWIND_IS_BAREMETAL)
 endif()
 
+if(LIBUNWIND_USE_FRAMEHEADER_CACHE)
+  add_compile_definitions(_LIBUNWIND_USE_FRAMEHEADER_CACHE)
+endif()
+
 # This is the _ONLY_ place where add_definitions is called.
 if (MSVC)
   add_definitions(-D_CRT_SECURE_NO_WARNINGS)
 endif()
 
 # Disable DLL annotations on Windows for static builds.
 if (WIN32 AND LIBUNWIND_ENABLE_STATIC AND NOT LIBUNWIND_ENABLE_SHARED)
   add_definitions(-D_LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS)

libunwind/src/AddressSpace.hpp

struct _LIBUNWIND_HIDDEN dl_iterate_cb_data {
   uintptr_t targetAddr;
 };
 
 #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
 
+#if defined(_LIBUNWIND_USE_FRAMEHEADER_CACHE)
 #include "FrameHeaderCache.hpp"
 
 // There should be just one of these per process.
 static FrameHeaderCache ProcessFrameHeaderCache;
+#endif
+
 
 static bool checkAddrInSegment(const Elf_Phdr *phdr, size_t image_base,
                                dl_iterate_cb_data *cbdata) {
   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;
       cbdata->sects->dwarf_section_length = phdr->p_memsz;
       return true;
     }
   }
   return false;
 }
 
 int findUnwindSectionsByPhdr(struct dl_phdr_info *pinfo, size_t pinfo_size,
                              void *data) {
   auto cbdata = static_cast<dl_iterate_cb_data *>(data);
   if (pinfo->dlpi_phnum == 0 || cbdata->targetAddr < pinfo->dlpi_addr)
     return 0;
+#if defined(_LIBUNWIND_USE_FRAMEHEADER_CACHE)
   if (ProcessFrameHeaderCache.find(pinfo, pinfo_size, data))
     return 1;
+#endif
 
   Elf_Addr image_base = calculateImageBase(pinfo);
   bool found_obj = false;
   bool found_hdr = false;
 
   // Third phdr is usually the executable phdr.
   if (pinfo->dlpi_phnum > 2)
     found_obj = checkAddrInSegment(&pinfo->dlpi_phdr[2], image_base, cbdata);
if (!found_hdr && phdr->p_type == PT_GNU_EH_FRAME) {
           *cbdata->addressSpace, eh_frame_hdr_start, phdr->p_memsz,
           hdrInfo);
       if (found_hdr)
         cbdata->sects->dwarf_section = hdrInfo.eh_frame_ptr;
     } else if (!found_obj) {
       found_obj = checkAddrInSegment(phdr, image_base, cbdata);
     }
     if (found_obj && found_hdr) {
+#if defined(_LIBUNWIND_USE_FRAMEHEADER_CACHE)
       ProcessFrameHeaderCache.add(cbdata->sects);
+#endif
       return 1;
     }
   }
   cbdata->sects->dwarf_section_length = 0;
   return 0;
 }
 
 #else  // defined(LIBUNWIND_SUPPORT_DWARF_UNWIND)

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.
 
-#ifdef __x86_64__
+#if defined(__x86_64__) && defined(_LIBUNWIND_USE_FRAMEHEADER_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; }