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

[Adrian Vogelsgesang via Phabricator via llvm-commits]

avogelsgesang marked 3 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:
> > > 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?
> I see.  I think that having the assert that its not supported is fine, but I would rather see that be more explicit before the region as:
> 
> ```
> #if DLFO_STRUCT_HAS_EH_DBASE
> #error eh_dbase is not supported for ...
> #endif
> ```
> 
> This makes it more obvious that it is something that we may need to address in the future and then subsequently "silently disabling" is safe - the compilation will fail.
> 
> Doing something similar for the `DLFO_EH_SEGMENT_TYPE` makes sense as well.
Makes sense. I updated the patch to use `#if` + `#error`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130668