[libunwind] Use `_dl_find_object` if available

[avogelsgesang]

Summary

[libunwind] Use _dl_find_object if available

As shown in P2544R0 [1] and the accompanying benchmark [2], the
current unwinding logic does not scale for multi-threaded programs.
This is because dl_iterate_phdr takes a global lock.

glibc 2.35 added _dl_find_object which directly returns the unwind
info for a given target address. _dl_find_object is fully lock-free
and hence allows parallel exception unwinding on multiple threads.

With this commit, libunwind now takes advantage of _dl_find_object.
Thereby, this commit improves libunwind's performance on benchmark [2]
for unwinding exception on 20 threads from 1103ms to 78ms.
(measured on Intel Xeon Silver 4114 with 20 physical cores)

[1] https://isocpp.org/files/papers/P2544R0.html
[2] https://github.com/neumannt/exceptionperformance

Detailed performance numbers from the benchmark:

Before:

Testing unwinding performance: sqrt computation with occasional errors

testing baseline using 1 2 4 8 16 20 threads
failure rate 0%: 34 35 34 35 35 36
testing exceptions using 1 2 4 8 16 20 threads
failure rate 0%: 16 32 33 34 35 36
failure rate 0.1%: 16 32 34 36 35 36
failure rate 1%: 20 40 40 43 90 113
failure rate 10%: 59 92 140 304 880 1103
[...]

Testing invocation overhead: recursive fib with occasional errors

testing exceptions using 1 2 4 8 16 20 threads
failure rate 0%: 19 32 37 38 39 36
failure rate 0.1%: 22 32 40 40 39 34
failure rate 1%: 20 28 38 39 48 40
failure rate 10%: 25 39 44 50 92 113

After:

Testing unwinding performance: sqrt computation with occasional errors

testing baseline using 1 2 4 8 16 20 threads
failure rate 0%: 19 30 35 38 39 35
testing baseline using 1 2 4 8 16 20 threads
failure rate 0%: 32 35 33 34 34 36
testing exceptions using 1 2 4 8 16 20 threads
failure rate 0%: 16 35 33 37 35 35
failure rate 0.1%: 16 32 36 33 34 37
failure rate 1%: 21 37 39 40 40 41
failure rate 10%: 72 75 76 80 80 78
[...]

Testing invocation overhead: recursive fib with occasional errors

testing baseline using 1 2 4 8 16 20 threads
failure rate 0%: 18 35 37 34 38 37
testing exceptions using 1 2 4 8 16 20 threads
failure rate 0%: 19 33 40 40 41 39
failure rate 0.1%: 21 33 39 38 39 38
failure rate 1%: 20 36 39 40 41 40
failure rate 10%: 25 45 41 42 44 43

[avogelsgesang]

not sure whom I should add as a reviewer.
@ldionne, @MaskRay you were on the last couple of libunwind reviews, are you the right reviewers for this patch? If not, do you know who could review this?

@neumannt FYI, as an alternative to D120243

[avogelsgesang]

Apply clang-format and fix commit title

[compnerd]

libunwind/src/AddressSpace.hpp

630

This static assertion doesn't make sense to me. You expect to define DLFO_STRUCT_HAS_EH_DBASE as 0 when it is available?

634

Could you move the comment to the assignment below please.

635

Hmm, I don't understand this assertion either.

642

I wonder if it makes sense to use info.dwarf_index_section_length here.

[avogelsgesang]

Pass info.dwarf_index_section_length instead of SIZE_MAX as requested by @compnerd

libunwind/src/AddressSpace.hpp

630

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

634

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?

635

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

[compnerd]

libunwind/src/AddressSpace.hpp

630

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.

634

A split sounds good to me.

635

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.

[avogelsgesang]

Rephrase comments/static_assert's

[avogelsgesang]

libunwind/src/AddressSpace.hpp

630

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?

634

I split the comment in the newly updated version

635

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

[compnerd]

libunwind/src/AddressSpace.hpp

630

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.

[avogelsgesang]

Turn build assertions from static_assert into #error

[avogelsgesang]

libunwind/src/AddressSpace.hpp

630

Makes sense. I updated the patch to use #if + #error

[compnerd]

Thanks, this looks reasonable to me.

[avogelsgesang]

Thanks for your review & your approval, @compnerd!

Do you know the next steps to get this landed? Is your approval sufficient or should we wait for additional reviewers?

[avogelsgesang]

Fix typo in error message

[compnerd]

It should be okay to commit.

[MaskRay]

libunwind/src/AddressSpace.hpp

614

typo: the the

653

.eh_frame_hdr records the start of .eh_frame

This sentence is wrong. .eh_frame_hdr is a different section.

[avogelsgesang]

Fix typo in comment: the the -> the

[avogelsgesang]

libunwind/src/AddressSpace.hpp

653

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

