[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:630
+        (char *)findResult.dlfo_map_end - (char *)findResult.dlfo_map_start);
+    static_assert(DLFO_STRUCT_HAS_EH_DBASE == 0, "unexpected base address");
+
----------------
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`


================
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:
> 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?


================
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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130668