Thanks for the review. It looks like I still need someone from the libunwind group to accept it, though.

Thanks for the review. It looks like I still need someone from the libunwind group to accept it, though.

I uploaded the Android benchmark I used: https://gist.github.com/rprichard/1688de36f0132a40294adb0409189533.

This change enables the FrameHeaderCache for EHABI.

Apply clang-format fix.

I think these libunwind tests are relying on the signal trampoline having "good enough" unwind info, but AFAIK, that's not the case with the signal trampoline (__kernel_rt_sigreturn) in the AArch64 Linux vDSO. There was a commit recently adjusting .cfi_startproc to let the unwinder find the vDSO's FDE, but then this was quickly reverted. The FDE change allowed libgcc to find the FDE, and that broke glibc pthread cancellation somehow, so the FDE was then removed:

• https://github.com/torvalds/linux/commit/a4eb355a3fdad85d16e4b098e8d56bb28b812ce0
• https://github.com/torvalds/linux/commit/87676cfca14171fc4c99d96ae2f3e87780488ac4

Abandoned in favor of D87750.

This seems like a bad name.

Rename dso_length to text_segment_length.

Adding @saugustine -- this patch makes the frame header cache one word larger per entry.

Stylistic change.

I uploaded D87750, an alternative to this patch.

An alternative to D86256.

Just want to comment "Or use LLD, which automatically inserts the terminator" - the zero terminator CIE is to work around a glibc issue D46566

In D86256#2270331, @mstorsjo wrote:

The commit description talks about making the caching optional, but I don't really see that aspect in the patch - or maybe I'm just not studying it closely enough?

I think I'm fine with this revision. I was wondering if the two functions should be removed from the GNU libsupc++'s cxxabi.h as well, which also declares both of the functions with an int return type. (i.e. It has the same issue as LLVM's cxxabi.h.)

Use -1 to indicate "search all cache entries".

In D86748#2262437, @compnerd wrote:

Why not use -1 as the indicator instead of 0?

The Linux ABI Extensions spec describes the binary search table of .eh_frame_hdr as optional, so maybe falling back to .eh_frame scanning could be necessary to accommodate some ELF files? I wonder what situation results in an omitted search table. ld.bfd does have code for outputting .eh_frame_hdr without a search table.

On second thought, maybe I don't actually need this change, and the includes still seem like a mess in libunwind. Maybe it makes more sense to do a larger revision eventually.

In D86768#2253572, @saugustine wrote:

I am going to accept this, since no one else seems to be able to review it. But the problem here is that no one really knows what a simplified ifdef chain with the same functionality looks like. I tried a couple of times and failed miserably.

So if this works, great! If not, we will need to revert it quickly.

Do I need an approval from a libunwind group member?

Remove most of the previous change. Add a comment about Apple/armv7k.

In D86767#2244767, @steven_wu wrote:

Sorry for the inconvenience that libunwind is not really buildable for armv7 from iOS SDK. You can build a single threaded version without referring to those interfaces. If this is really troublesome for you, feel free to file a bug report and we can see what can be done here.

If the unwinder uses dl_iterate_phdr, then entries that are automatically added to DwarfFDECache would become invalid if the module containing the entry were unloaded. (On Apple systems, DwarfFDECache registers dyldUnloadHook to remove unloaded entries.)

FWIW, when compact unwinding defers to DWARF (compactSaysUseDwarf), the unwinder begins a scan of .eh_frame at a given offset. Is that really the right behavior, or will the offset always point directly at the appropriate FDE (as with the DWARF index .eh_frame_hdr)?

FWIW, I compiled libunwind for Darwin, and I had trouble getting it to compile for armv7. Unwind-sjlj.c needs these things:

• #include <System/pthread_machdep.h>
• _pthread_getspecific_direct(__PTK_LIBC_DYLD_Unwind_SjLj_Key)

Rebase this change.

Split out the chain-of-ifdefs change into D86768.

In D86372#2234459, @rprichard wrote:

In D86372#2234249, @hans wrote:

From the discussion on D86254, this should be cherry-picked to 11.x right?

Correct. This findUnwindSectionsByPhdr change (D86372) is needed for 11.

In D86411#2233326, @mstorsjo wrote:

Oh, ok, I see - maybe CMake deduced that the option wasn't needed in my case, when building with a compiler that defaults to a new enough version.

In D86372#2234249, @hans wrote:

From the discussion on D86254, this should be cherry-picked to 11.x right?

In D86254#2234248, @hans wrote:

In D86254#2230647, @compnerd wrote:

Please ensure that the fix goes into 11 before release (CC: @hans), but the remainder of the change is fine.

Just to make sure I get it right, the fix need for 11 is https://reviews.llvm.org/D86372, right?

In D86411#2232638, @mstorsjo wrote:

I guess this fix makes sense - I'd be surprised if we don't already require C++11 for building in general.

This change fixes this GCC build failure, https://reviews.llvm.org/D86102#2232290.

(This change was previously accepted when it was part of D86254.)

After this change, I see a compile error with GCC. There's something broken with libunwind's static_assert emulation. I'd guess the fix is to remove that emulation and simply use the C++11 static_assert.

Update summary.

Break the "Make findUnwindSectionsByPhdr static" change into its own patch.

Stylistic changes.

Stylistic changes. Also, only a single caller to getInfoFromFdeCie adds the result to the DwarfFDECache, so leave the cache addition code in getInfoFromDwarfSection.

There is a libunwind::findUnwindSectionsByPhdr as of 11.x. When I build 11.x libunwind.so for glibc (I tested arm and x86_64), the symbol is exposed:

Revert edit that crept in from a later patch: SECTION -> UNWIND.

Should we cherry-pick this change to 11.x to help with https://bugs.llvm.org/show_bug.cgi?id=46743? The cache also relies on the dlpi_adds / dlpi_subs fields that are only added in Android R (the upcoming version).

The change seems alright to me.

AFAIK, there's no regression, and no one has hit this issue in practice, so maybe it can wait until the next release? It's probably only an issue when the size of something exceeds 2**32?

There's only an issue if the return address register is in the range [128..255], and my impression is that probably never happens. It's also not a regression.

For reference, here's where Clang selects the CIE version:
https://github.com/llvm/llvm-project/blob/74c14202d90b46dda64a2542602855727b7d7f60/llvm/lib/MC/MCDwarf.cpp#L1592-L1605

Stylistic changes.