Page MenuHomePhabricator

Improve readability and correctness of the OS specific libunwind bits.
ClosedPublic

Authored by ed on Mar 7 2017, 7:58 AM.

Details

Summary

All of the access to __exidx_*, dl_iterate_phdr(), etc. is specific to
the findUnwindSections() function. Right now all of the includes and
declarations related to them are scattered throughout the source file.
For example, for <link.h>, we have a full list of operating systems
guarding the #include, even though the code that uses dl_iterate_phdr()
miraculously doesn't use the same list.

Change the code so that findUnwindSections() is preceded by a block of
#ifdefs that share the same structure as the function itself. First
comes all of the macOS specific bits, followed by bare-metal ARM,
followed by ELF EHABI + DWARF.

This actually allows us to build a copy of libunwind without any
specific ifdefs for NetBSD, CloudABI, etc. It likely also unbreaks the
build of libunwind on FreeBSD/armv6, though I can't confirm.

Diff Detail

Repository
rL LLVM

Event Timeline

ed created this revision.Mar 7 2017, 7:58 AM
compnerd accepted this revision.Mar 7 2017, 9:52 AM

Not sure if I see the movement as being a real improvement. However, it is no worse than before, and not having the explicit list of targets is a huge win.

This revision is now accepted and ready to land.Mar 7 2017, 9:52 AM
This revision was automatically updated to reflect the committed changes.
bsdjhb added a subscriber: bsdjhb.Sep 20 2017, 3:41 PM

This broke the build on FreeBSD it seems because the include of <link.h> is now done inside of the open 'libunwind' namespace. <link.h> pulls in <sys/types.h> causing types like pid_t to be defined in the libunwind namespace which then fails to build:

In file included from /home/john/work/git/llvm/runtimes/libunwind/src/libunwind.cpp:28:
In file included from /home/john/work/git/llvm/runtimes/libunwind/src/UnwindCursor.hpp:20:
In file included from /usr/include/pthread.h:46:
/usr/include/sched.h:236:24: error: C++ requires a type specifier for all
      declarations
int     sched_getparam(pid_t, struct sched_param *);
                       ^
/usr/include/sched.h:237:28: error: unknown type name 'pid_t'; did you mean
      'libunwind::pid_t'?
int     sched_getscheduler(pid_t);
                           ^
/usr/include/sys/types.h:183:18: note: 'libunwind::pid_t' declared here
typedef __pid_t         pid_t;          /* process id */
                        ^

While we could close the namespace inside of each of the #ifdef blocks, I wonder if it wouldn't be cleaner to move this back up to where it was originally before the namespace opens but maybe inside a '#ifndef APPLE'?

@bsdjhb shouldn't link.h be includable in a C++ header without worrying about the namespacing? But, moving the declarations is certainly reasonable.