Page MenuHomePhabricator

libc++abi: Make the .eh_frame_hdr code work on FreeBSD as well
ClosedPublic

Authored by ed on Mar 9 2015, 6:59 AM.

Details

Summary

We currently only include <link.h> on CloudABI and Linux. We can enable it on FreeBSD as well, as it also supports the dl_iterate_phdr() function that's provided by <link.h>.

FreeBSD, however, does not provide the ElfW() macro. Instead, the host-specific ELF datastructures are named just Elf_XXX in addition to the host-independent Elf32_XXX and Elf64_XXX types.

Diff Detail

Event Timeline

ed updated this revision to Diff 21487.Mar 9 2015, 6:59 AM
ed retitled this revision from to libc++abi: Make the .eh_frame_hdr code work on FreeBSD and CloudABI as well.
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added a reviewer: danalbert.
ed set the repository for this revision to rL LLVM.
ed added a subscriber: Unknown Object (MLST).
ed added a reviewer: compnerd.Mar 14 2015, 3:32 AM

Saleem, could you please take a look at this change?

Thanks,
Ed

compnerd requested changes to this revision.Mar 16 2015, 7:36 PM
compnerd edited edge metadata.
compnerd added inline comments.
src/Unwind/AddressSpace.hpp
60

This part is safe.

71

Hmm..Im not sure that this is the best way to approach this. What if you want to do remote unwinding, where your host is 32-bit and your remote is 64-bit or vice-versa. This breaks in that case, as you are using __INTPTR_WIDTH__. Or am I overlooking something?

This revision now requires changes to proceed.Mar 16 2015, 7:36 PM
ed added inline comments.Mar 16 2015, 11:19 PM
src/Unwind/AddressSpace.hpp
71

This piece of code always has to be executed on the target. This macro is simply needed to fulfil the contract of dl_iterate_phdr(). This function is executed on the target system to obtain the address of the ELF program headers of the running process.

Using a width unequal to __INTPTR_WIDTH__ wouldn't make any sense. In fact, it would only generate compiler errors, for the reason that struct dl_phdr_info::dlpi_phdr is defined as a const ElfW(Phdr) *. See the following man page for details:

http://linux.die.net/man/3/dl_iterate_phdr

jroelofs added inline comments.
src/Unwind/AddressSpace.hpp
71

@compnerd: Does the remote unwinding stuff even build? I don't see a declaration for template <typename T> class Pointer32;, nor for class LittleEndian;...

Either way, I think @ed is right here: no matter the pointer size of the target process, if we link against dl_iterate_phdr(), we have to use the pointer size for *this* process, not the pointer size for the *other* process. This will work unless the ElfW's are used in the interface between LocalAddressSpace and OtherAddressSpace<Foo>.

compnerd accepted this revision.Mar 18 2015, 7:18 PM
compnerd edited edge metadata.
compnerd added inline comments.
src/Unwind/AddressSpace.hpp
67

I tend to prefer #if !defined() as it makes it easier to extend in the future if necessary.

70

It would be nice if you renamed ElfW2 and ElfW3 to something more generic (GLUE, GLUE_EXPANDED?). They don't have anything to do with ELF specifically.

71

Ah, right, this would be run on the remote thread. Fair enough.

This revision is now accepted and ready to land.Mar 18 2015, 7:18 PM
ed updated this revision to Diff 22929.Mar 31 2015, 1:49 AM
ed updated this revision to Diff 22930.
ed retitled this revision from libc++abi: Make the .eh_frame_hdr code work on FreeBSD and CloudABI as well to libc++abi: Make the .eh_frame_hdr code work on FreeBSD as well.
ed updated this object.
ed edited edge metadata.
ed removed rL LLVM as the repository for this revision.
ed added inline comments.Mar 31 2015, 1:52 AM
src/Unwind/AddressSpace.hpp
67

Done!

70

I looked into it a bit more closely and it looks like we can avoid it altogether.

Systems that don't provide ElfW() seem to provide additional typedefs so that we can just Elf_XXX. There is no need to put the INTPTR_WIDTH in between. Do you think that looks all right?

emaste added a subscriber: emaste.Apr 9 2015, 12:53 PM
emaste accepted this revision.Apr 29 2015, 1:05 PM
emaste added a reviewer: emaste.

LGTM

ed closed this revision.May 10 2015, 1:21 PM