[libcxx-commits] [PATCH] D130668: [libunwind] Use `_dl_find_object` if available

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
Comment at: libunwind/src/AddressSpace.hpp:640
+
+    // `_dl_find_object` gives us the start of the PT_GNU_EH_FRAME section.
+    info.dwarf_index_section =
----------------
MaskRay wrote:
> No need to replicate the member variable name. Just say "Record the start of PT_GNU_EH_FRAME"
> 
> PT_GNU_EH_FRAME is not a section.
Thanks for catching this! :)


================
Comment at: libunwind/src/AddressSpace.hpp:653
+    }
+    // .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.
----------------
MaskRay wrote:
> avogelsgesang wrote:
> > MaskRay wrote:
> > > > .eh_frame_hdr records the start of .eh_frame
> > > 
> > > This sentence is wrong. .eh_frame_hdr is a different section.
> > `PT_GNU_EH_FRAME` is a bit of a misnormer as it refers to `eh_frame_hdr`, not `eh_frame`. From https://refspecs.linuxbase.org/LSB_3.0.0/LSB-PDA/LSB-PDA/progheader.html
> > 
> > > PT_GNU_EH_FRAME:
> > > The array element specifies the location and size of the exception handling information as defined by the .eh_frame_hdr section.
> > 
> > The `eh_frame_hdr` is hence the section retrieved by `_dl_find_object`. `EHHeaderParser<LocalAddressSpace>::decodeEHHdr` a couple of lines above decodes the `eh_frame_hdr`.  Parsing the`eh_frame_hdr` gives us the pointer to the start of `eh_frame`, but doesn't give us its size. Hence, we set `dwarf_section_length` to `SIZE_MAX` and thereby rely on the zero terminator to find the end of the section.
> > 
> > Please let me know how to update this comment to make it more understandable.
> > 
> > Also, I shamelessly copied this comment from the `checkForUnwindInfoSegment` function. In case you want me to update also that comment, please let me know
> I know PT_GNU_EH_FRAME. The comment just doesn't make sense to me. I suggest something aligning:
> 
> Record the start of a FDE and use SIZE_MAX to indicate that we do not know the end address.
Sounds good to me. I updated the comment accordingly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130668