This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Partially revert r297174 to fix build on at least FreeBSD.
ClosedPublic

Authored by bsdjhb on Sep 20 2017, 4:13 PM.

Details

Summary

The changes in r297174 moved the #include of <link.h> on FreeBSD (and
probably other systems) inside of the open 'libunwind' namespace
causing various system-provided types such as pid_t to be declared in
this namespace rather than the global namespace. Fix this by moving
the relevant declarations before the 'libunwind' namespace is opened,
but still using the cleaned up declarations from r297174.

Diff Detail

Repository
rL LLVM

Event Timeline

bsdjhb created this revision.Sep 20 2017, 4:13 PM

I posted a sample compile failure on FreeBSD/amd64 in the review for the earlier change: D30696

compnerd added inline comments.Sep 20 2017, 4:18 PM
src/AddressSpace.hpp
38 ↗(On Diff #116109)

Why does this need a #ifndef __APPLE__ guard?

bsdjhb added inline comments.Sep 20 2017, 4:42 PM
src/AddressSpace.hpp
38 ↗(On Diff #116109)

This is to mimic the same conditionals in the existing code. The current flow is '#ifdef APPLE <do stuff> #else <the code being moved> #endif' I chose to use '#ifndef APPLE' rather than having an empty '#if APPLE' body but can use whichever is preferred. I could also push the !defined(APPLE) down into the other conditionals.

ed edited edge metadata.Sep 20 2017, 11:23 PM

Hey! Sorry for the breakage. I thought I did a good job of testing this change on various operating systems, but somehow this still fell through the cracks. Odd.

src/AddressSpace.hpp
268 ↗(On Diff #116109)

Considering that this block only declares an inline function, maybe it would make sense to move this to the top of the source file as well? That way we can keep the existing order of #ifs.

ed accepted this revision.Sep 20 2017, 11:24 PM
This revision is now accepted and ready to land.Sep 20 2017, 11:24 PM
compnerd added inline comments.Sep 21 2017, 9:33 AM
src/AddressSpace.hpp
38 ↗(On Diff #116109)

_LIBUNWIND_ARM_EHABI should never be defined under a Darwin build AFAIK. I would just entirely drop the __APPLE__ check.

bsdjhb added inline comments.Sep 21 2017, 10:56 AM
src/AddressSpace.hpp
38 ↗(On Diff #116109)

Does Darwin define _LIBUNWIND_SUPPORT_DWARF_UNWIND (line 46 below)? I also wonder what you think of @ed 's suggestion of moving the function for the #ifdef APPLE case up here as well?

Moving the __APPLE__ case up to the top of the file would be nice, although, it does move a compatibility implementation too. I suppose that it isn't too terrible though.

src/AddressSpace.hpp
38 ↗(On Diff #116109)

Yeah, it does. Although, I think we could just do:

#elif defined(_LIBUNWIND_ARM_EHABI) || (!defined(__APPLE__) && defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND))
bsdjhb updated this revision to Diff 116241.Sep 21 2017, 11:54 AM
  • Move the Apple-specific _dyld_find_unwind_sections up above the namespace
ed accepted this revision.Sep 21 2017, 12:27 PM
compnerd accepted this revision.Sep 21 2017, 1:30 PM
bsdjhb retitled this revision from Partially revert r297174 to fix build on at least FreeBSD. to [libunwind] Partially revert r297174 to fix build on at least FreeBSD..Sep 21 2017, 2:24 PM
This revision was automatically updated to reflect the committed changes.