[libcxx-commits] [PATCH] D75781: Rework findUnwindSectionsByPhdr to be more optimal.

[Sterling Augustine via Phabricator via libcxx-commits]

saugustine marked 3 inline comments as done.
saugustine added inline comments.


================
Comment at: libunwind/src/AddressSpace.hpp:463-464
 
-  if (cbdata->targetAddr < pinfo->dlpi_addr)
+int findUnwindSectionsByPhdr(struct dl_phdr_info *pinfo, size_t, void *data) {
+  auto cbdata = static_cast<dl_iterate_cb_data *>(data);
+  if (pinfo->dlpi_phnum == 0)
     return 0;
 
----------------
saugustine wrote:
> jgorbe wrote:
> > Is this early check no longer necessary/beneficial?
> I think it is still needed by the reverse-iteration below, which assumes there is one or more phdrs.
> 
> This is likely always true--Would dl_iterate_phdr ever pass a phdr-less pinfo? But I don't think dl_iterate_phdr actually guarantees that, at least not in any documentation I have read.
Actually, I responded to the wrong question. You are right that this code is still useful. Restored.


================
Comment at: libunwind/src/AddressSpace.hpp:502
   }
-
-  if (found_obj && found_hdr) {
-    cbdata->sects->dwarf_section_length = object_length;
-    return true;
-  } else {
-    return false;
-  }
+  cbdata->sects->dwarf_section_length = 0;
+  return 0;
----------------
jgorbe wrote:
> The old code only touches `dwarf_section_length` if both phdrs are found, whereas this patch always resets it to 0 after the loop if it didn't find both. Is this intentional?
It is. I believe the code is equivalent if dwarf_section_length is always zero on entry (which it is). The new code avoids carrying object_length as a separate variable, and trying to figure out what it means.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75781