[libunwind][DWARF] Fix end of .eh_frame calculation

[rprichard]

Summary

• When .eh_frame is located using .eh_frame_hdr (PT_GNU_EH_FRAME), the start of .eh_frame is known, but not the size. In this case, the unwinder must rely on a terminator present at the end of .eh_frame. Set dwarf_section_length to UINTPTR_MAX to indicate this.
• Add a new field, dso_length, that the FrameHeaderCache uses to track to size of the PT_LOAD segment indicated by dso_base.
• Compute ehSectionEnd by adding sectionLength to ehSectionStart, never to fdeHint.

Fixes PR46829.

[rprichard]

An alternative to D86256.

It adds another field to UnwindInfoSections, so the cache will use more memory:

• This can obviously be avoided, because dwarf_section_length is now always initialized to UINTPTR_MAX when _LIBUNWIND_USE_DL_ITERATE_PHDR and _LIBUNWIND_SUPPORT_DWARF_INDEX are defined.
• Even the dwarf_section field is unnecessary if we're willing to decode the .eh_frame_hdr on a cache hit. (The unwinder already decodes it again, later, because most of the fields aren't stored in UnwindInfoSections. I think decoding is fairly cheap.)
• The dwarf_index_section_length field isn't really used much, currently. It's used to bounds-check fields if they use LEB encodings. (They probably don't). It *could* be used to do more bounds checking, though.
• If this matters, I think it can be fixed later.

After disentangling dso_length from dwarf_section_length, it should be easy to combine the DWARF and EHABI versions of findUnwindSectionsByPhdr in AddressSpace.hpp. Only the PT_GNU_EH_FRAME vs PT_ARM_EXIDX bits should be different. I assume that could be a later patch.

libunwind/src/AddressSpace.hpp

465

FWIW: None of the UnwindInfoSections fields are used if LocalAddressSpace::findUnwindSections returns false. I removed this line because it's inconsistent to clear only this field, and only for the dl_iterate_phdr codepath.

[rprichard]

Stylistic change.

[rprichard]

Adding @saugustine -- this patch makes the frame header cache one word larger per entry.

[compnerd]

I think that tracking the segment length should be okay. It’s a total overhead of 8 bytes, which is only allocated once, however, the naming here is egregiously bad. This is the segment length and not the DSO length nor the DSO mapped length (which may actually be larger or smaller based on the load segment layout).

libunwind/src/AddressSpace.hpp

417

This seems like a bad name. This is the segment length, not the DSO length. There can be multiple PT_LOAD segments.

libunwind/test/frameheadercache_test.pass.cpp

19

This also shouldn’t be called DSO Length,

[rprichard]

Rename dso_length to text_segment_length.

[compnerd]

Thanks!

[rprichard]

This seems like a bad name.

Good point. How about text_segment_length?

I think that tracking the segment length should be okay. It’s a total overhead of 8 bytes, which is only allocated once, however, the naming here is egregiously bad. This is the segment length and not the DSO length nor the DSO mapped length (which may actually be larger or smaller based on the load segment layout).

It's 8 bytes, per cache entry (8 entries), and with the Android NDK, each shared object would have its own copy of the cache. Happily, the Android platform itself now has only a single copy of the unwinder in libc.so, which mitigates the waste. I'd still prefer to refactor the waste away (e.g. maybe by removing UnwindInfoSections from FrameHeaderCache::CacheEntry and inlining just what's needed).

The DwarfFDECache waste is a worse. In practice, it will always be empty, and it's adding 2KiB of .bss in every DSO. That's easy to fix by lowering the number of elements in _initialBuffer[64].

libunwind/src/AddressSpace.hpp

 
 /// Used by findUnwindSections() to return info about needed sections.
 struct UnwindInfoSections {
 #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) || defined(_LIBUNWIND_SUPPORT_DWARF_INDEX) ||       \
     defined(_LIBUNWIND_SUPPORT_COMPACT_UNWIND)
   // No dso_base for SEH or ARM EHABI.
   uintptr_t       dso_base;
 #endif
+#if defined(_LIBUNWIND_USE_DL_ITERATE_PHDR) &&                                 \
+    defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
+  uintptr_t       text_segment_length;
+#endif
 #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
   uintptr_t       dwarf_section;
   uintptr_t       dwarf_section_length;
 #endif
 #if defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
   uintptr_t       dwarf_index_section;
   uintptr_t       dwarf_index_section_length;
 #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;
+      cbdata->sects->text_segment_length = phdr->p_memsz;
       return true;
     }
   }
   return false;
 }
 
 static int findUnwindSectionsByPhdr(struct dl_phdr_info *pinfo,
                                     size_t pinfo_size, void *data) {
for (Elf_Half i = pinfo->dlpi_phnum; i > 0; i--) {
     if (!found_hdr && 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)
+      if (found_hdr) {
+        // .eh_frame_hdr records the start of .eh_frame, but not its size.
+        // Rely on a zero terminator to find the end of the section.
         cbdata->sects->dwarf_section = hdrInfo.eh_frame_ptr;
+        cbdata->sects->dwarf_section_length = UINTPTR_MAX;
+      }
     } else if (!found_obj) {
       found_obj = checkAddrInSegment(phdr, image_base, cbdata);
     }
     if (found_obj && found_hdr) {
 #if defined(_LIBUNWIND_USE_FRAME_HEADER_CACHE)
       TheFrameHeaderCache.add(cbdata->sects);
 #endif
       return 1;
     }
   }
