Page MenuHomePhabricator

Fix missing defines in libc++abi when compiled against libgcc's arm unwind (follow-up of D42242)
Needs ReviewPublic

Authored by stefson on Oct 10 2018, 11:52 PM.

Details

Summary

This is follow-up of https://bugs.llvm.org/show_bug.cgi?id=35945, which reports that libc++ doesn't build for the Raspberry PI.

Errors related to char[8] are fixed now, but there are some missing values from _Unwind_Reason_Code enum and wrong behavior of _US_FORCE_UNWIND

this is taken (with permission) from: https://src.fedoraproject.org/rpms/v8/blob/master/f/v8-6.7.17-fix-gcc-unwind-header.patch

Not sure about who should review this, so I added all reviewers from D42242

The reason this went unnoticed for so long is that allmost all distros use clang to build their libcxx/libcxxabi package, which passes fine.

Diff Detail

Repository
rCXXA libc++abi

Event Timeline

stefson created this revision.Oct 10 2018, 11:52 PM
mgorny requested changes to this revision.Oct 11 2018, 1:21 AM

To be honest, I'm not really convinced this is the best way to do it. I don't really know why GCC's libunwind doesn't declare those constants, and I have no clue what the implications of forcing them on top of possibly-gcc libunwind might be.

In any case, I think you shouldn't declare them when using clang, at least. Furthermore, I suppose a future version of GCC may bring the missing enum constants, so maybe it'd be best to check whether they're present and define them only when missing (e.g. using a CMake check).

src/cxa_exception.hpp
31

__GNUC__ also defined when using clang, so the definitions from clang will be overwritten by this.

This revision now requires changes to proceed.Oct 11 2018, 1:21 AM

Well, if __defined(___GNUC___) does target both gcc and clang, it doesn't make much sense in the first place. How can I state the defined marcro only for gcc?

Well, if __defined(___GNUC___) does target both gcc and clang, it doesn't make much sense in the first place. How can I state the defined marcro only for gcc?

In libc++'s <__config> header, we check for clang first (using #if defined(__clang__)) and then if that fails, check for GCC (using #elif defined(__GNUC__))

BTW, I'm pretty sure that the title here is wrong. It's not "compiling with GCC" that's the problem, it's "compiling with a libsupc++" or something like that.
(and that should drive the tests _URC_FATAL_PHASE2_ERROR etc)

stefson added a comment.EditedOct 11 2018, 9:32 AM

How about this as a guard in line 31? #if defined(__arm__) && !defined(__clang__)

//Edit: correct #if

mclow.lists added inline comments.Oct 11 2018, 9:54 AM
src/cxa_exception.hpp
31

Is there an identifier that we can check to say "we're using the GNU unwind library"? Maybe __ARM_EABI_UNWINDER__?

both of these guards work, in terms of libc++abi can be compiled with either clang or gcc:

#if defined(__arm__) && !defined(__clang__)
#if defined(__arm__) && defined(__ARM_EABI_UNWINDER__)

any unforeseen problems with either of them?

(I use llvm-libunwind, and the patch is tested against it)

stefson updated this revision to Diff 169553.Oct 13 2018, 1:43 AM

Still not really sure wether (__ARM_EABI_UNWINDER__) is better or not.

Any feedback?

stefson retitled this revision from Fix declaration of _URC_FATAL_PHASE1_ERROR in libc++abi when compiled with gcc (follow-up of D42242) to Fix declaration of _URC_FATAL_PHASE1_ERROR in libc++abi when compiled with libsupc++ (follow-up of D42242).Oct 13 2018, 6:05 AM

I'm not sure I understand this patch. Building libc++abi with libsupc++ doesn't make sense. They are both providing the same API. Are you trying to build against libgcc? The GNU Savannah libunwind as well as the LLVM libunwind provide the definitions AFAICT. libgcc has the definition in unwind-common.h.

src/cxa_personality.cpp
1112 ↗(On Diff #169553)

Unnecessary change?

I think there's some confusion here. This has nothing to do with libsupc++ but FWIU with gcc's libunwind header which is apparently used when compiling it with gcc.

@stefson, I think it wouldn't be unreasonable to include the compiler error in the description.

src/cxa_personality.cpp
1112 ↗(On Diff #169553)

This is definitely needed to solve one of the problems. I'm not convinced it belongs to the same patch.

mgorny resigned from this revision.Oct 14 2018, 10:32 AM

Thanks for your input. Regarding the title, I've been critisized for suggesting that this issue is not about gcc misscompiling libc++abi, but for using gcc's ARM unwind abi. It's that I just now got my head around this, and I'm going to change it to reflect that. To answer the question: yes, I think this is about building against libgcc's arm unwind implemention and its lack of a few defines.

This patch is the missing part from the aforementioned fedora patch, the main part has been merged in D42242.

Now, it's been a bit naive of me to assume you'd accept it without requesting changes, as the patch makes things work for arm but may not be upstreamable, or not that easily.

stefson retitled this revision from Fix declaration of _URC_FATAL_PHASE1_ERROR in libc++abi when compiled with libsupc++ (follow-up of D42242) to Fix missing defines in libc++abi when compiled against libgcc's arm unwind (follow-up of D42242).Oct 14 2018, 10:52 AM
stefson edited the summary of this revision. (Show Details)

here is the full build log of libcxxabi-7.0.0:

it includes the patch from D42242

todo: use CMake check to check for libgcc arm unwind, and add or don't add defines based on that.

stefson updated this revision to Diff 169648.Oct 14 2018, 11:20 PM

removed questionable hack from cxa_personality, this still needs a fix but it will be taken care of in another pullrequest.