Re: git: b2127b6f1ae2 - stable/13 - Install unwind.h into /usr/include

Reply: Tomoaki AOKI : "Re: git: b2127b6f1ae2 - stable/13 - Install unwind.h into /usr/include"
In reply to: David Chisnall : "Re: git: b2127b6f1ae2 - stable/13 - Install unwind.h into /usr/include"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Dimitry Andric <dim_at_FreeBSD.org>
Date: Sun, 06 Mar 2022 10:13:58 UTC
In an ideal world, we would just merge https://github.com/libcxxrt/libcxxrt/commit/88bdf6b290da2f32fe212564bfad9c61481fb878 ("Specify double-word alignment for ARM unwind. This matches the latest revisions from libunwind.") and the corresponding https://github.com/libcxxrt/libcxxrt/commit/b96169641f794f7b8e28c1b78b66bb780445d0cd ("Updated Itanium unwind. This matches the latest revisions from libunwind."). Then at least libcxxrt's and libunwind's idea of the struct layouts and alignments would be the same.

However, that would still affect older __cxa_exception and _Unwind_Exception consumers, at least those that rely explicitly on the old sizes and alignments of those structs. We could bump libcxxrt.so to .2, but then we'd still have to maintain the old .1 version for those old consumers.

Btw, now that I think about it, I think we merged the unwind.h changes for 13.1, and we should really fix them up before 13.1 goes out the door.

-Dimitry