[MaskRay]

libunwind/src/AddressSpace.hpp

640

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.

653

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.

[avogelsgesang]

Adjust comments as suggested by @MaskRay

[avogelsgesang]

Update one more comment based on @MaskRay's comment that PT_GNU_EH_FRAME is not a section

libunwind/src/AddressSpace.hpp

640

Thanks for catching this! :)

653

Sounds good to me. I updated the comment accordingly

[avogelsgesang]

@MaskRay Do you have any additional comments/requests? Or am I good to land this patch?

[MaskRay]

libunwind/src/AddressSpace.hpp

614

PT_GNU_EH_FRAME is not a section.

"return PT_GNU_EH_FRAME" should suffice.

640

End full sentences with a period.

[avogelsgesang]

small comment fixes

[mgorny]

I'm sorry to be reporting it this late but this change broke building on 32-bit x86: https://github.com/llvm/llvm-project/issues/57733

libunwind/src/AddressSpace.hpp

 #elif defined(_LIBUNWIND_USE_DL_UNWIND_FIND_EXIDX)
   int length = 0;
   info.arm_section =
       (uintptr_t)dl_unwind_find_exidx((_Unwind_Ptr)targetAddr, &length);
   info.arm_section_length = (size_t)length * sizeof(EHABIIndexEntry);
   if (info.arm_section && info.arm_section_length)
     return true;
 #elif defined(_LIBUNWIND_USE_DL_ITERATE_PHDR)
+  // Use DLFO_STRUCT_HAS_EH_DBASE to determine the existence of
+  // `_dl_find_object`. Use _LIBUNWIND_SUPPORT_DWARF_INDEX, because libunwind
+  // support for _dl_find_object on other unwind formats is not implemented,
+  // yet.
+#if defined(DLFO_STRUCT_HAS_EH_DBASE) & defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
+  // We expect to run on a platform which does not use a base address for
+  // exception information.
+#if DLFO_STRUCT_HAS_EH_DBASE
+#error dlfo_eh_dbase is not supported for DWARF-based unwinding
+#endif
+  // We expect `_dl_find_object` to return PT_GNU_EH_FRAME.
+#if DLFO_EH_SEGMENT_TYPE != PT_GNU_EH_FRAME
+#error _dl_find_object retrieves an unexpected section type
+#endif
+  // We look-up `dl_find_object` dynamically at runtime to ensure backwards
+  // compatibility with earlier version of glibc not yet providing it. On older
+  // systems, we gracefully fallback to `dl_iterate_phdr`. Cache the pointer
+  // so we only look it up once. Do manual lock to avoid _cxa_guard_acquire.
+  static decltype(_dl_find_object) *dlFindObject;
+  static bool dlFindObjectChecked = false;
+  if (!dlFindObjectChecked) {
+    dlFindObject = reinterpret_cast<decltype(_dl_find_object) *>(
+        dlsym(RTLD_DEFAULT, "_dl_find_object"));
+    dlFindObjectChecked = true;
+  }
+  // Try to find the unwind info using `dl_find_object`
+  dl_find_object findResult;
+  if (dlFindObject && dlFindObject((void *)targetAddr, &findResult) == 0) {
+    if (findResult.dlfo_eh_frame == nullptr) {
+      // Found an entry for `targetAddr`, but there is no unwind info.
+      return false;
+    }
+    info.dso_base = reinterpret_cast<uintptr_t>(findResult.dlfo_map_start);
+    info.text_segment_length = static_cast<size_t>(
+        (char *)findResult.dlfo_map_end - (char *)findResult.dlfo_map_start);
+
+    // Record the start of PT_GNU_EH_FRAME.
+    info.dwarf_index_section =
+        reinterpret_cast<uintptr_t>(findResult.dlfo_eh_frame);
+    // `_dl_find_object` does not give us the size of PT_GNU_EH_FRAME.
+    // Setting length to `SIZE_MAX` effectively disables all range checks.
+    info.dwarf_index_section_length = SIZE_MAX;
+    EHHeaderParser<LocalAddressSpace>::EHHeaderInfo hdrInfo;
+    if (!EHHeaderParser<LocalAddressSpace>::decodeEHHdr(
+            *this, info.dwarf_index_section, info.dwarf_index_section_length,
+            hdrInfo)) {
+      return false;
+    }
+    // Record the start of the FDE and use SIZE_MAX to indicate that we do
+    // not know the end address.
+    info.dwarf_section = hdrInfo.eh_frame_ptr;
+    info.dwarf_section_length = SIZE_MAX;
+    return true;
+  }
+#endif
   dl_iterate_cb_data cb_data = {this, &info, targetAddr};
   int found = dl_iterate_phdr(findUnwindSectionsByPhdr, &cb_data);
   return static_cast<bool>(found);
 #endif
 
   return false;
 }