This is an archive of the discontinued LLVM Phabricator instance.

Add .eh_frame_hdr search to Linux unwinder.
ClosedPublic

Authored by danalbert on Jan 5 2015, 12:15 PM.

Details

Summary

This improves the performance of unwinding on DWARF based targets. The
32-bit x86 support for scanning the full eh_frame
(CFI_Parser::findFDE) apparently does not work (at least not on
Linux). Since the eh_frame_hdr code delegates to that, this still
doesn't work for x86 Linux, but it has been tested on x86_64 Linux and
aarch64 Android.

Diff Detail

Event Timeline

danalbert updated this revision to Diff 17810.Jan 5 2015, 12:15 PM
danalbert retitled this revision from to Add .eh_frame_hdr search to Linux unwinder..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert set the repository for this revision to rL LLVM.
danalbert added a subscriber: Unknown Object (MLST).

This is passing all of the tests for x86_64, but I'm still having some (seemingly unrelated trouble) with x86. On x86, I'm seeing a bogus return address coming from the CFI here: https://github.com/llvm-mirror/libcxxabi/blob/master/src/Unwind/DwarfInstructions.hpp#L187 So far I haven't had any luck in figuring out why.

In theory this should work for aarch64 too, but I haven't had a chance to test that.

The commit message is actually not correct. This should work for the BSDs as well.

jroelofs added inline comments.Jan 22 2015, 12:56 PM
src/Unwind/AddressSpace.hpp
375

Do byval captures not work here?

383

As an optimization, I think you can bail early here if the targetAddr cannot possibly be in the particular *.so:

if (cbdata->targetAddr < pinfo->dlpi_addr)
  return false;
409
src/Unwind/EHHeaderParser.hpp
112

The EHABI unwinder uses some library function for doing its binary search for the EHT entry. Maybe that can be re-used here too.

src/Unwind/UnwindCursor.hpp
839

It's interesting that the cache lookup isn't first. I wonder why (not really important for this review).

src/Unwind/config.h
86–88

I think one of @joerg's platforms uses dwarf unwind on arm. I don't remember the name of the guard he created to detect that though.

dschuff added inline comments.Jan 22 2015, 1:04 PM
src/Unwind/config.h
86–88

elsewhere in libcxxabi there are ifdefs for ARM_DWARF_EH

danalbert added inline comments.Jan 22 2015, 1:42 PM
src/Unwind/AddressSpace.hpp
375

No captures whatsoever. Those only work where the function actually takes a std::function, whereas this is a plain old function pointer.

409
src/Unwind/EHHeaderParser.hpp
112

I'll look in to that.

joerg added inline comments.Jan 25 2015, 11:53 AM
src/Unwind/EHHeaderParser.hpp
120

Isn't this wrong if you hit a case exactly?

danalbert updated this revision to Diff 19496.Feb 6 2015, 10:13 AM

Address review comments.

danalbert added inline comments.Feb 6 2015, 10:15 AM
src/Unwind/EHHeaderParser.hpp
112

The case you were thinking of is https://github.com/llvm-mirror/libcxxabi/blob/master/src/Unwind/UnwindCursor.hpp#L676

That's a dependency on libc++, and you've been trying to fix the layering issues here. I don't think I'll add to them :)

jroelofs added inline comments.Feb 6 2015, 10:21 AM
src/Unwind/EHHeaderParser.hpp
112

yeah, that's it. I forgot it was using something from std::. Agreed, no need to add one more layering problem.

danalbert updated this revision to Diff 19511.Feb 6 2015, 1:40 PM

Follow same header search pattern as netbsd at nbjoerg's suggestion.

jroelofs edited edge metadata.Feb 26 2015, 3:01 PM

@danalbert: reminder so you don't forget about this before @compnerd moves the unwdiner.

ed added a subscriber: ed.Feb 27 2015, 1:15 AM

Just stumbled upon this patch and thought I'd chime in briefly.

I am currently working on a POSIX-like environment that is completely built around the concept of capability-based security which I am planning on releasing under a BSD license 1-2 months from now. I'm leaning on LLVM components quite heavily (LLVM, Clang, libcxx, libcxxabi, compiler-rt).

To get exceptions working in my environment I previously used a small patch to ld's linker script to add symbols where .eh_frame begins/ends and a change for libcxxabi to use those. After discovering this patch I decided to add dl_iterate_phdr() as well and revert my local changes. So far things work great. All libcxx exception related tests pass properly. At least on x86-64.

That said, keep in mind that ElfW() does not exist on FreeBSD. Adding some code like this makes the code build properly.

#ifndef ElfW                                                                     
#define ElfW(type) ElfW2(__INTPTR_WIDTH__, type)
#define ElfW2(width, type) ElfW3(width, type)
#define ElfW3(width, type) Elf##width##_##type
#endif

Thanks!

danalbert updated this revision to Diff 20888.Feb 27 2015, 1:27 PM
danalbert edited edge metadata.

Fix the base address for the unwind table.

Apparently this hadn't actually been working before. The header search was
failing, and then the seach was falling back to scanning the whole table.
Thanks to Saleem for noticing my mistake.

Unfortunately this means that we can't actually test this code directly. If it
fails to find the entry in the search table, it will still find it by scanning
the whole section. We should probably add some real unit tests to this project
at some point to deal with this.

danalbert updated this object.Feb 27 2015, 1:28 PM
danalbert set the repository for this revision to rL LLVM.
compnerd edited edge metadata.Feb 27 2015, 2:03 PM

Looks like it should be okay beyond the inline comments.

src/Unwind/EHHeaderParser.hpp
34

Can you change these to the specification specified names? It makes it easier to cross-reference.

src/Unwind/config.h
86

Does clang-format format this statement this way?

danalbert updated this revision to Diff 20900.Feb 27 2015, 2:17 PM
danalbert edited edge metadata.

Make the header info names match the names in the spec.

danalbert added inline comments.Feb 27 2015, 2:18 PM
src/Unwind/config.h
86

clang-format hates everything about this project. The other defines in the file match this.

compnerd accepted this revision.Feb 27 2015, 2:20 PM
compnerd edited edge metadata.
This revision is now accepted and ready to land.Feb 27 2015, 2:20 PM