> On 6 Mar 2022, at 10:17, David Chisnall <theraven@FreeBSD.org> wrote:
> 
> libcxxrt provides its own unwind.h that includes an abstraction layer over the differences between the Arm and Itanium ABIs.  It provides all of the standard definitions in addition to this, so you should be able to build other things against it, but you won’t be able to build it with a different unwind.h.
> 
> You could potentially split the unwind.h that it provides into two headers, one with only the standard definitions and one with the others, I’d be happy to review a PR to do this.
> 
> David
> 
>> On 6 Mar 2022, at 08:46, Tomoaki AOKI <junchoon@dec.sakura.ne.jp> wrote:
>> 
>> On Sat, 5 Mar 2022 23:33:54 +0100
>> Dimitry Andric <dim@FreeBSD.org> wrote:
>> 
>>> On 5 Mar 2022, at 22:34, Dimitry Andric <dim@FreeBSD.org> wrote:
>>>> 
>>>> On 5 Mar 2022, at 03:36, Tomoaki AOKI <junchoon@dec.sakura.ne.jp> wrote:
>>> ...
>>>> So according to the spec, casting the void pointer 'thrown_exception' to
>>>> a __cxa_exception pointer, then subtracting 1, should give you the
>>>> original __cxa_exception struct. In this case, it subtracts 8 bytes,
>>>> going from 0x87b5aff00 to 0x87b5afe88.
>>> 
>>> Ugh, actually this was 120 bytes!
>>> 
>>> 
>>>> Now I do exactly the same in the libreoffice frame one below, where the
>>>> incoming void pointer 'pExc' is the previous 'thrown_exception' value:
>>>> 
>>>> (gdb) frame 1
>>>> #1  gcc3::deleteException (pExc=0x87b5aff00) at bridges/source/cpp_uno/gcc3_linux_x86-64/except.cxx:139
>>>> 139         OUString unoName( toUNOname( header->exceptionType->name() ) );
>>>> (gdb) print pExc
>>>> $33 = (void *) 0x87b5aff00
>>>> (gdb) print static_cast<__cxa_exception*>(pExc)-1
>>>> $34 = (__cxa_exception *) 0x87b5afe80
>>>> 
>>>> So in *this* function, subtracting 1 from a __cxa_exception pointer
>>>> subtracts 16 bytes instead, going from 0x87b5aff00 to 0x87b5afe80!
>>> 
>>> And this was 128 bytes instead. I think I now know what's going on,
>>> which is that our declaration of __cxa_exception changed its size from
>>> 120 bytes to 128 bytes, due to the new unwind headers.
>>> 
>>> Our libcxxrt cxxabi.h header has:
>>> 
>>> struct __cxa_exception
>>> {
>>> ... lots of stuff ...
>>>         /** The language-agnostic part of the exception header. */
>>>         _Unwind_Exception unwindHeader;
>>> };
>>> 
>>> so the last field is a struct _Unwind_Exception. Our libcxxrt
>>> unwind-itanium.h header has:
>>> 
>>> struct _Unwind_Exception
>>>   {
>>>     uint64_t exception_class;
>>>     _Unwind_Exception_Cleanup_Fn exception_cleanup;
>>>     unsigned long private_1;
>>>     unsigned long private_2;
>>>   } ;
>>> 
>>> while libunwind's version has an __aligned__ attribute at the end:
>>> 
>>> struct _Unwind_Exception {
>>>   uint64_t exception_class;
>>>   void (*exception_cleanup)(_Unwind_Reason_Code reason,
>>>                             _Unwind_Exception *exc);
>>> #if defined(__SEH__) && !defined(__USING_SJLJ_EXCEPTIONS__)
>>>   uintptr_t private_[6];
>>> #else
>>>   uintptr_t private_1; // non-zero means forced unwind
>>>   uintptr_t private_2; // holds sp that phase1 found for phase2 to use
>>> #endif
>>> #if __SIZEOF_POINTER__ == 4
>>>   // The implementation of _Unwind_Exception uses an attribute mode on the
>>>   // above fields which has the side effect of causing this whole struct to
>>>   // round up to 32 bytes in size (48 with SEH). To be more explicit, we add
>>>   // pad fields added for binary compatibility.
>>>   uint32_t reserved[3];
>>> #endif
>>>   // The Itanium ABI requires that _Unwind_Exception objects are "double-word
>>>   // aligned".  GCC has interpreted this to mean "use the maximum useful
>>>   // alignment for the target"; so do we.
>>> } __attribute__((__aligned__));
>>> 
>>> (Note that upstream libcxxrt also added the reserved field and aligned
>>> attribute, in https://github.com/libcxxrt/libcxxrt/commit/b9616964 !)
>>> 
>>> The aligned attribute on _Unwind_Exception causes the enclosing
>>> __cxa_exception struct to *also* be aligned maximally, growing it from
>>> 120 to 128 bytes on x86_64.
>>> 
>>> So this is a bit of a fine mess we are in. There are multiple issues
>>> here:
>>> 
>>> 1) We broke the ABI by increasing __cxa_exception's size.
>>> 
>>> 2) We compile libcxxrt against its *own* unwind headers, so it assumes a
>>> 120-byte __cxa_exception size. But all other programs use the libunwind
>>> headers, so they assume a 128 byte __cxa_exception size.
>>> 
>>> I guess LibreOffice is just a good example which breaks because it does
>>> this deep poking in exception-handling land, which most programs never
>>> go near. That said, LibreOffice also includes the unwind.h header
>>> installed by the libunwind-20201110 port, so that is yet *another*
>>> possible incompatibility!
>>> 
>>> But I think we must do something about this. The most backward
>>> compatible change would be to *remove* the aligned attribute from our
>>> _Unwind_Exception declaration, so the old __cxa_exception size is
>>> restored. The problem with that is that we have to carry a patch for
>>> libunwind forever.
>>> 
>>> The other way would be to force libcxxrt to use the libunwind headers
>>> instead of its own, so that at least libcxxrt and libunwind agree on the
>>> size and alignment of all these structures! But that may still lead to
>>> crashes for older consumers.
>>> 
>>> No easy way out, in any case... :-/
>>> 
>>> -Dimitry
>> 
>> Did a quick stupid test by
>> 
>> *Replacing #include "unwind.h" by #include <unwind.h> in
>> contrib/libcxxrt/cxxabi.h, the only file including unwind.h on
>> contrib/cxxrt directory to pick it from standard place.
>> 
>> *Rename unsind*.h to something else just to be sure.
>> 
>> and got errors by missing macros. At least BEGIN_PERSONALITY_FUNCTION.
>> 
>> Insufficient part would be needed to be extracted to additional header.
>> 
>> Emitted errors are as follows:
>> 
>> /usr/src/lib/libcxxrt# make
>> Building /usr/obj/usr/src/amd64.amd64/lib/libcxxrt/auxhelper.o
>> Building /usr/obj/usr/src/amd64.amd64/lib/libcxxrt/dynamic_cast.o
>> Building /usr/obj/usr/src/amd64.amd64/lib/libcxxrt/exception.o
>> /usr/src/contrib/libcxxrt/exception.cc:1084:1: error: unknown type name
>> 'BEGIN_PERSONALITY_FUNCTION'
>> BEGIN_PERSONALITY_FUNCTION(__gxx_personality_v0) ^
>> /usr/src/contrib/libcxxrt/exception.cc:1084:49: error: expected ';'
>> after top level declarator
>> BEGIN_PERSONALITY_FUNCTION(__gxx_personality_v0) ^
>>                                               ;
>> /usr/src/contrib/libcxxrt/exception.cc:1098:42: error: use of
>> undeclared identifier 'exceptionClass'; did you mean 'exception_class'?
>> bool foreignException = !isCXXException(exceptionClass); ^~~~~~~~~~~~~~
>>                                               exception_class
>> /usr/src/contrib/libcxxrt/exception.cc:242:23: note: 'exception_class'
>> declared here static const uint64_t exception_class =
>>                     ^
>> /usr/src/contrib/libcxxrt/exception.cc:1101:2: error: expected
>> unqualified-id if (!foreignException)
>>       ^
>> /usr/src/contrib/libcxxrt/exception.cc:1112:91: error: use of
>> undeclared identifier 'context' reinterpret_cast<unsigned
>> char*>(static_cast<uintptr_t>(_Unwind_GetLanguageSpecificData(context)));
>> ^ /usr/src/contrib/libcxxrt/exception.cc:1116:2: error: expected
>> unqualified-id if (0 == lsda_addr) { return
>> continueUnwinding(exceptionObject, context); } ^
>> /usr/src/contrib/libcxxrt/exception.cc:1137:2: error: expected
>> unqualified-id if (actions & _UA_SEARCH_PHASE)
>>       ^
>> /usr/src/contrib/libcxxrt/exception.cc:1176:2: error: expected
>> unqualified-id if (!(actions & _UA_HANDLER_FRAME))
>>       ^
>> /usr/src/contrib/libcxxrt/exception.cc:1188:2: error: expected
>> unqualified-id else if (foreignException)
>>       ^
>> /usr/src/contrib/libcxxrt/exception.cc:1195:2: error: expected
>> unqualified-id else if (ex->catchTemp == 0)
>>       ^
>> /usr/src/contrib/libcxxrt/exception.cc:1200:2: error: expected
>> unqualified-id else
>>       ^
>> /usr/src/contrib/libcxxrt/exception.cc:1209:2: error: C++ requires a
>> type specifier for all declarations _Unwind_SetIP(context,
>> reinterpret_cast<unsigned long>(action.landing_pad)); ^
>> /usr/src/contrib/libcxxrt/exception.cc:1209:16: error: use of
>> undeclared identifier 'context' _Unwind_SetIP(context,
>> reinterpret_cast<unsigned long>(action.landing_pad)); ^
>> /usr/src/contrib/libcxxrt/exception.cc:1210:2: error: C++ requires a
>> type specifier for all declarations _Unwind_SetGR(context,
>> __builtin_eh_return_data_regno(0), ^
>> /usr/src/contrib/libcxxrt/exception.cc:1210:16: error: use of
>> undeclared identifier 'context' _Unwind_SetGR(context,
>> __builtin_eh_return_data_regno(0), ^
>> /usr/src/contrib/libcxxrt/exception.cc:1211:48: error: use of
>> undeclared identifier 'exceptionObject' reinterpret_cast<unsigned
>> long>(exceptionObject)); ^
>> /usr/src/contrib/libcxxrt/exception.cc:1212:2: error: C++ requires a
>> type specifier for all declarations _Unwind_SetGR(context,
>> __builtin_eh_return_data_regno(1), selector); ^
>> /usr/src/contrib/libcxxrt/exception.cc:1212:16: error: use of
>> undeclared identifier 'context' _Unwind_SetGR(context,
>> __builtin_eh_return_data_regno(1), selector); ^
>> /usr/src/contrib/libcxxrt/exception.cc:1214:2: error: expected
>> unqualified-id return _URC_INSTALL_CONTEXT;
>>       ^
>> fatal error: too many errors emitted, stopping now [-ferror-limit=]
>> 20 errors generated.
>> *** Error code 1
>> 
>> Stop.
>> make: stopped in /usr/src/lib/libcxxrt
>> .ERROR_TARGET='exception.o'
>> .ERROR_META_FILE='/usr/obj/usr/src/amd64.amd64/lib/libcxxrt/exception.o.meta'
>> .MAKE.LEVEL='0'
>> MAKEFILE=''
>> .MAKE.MODE='meta missing-filemon=yes missing-meta=yes silent=yes
>> verbose' _ERROR_CMD='c++  -O2 -pipe -fno-common
>> -isystem /usr/src/contrib/libcxxrt -nostdinc++ -march=haswell
>> -mretpoline -Wno-format-zero-length -fstack-protector-strong
>> -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable
>> -Wno-error=unused-but-set-variable -Wno-tautological-compare
>> -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function
>> -Wno-enum-conversion -Wno-unused-local-typedef
>> -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum
>> -Wno-knr-promoted-parameter -Wno-parentheses -Qunused-arguments
>> -mretpoline -std=c++14    -Wno-c++11-extensions
>> -c /usr/src/contrib/libcxxrt/exception.cc -o
>> exception.o;' .CURDIR='/usr/src/lib/libcxxrt' .MAKE='make' .OBJDIR='/usr/obj/usr/src/amd64.amd64/lib/libcxxrt' .TARGETS='
>> all' DESTDIR='' LD_LIBRARY_PATH='' MACHINE='amd64' MACHINE_ARCH='amd64'
>> MAKEOBJDIRPREFIX='' MAKESYSPATH='/usr/src/share/mk'
>> MAKE_VERSION='20220208'
>> PATH='/sbin:/bin:/usr/sbin:/usr/bin:/usr/games:/usr/local/sbin:/usr/local/bin:/root/bin'
>> SRCTOP='/usr/src' OBJTOP='/usr/obj/usr/src/amd64.amd64'
>> 
>> 
>> --
>> Tomoaki AOKI    <junchoon@dec.sakura.ne.jp>
>> <libcxxrt_errors_20220306-001.log>
>