-  cbdata->sects->dwarf_section_length = 0;
   return 0;
 }
 
 #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);

libunwind/src/DwarfParser.hpp

#endif
   struct PrologInfoStackEntry {
     PrologInfoStackEntry(PrologInfoStackEntry *n, const PrologInfo &i)
         : next(n), info(i) {}
     PrologInfoStackEntry *next;
     PrologInfo info;
   };
 
   static bool findFDE(A &addressSpace, pint_t pc, pint_t ehSectionStart,
-                      uint32_t sectionLength, pint_t fdeHint, FDE_Info *fdeInfo,
+                      uintptr_t sectionLength, pint_t fdeHint, FDE_Info *fdeInfo,
                       CIE_Info *cieInfo);
   static const char *decodeFDE(A &addressSpace, pint_t fdeStart,
                                FDE_Info *fdeInfo, CIE_Info *cieInfo);
   static bool parseFDEInstructions(A &addressSpace, const FDE_Info &fdeInfo,
                                    const CIE_Info &cieInfo, pint_t upToPC,
                                    int arch, PrologInfo *results);
 
   static const char *parseCIE(A &addressSpace, pint_t cie, CIE_Info *cieInfo);
const char *CFI_Parser<A>::decodeFDE(A &addressSpace, pint_t fdeStart,
   pint_t cfiLength = (pint_t)addressSpace.get32(p);
   p += 4;
   if (cfiLength == 0xffffffff) {
     // 0xffffffff means length is really next 8 bytes
     cfiLength = (pint_t)addressSpace.get64(p);
     p += 8;
   }
   if (cfiLength == 0)
-    return "FDE has zero length"; // end marker
+    return "FDE has zero length"; // zero terminator
   uint32_t ciePointer = addressSpace.get32(p);
   if (ciePointer == 0)
     return "FDE is really a CIE"; // this is a CIE not an FDE
   pint_t nextCFI = p + cfiLength;
   pint_t cieStart = p - ciePointer;
   const char *err = parseCIE(addressSpace, cieStart, cieInfo);
   if (err != NULL)
     return err;
const char *CFI_Parser<A>::decodeFDE(A &addressSpace, pint_t fdeStart,
   fdeInfo->pcStart = pcStart;
   fdeInfo->pcEnd = pcStart + pcRange;
   return NULL; // success
 }
 
 /// Scan an eh_frame section to find an FDE for a pc
 template <typename A>
 bool CFI_Parser<A>::findFDE(A &addressSpace, pint_t pc, pint_t ehSectionStart,
-                            uint32_t sectionLength, pint_t fdeHint,
+                            uintptr_t sectionLength, pint_t fdeHint,
                             FDE_Info *fdeInfo, CIE_Info *cieInfo) {
   //fprintf(stderr, "findFDE(0x%llX)\n", (long long)pc);
   pint_t p = (fdeHint != 0) ? fdeHint : ehSectionStart;
-  const pint_t ehSectionEnd = p + sectionLength;
+  const pint_t ehSectionEnd = (sectionLength == UINTPTR_MAX)
+                                  ? static_cast<pint_t>(-1)
+                                  : (ehSectionStart + sectionLength);
   while (p < ehSectionEnd) {
     pint_t currentCFI = p;
     //fprintf(stderr, "findFDE() CFI at 0x%llX\n", (long long)p);
     pint_t cfiLength = addressSpace.get32(p);
     p += 4;
     if (cfiLength == 0xffffffff) {
       // 0xffffffff means length is really next 8 bytes
       cfiLength = (pint_t)addressSpace.get64(p);
       p += 8;
     }
     if (cfiLength == 0)
-      return false; // end marker
+      return false; // zero terminator
     uint32_t id = addressSpace.get32(p);
     if (id == 0) {
       // Skip over CIEs.
       p += cfiLength;
     } else {
       // Process FDE to see if it covers pc.
       pint_t nextCFI = p + cfiLength;
       uint32_t ciePointer = addressSpace.get32(p);

libunwind/src/FrameHeaderCache.hpp

 // This cache should only be be used from within a dl_iterate_phdr callback.
 // dl_iterate_phdr does the necessary synchronization to prevent problems
 // with concurrent access via the libc load lock. Adding synchronization
 // for other uses is possible, but not currently done.
 
 class _LIBUNWIND_HIDDEN FrameHeaderCache {
   struct CacheEntry {
     uintptr_t LowPC() { return Info.dso_base; };
-    uintptr_t HighPC() { return Info.dso_base + Info.dwarf_section_length; };
+    uintptr_t HighPC() { return Info.dso_base + Info.text_segment_length; };
     UnwindInfoSections Info;
     CacheEntry *Next;
   };
 
   static const size_t kCacheEntryCount = 8;
 
   // Can't depend on the C++ standard library in libunwind, so use an array to
   // allocate the entries, and two linked lists for ordering unused and recently
   // used entries.  FIXME: Would the the extra memory for a doubly-linked list
void resetCache() {
     MostRecentlyUsed = nullptr;
     Unused = &Entries[0];
     for (size_t i = 0; i < kCacheEntryCount - 1; i++) {
       Entries[i].Next = &Entries[i + 1];
     }
     Entries[kCacheEntryCount - 1].Next = nullptr;
   }
 
   bool cacheNeedsReset(dl_phdr_info *PInfo) {
     // C libraries increment dl_phdr_info.adds and dl_phdr_info.subs when
     // loading and unloading shared libraries. If these values change between
     // iterations of dl_iterate_phdr, then invalidate the cache.
 
     // These are static to avoid needing an initializer, and unsigned long long
     // because that is their type within the extended dl_phdr_info.  Initialize
     // these to something extremely unlikely to be found upon the first call to
     // dl_iterate_phdr.
     static unsigned long long LastAdds = ULLONG_MAX;
     static unsigned long long LastSubs = ULLONG_MAX;
     if (PInfo->dlpi_adds != LastAdds || PInfo->dlpi_subs != LastSubs) {
       // Resetting the entire cache is a big hammer, but this path is rare--
       // usually just on the very first call, when the cache is empty anyway--so
       // added complexity doesn't buy much.
       LastAdds = PInfo->dlpi_adds;
       LastSubs = PInfo->dlpi_subs;
       resetCache();
       return true;
     }
     return false;
   }
 
 public:
   bool find(dl_phdr_info *PInfo, size_t, void *data) {
     if (cacheNeedsReset(PInfo) || MostRecentlyUsed == nullptr)
       return false;
 
     auto *CBData = static_cast<dl_iterate_cb_data *>(data);
     CacheEntry *Current = MostRecentlyUsed;
     CacheEntry *Previous = nullptr;
     while (Current != nullptr) {
       _LIBUNWIND_FRAMEHEADERCACHE_TRACE(
           "FrameHeaderCache check %lx in [%lx - %lx)", CBData->targetAddr,
           Current->LowPC(), Current->HighPC());
       if (Current->LowPC() <= CBData->targetAddr &&
           CBData->targetAddr < Current->HighPC()) {
         _LIBUNWIND_FRAMEHEADERCACHE_TRACE(
             "FrameHeaderCache hit %lx in [%lx - %lx)", CBData->targetAddr,
             Current->LowPC(), Current->HighPC());
         if (Previous) {
           // If there is no Previous, then Current is already the
           // MostRecentlyUsed, and no need to move it up.
           Previous->Next = Current->Next;
           Current->Next = MostRecentlyUsed;
           MostRecentlyUsed = Current;
         }
         *CBData->sects = Current->Info;
         return true;
       }
       Previous = Current;
       Current = Current->Next;
     }
     _LIBUNWIND_FRAMEHEADERCACHE_TRACE("FrameHeaderCache miss for address %lx",
                                       CBData->targetAddr);
     return false;
   }
 
   void add(const UnwindInfoSections *UIS) {
     CacheEntry *Current = nullptr;
 
     if (Unused != nullptr) {
       Current = Unused;
       Unused = Unused->Next;
     } else {
       Current = MostRecentlyUsed;
       CacheEntry *Previous = nullptr;

libunwind/src/UnwindCursor.hpp

bool UnwindCursor<A, R>::getInfoFromDwarfSection(pint_t pc,
                                                 const UnwindInfoSections &sects,
                                                 uint32_t fdeSectionOffsetHint) {
   typename CFI_Parser<A>::FDE_Info fdeInfo;
   typename CFI_Parser<A>::CIE_Info cieInfo;
   bool foundFDE = false;
   bool foundInCache = false;
   // If compact encoding table gave offset into dwarf section, go directly there
   if (fdeSectionOffsetHint != 0) {
     foundFDE = CFI_Parser<A>::findFDE(_addressSpace, pc, sects.dwarf_section,
-                                    (uint32_t)sects.dwarf_section_length,
+                                    sects.dwarf_section_length,
                                     sects.dwarf_section + fdeSectionOffsetHint,
                                     &fdeInfo, &cieInfo);
   }
 #if defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
   if (!foundFDE && (sects.dwarf_index_section != 0)) {
     foundFDE = EHHeaderParser<A>::findFDE(
         _addressSpace, pc, sects.dwarf_index_section,
         (uint32_t)sects.dwarf_index_section_length, &fdeInfo, &cieInfo);
   }
 #endif
   if (!foundFDE) {
     // otherwise, search cache of previously found FDEs.
     pint_t cachedFDE = DwarfFDECache<A>::findFDE(sects.dso_base, pc);
     if (cachedFDE != 0) {
       foundFDE =
           CFI_Parser<A>::findFDE(_addressSpace, pc, sects.dwarf_section,
-                                 (uint32_t)sects.dwarf_section_length,
+                                 sects.dwarf_section_length,
                                  cachedFDE, &fdeInfo, &cieInfo);
       foundInCache = foundFDE;
     }
   }
   if (!foundFDE) {
     // Still not found, do full scan of __eh_frame section.
     foundFDE = CFI_Parser<A>::findFDE(_addressSpace, pc, sects.dwarf_section,
-                                      (uint32_t)sects.dwarf_section_length, 0,
+                                      sects.dwarf_section_length, 0,
                                       &fdeInfo, &cieInfo);
   }
   if (foundFDE) {
     if (getInfoFromFdeCie(fdeInfo, cieInfo, pc, sects.dso_base)) {
       // Add to cache (to make next lookup faster) if we had no hint
       // and there was no index.
       if (!foundInCache && (fdeSectionOffsetHint == 0)) {
   #if defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
#endif // #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
   // no unwind info, flag that we can't reliably unwind
   _unwindInfoMissing = true;
 }
 
 template <typename A, typename R>
 int UnwindCursor<A, R>::step() {
   // Bottom of stack is defined is when unwind info cannot be found.
   if (_unwindInfoMissing)
     return UNW_STEP_END;
 
   // Use unwinding info to modify register set as if function returned.
   int result;
 #if defined(_LIBUNWIND_SUPPORT_COMPACT_UNWIND)
   result = this->stepWithCompactEncoding();
 #elif defined(_LIBUNWIND_SUPPORT_SEH_UNWIND)
   result = this->stepWithSEHData();
 #elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
   result = this->stepWithDwarfFDE();
 #elif defined(_LIBUNWIND_ARM_EHABI)
   result = this->stepWithEHABI();
 #else
   #error Need _LIBUNWIND_SUPPORT_COMPACT_UNWIND or \
               _LIBUNWIND_SUPPORT_SEH_UNWIND or \
               _LIBUNWIND_SUPPORT_DWARF_UNWIND or \
               _LIBUNWIND_ARM_EHABI
 #endif
 
   // update info based on new PC
   if (result == UNW_STEP_SUCCESS) {
     this->setInfoBasedOnIPRegister(true);
     if (_unwindInfoMissing)
       return UNW_STEP_END;
   }
 
   return result;
 }
 
 template <typename A, typename R>
 void UnwindCursor<A, R>::getInfo(unw_proc_info_t *info) {
   if (_unwindInfoMissing)

libunwind/test/frameheadercache_test.pass.cpp

 #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"
 
 #define kBaseAddr 0xFFF000
-#define kDwarfSectionLength 0xFF
+#define kTextSegmentLength 0xFF
 
 using namespace libunwind;
 
 int main() {
   FrameHeaderCache FHC;
   struct dl_phdr_info PInfo;
   memset(&PInfo, 0, sizeof(PInfo));
   // The cache itself should only care about these two fields--they
   // tell the cache to invalidate or not; everything else is handled
   // by AddressSpace.hpp.
   PInfo.dlpi_adds = 6;
   PInfo.dlpi_subs = 7;
 
   UnwindInfoSections UIS;
   UIS.dso_base = kBaseAddr;
-  UIS.dwarf_section_length = kDwarfSectionLength;
+  UIS.text_segment_length = kTextSegmentLength;
   dl_iterate_cb_data CBData;
   // Unused by the cache.
   CBData.addressSpace = nullptr;
   CBData.sects = &UIS;
   CBData.targetAddr = kBaseAddr + 1;
 
   // Nothing present, shouldn't find.
   if (FHC.find(&PInfo, 0, &CBData))
int main() {
 
   FHC.add(&UIS);
   CBData.targetAddr = kBaseAddr - 1;
   // Shouldn't find something outside of the addresses.
   if (FHC.find(&PInfo, 0, &CBData))
     abort();
   // Add enough things to the cache that the entry is evicted.
   for (int i = 0; i < 9; i++) {
-    UIS.dso_base = kBaseAddr + (kDwarfSectionLength * i);
+    UIS.dso_base = kBaseAddr + (kTextSegmentLength * 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