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

[stefson]

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.

[mgorny]

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.

[stefson]

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?

[mclow.lists]

In D53127#1261618, @stefson wrote:

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__))

[mclow.lists]

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]

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

//Edit: correct #if

[mclow.lists]

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__?

[stefson]

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]

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

Any feedback?

[compnerd]

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?

[mgorny]

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.

[stefson]

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]

here is the full build log of libcxxabi-7.0.0:

libcxxabi-7.0.0-gcc.gz3 KBDownload

it includes the patch from D42242

[stefson]

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

[stefson]

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

src/cxa_exception.hpp

 void     __setExceptionClass  (      _Unwind_Exception*, uint64_t);
 bool     __isOurExceptionClass(const _Unwind_Exception*);
 
+#if defined(__arm__) && defined(__ARM_EABI_UNWINDER__)
+// missing values from _Unwind_Reason_Code enum
+#define _URC_FATAL_PHASE2_ERROR ((_Unwind_Reason_Code)2)
+#define _URC_FATAL_PHASE1_ERROR ((_Unwind_Reason_Code)3)
+#define _URC_NORMAL_STOP ((_Unwind_Reason_Code)4)
+#endif
+
 struct _LIBCXXABI_HIDDEN __cxa_exception {
 #if defined(__LP64__) || defined(_LIBCXXABI_ARM_EHABI)
     // This is a new field to support C++ 0x exception_ptr.