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

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
Comment at: libunwind/src/AddressSpace.hpp:630
+        (char *)findResult.dlfo_map_end - (char *)findResult.dlfo_map_start);
+    static_assert(DLFO_STRUCT_HAS_EH_DBASE == 0, "unexpected base address");
+
----------------
avogelsgesang wrote:
> compnerd wrote:
> > This static assertion doesn't make sense to me.  You expect to define `DLFO_STRUCT_HAS_EH_DBASE` as `0` when it is available?
> based on https://www.gnu.org/software/libc/manual/html_node/Dynamic-Linker-Introspection.html
> 
> > On most targets, this macro is defined as 0. If it is defined to 1, struct dl_find_object contains an additional member dlfo_eh_dbase of type void *. It is the base address for DW_EH_PE_datarel DWARF encodings to this location.
> 
> The code here does not handle `dlfo_eh_dbase`, hence I want the build fail if I am on any platform where `dlfo_eh_dbase` exists. Hence, the static_assert checks that for `DLFO_STRUCT_HAS_EH_DBASE == 0`
I think that I would rather have this be written as `DLFO_STRUCT_HAS_EH_DBASE-0` on L608 rather than `#if defined(DLFO_STRUCT_HAS_EH_DBASE)` and then the static assert.  Additionally, this seems like it would then cause a build failure if that was ever defined as `0`.


================
Comment at: libunwind/src/AddressSpace.hpp:634
+    // PT_GNU_EH_FRAME section. Setting length to `SIZE_MAX` effectively
+    // disables all range checks.
+    static_assert(DLFO_EH_SEGMENT_TYPE == PT_GNU_EH_FRAME,
----------------
avogelsgesang wrote:
> compnerd wrote:
> > Could you move the comment to the assignment below please.
> The
> 
> > _dl_find_object` gives us the start but not the size of the PT_GNU_EH_FRAME section.
> 
> part of this comment is actually checked by the `static_assert` directly below this comment.
> 
> With the explanation of the `static_assert` below, do you still want me to move this comment? Should I split the comment into two? Should I keep it as is?
A split sounds good to me.


================
Comment at: libunwind/src/AddressSpace.hpp:635
+    // disables all range checks.
+    static_assert(DLFO_EH_SEGMENT_TYPE == PT_GNU_EH_FRAME,
+                  "unexpected segment retrieved by `_dl_find_object`");
----------------
avogelsgesang wrote:
> compnerd wrote:
> > Hmm, I don't understand this assertion either.
> from https://www.gnu.org/software/libc/manual/html_node/Dynamic-Linker-Introspection.html
> 
> > On targets using DWARF-based exception unwinding, this macro expands to PT_GNU_EH_FRAME. This indicates that dlfo_eh_frame in struct dl_find_object points to the PT_GNU_EH_FRAME segment of the object. On targets that use other unwinding formats, the macro expands to the program header type for the unwinding data.
> 
> The code below assumes that `dlfo_eh_frame` points to the `PT_GNU_EH_FRAME` section. The `static_assert` makes this assumption explicit
The confusion on my part stems from `DLFO_EH_SEGMENT_TYPE` ... that seems to be a macro that would be statically defined.  This is confusing as a check because it is not the type of segment that is _retrieved_, it is an assumption at build time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130668