[PATCH] D130668: [libunwind] Use `_dl_find_object` if available

[Adrian Vogelsgesang via Phabricator via llvm-commits]

avogelsgesang marked 2 inline comments as done.
avogelsgesang added inline comments.


================
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");
+
----------------
compnerd wrote:
> 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`.
> this seems like it would then cause a build failure if that was ever defined as 0

That was my intent. My line of thinking is:
I don't know of any system which defines `DLFO_STRUCT_HAS_EH_DBASE != 0`. As such, I didn't handle it in this code, under the assumption that those systems are not relevant for libunwind. In case there is indeed a relevant system with `DLFO_STRUCT_HAS_EH_DBASE != 0`, I would rather want to get a build error than silently disabling this optimization.

But this is not my decision to make, but rather up to the maintainers/you

> 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)

Should I also move the similar `DLFO_EH_SEGMENT_TYPE == PT_GNU_EH_FRAME` to L608?


================
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,
----------------
compnerd wrote:
> 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.
I split the comment in the newly updated version


================
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`");
----------------
compnerd wrote:
> 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.
I rephrased the assert to

> `_dl_find_object` retrieves an unexpected section type

I hope together with the updated comments, this makes the intent of the assert more easily understandable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130668