The base system unwind.h (since base c00d34566536) aligns struct _Unwind_Exception using __attribute__((__aligned__)) which is 16 byte alignment on i386 and amd64. The unwind.h from devel/libunwind used to have this as well but replaced it (erroneously) with _Alignas(8) in version 1.6 which is incompatible. This difference affects the layout (size, alignment, padding, field offsets) of struct __cxa_exception in cxxabi.h. By default libreoffice depends on gstreamer which by default depends on devel/libunwind, so with default options libreoffice is built with the wrong _Unwind_Exception and __cxa_exception. This leads to crashes already during build, at least on i386 (gengal.bin segfaults as reported in bug 262008). For amd64, libreoffice has hacks in place to detect ABI differences at runtime which also happen to work in this case. I suspect the same happens with openoffice. Either devel/libunwind needs a patch that reverts https://github.com/libunwind/libunwind/commit/da8dc856ab5646e04160060aae9425db3f5428ce#diff-0b26e6872c56fde17faa6297bdcd1cc72357de35a30894de5d48b4006e5fe83a= but how does that affect older versions of FreeBSD? Or it could be configured with --disable-unwind-header so its unwind.h isn't installed, but then how does that affect other ports? Additionally, the comments in struct __cxa_exception in cxxabi.h state that _Unwind_Exception is 64-bit aligned and that adding referenceCount at the beginning is safe. This may have been true at the time because LLVM libunwind added __attribute__((__aligned__)) later (to be GCC compatible I think: https://github.com/llvm/llvm-project/commit/19f802ff68361af0a28c8ad6e12daf9bd740b96d), but it's not safe now and the GCC folks were bitten by this as well (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38732). For architectures where __attribute__((__aligned__)) means 16 byte alignment adding it to _Unwind_Exception also adds 8 bytes of padding right before the unwindHeader field in __cxa_exception which breaks ABI. That's why libreoffice on amd64 has the hacks I mentioned above. Could we change our libcxxrt similar to what libc++abi did in https://github.com/llvm/llvm-project/commit/f2a436058fcbc11291e73badb44e243f61046183? I believe that way we maintain ABI compatibility at least on 64-bit architectures, i.e. libreoffice built with FreeBSD 13.0 cxxabi.h and unwind.h (or devel/libunwind unwind.h with _Alignas(8)) would then run on FreeBSD 13.1. Can we delay FreeBSD 13.1 release until this is fully understood and sorted out?
See https://github.com/libcxxrt/libcxxrt/pull/18
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=434215c26da3c6acf2423ab93ff2b41b2d823cc8 commit 434215c26da3c6acf2423ab93ff2b41b2d823cc8 Author: Dimitry Andric <dim@FreeBSD.org> AuthorDate: 2022-04-19 16:11:11 +0000 Commit: Dimitry Andric <dim@FreeBSD.org> CommitDate: 2022-04-19 16:11:11 +0000 Merge libcxxrt commit 45ca8b1942090226ba9368caeeeabc0d4ee41ad6 Insert padding in __cxa_exception struct for compatibility Similar to https://github.com/llvm/llvm-project/commit/f2a436058fcb, the addition of __attribute__((__aligned__)) to _Unwind_Exception (in commit b9616964) causes implicit padding to be inserted before the unwindHeader field in __cxa_exception. Applications attempt to get at the earlier fields in __cxa_exception, so preserve the same negative offsets in __cxa_exception, by moving the padding to the beginning of the struct. The assumption here is that if the ABI is not aware of the padding before unwindHeader and put the referenceCount/primaryException in there, no padding should exist before unwindHeader. This should make libreoffice's custom exception handling mechanisms work correctly, even if it was built against an older cxxabi.h/unwind.h pair. PR: 263370 MFC after: 3 days contrib/libcxxrt/cxxabi.h | 7 +++++++ 1 file changed, 7 insertions(+)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=06394327dd1fd77c66af06f6f89713c5142fe1b2 commit 06394327dd1fd77c66af06f6f89713c5142fe1b2 Author: Dimitry Andric <dim@FreeBSD.org> AuthorDate: 2022-04-19 16:11:11 +0000 Commit: Dimitry Andric <dim@FreeBSD.org> CommitDate: 2022-04-19 18:19:52 +0000 Merge libcxxrt commit 45ca8b1942090226ba9368caeeeabc0d4ee41ad6 Insert padding in __cxa_exception struct for compatibility Similar to https://github.com/llvm/llvm-project/commit/f2a436058fcb, the addition of __attribute__((__aligned__)) to _Unwind_Exception (in commit b9616964) causes implicit padding to be inserted before the unwindHeader field in __cxa_exception. Applications attempt to get at the earlier fields in __cxa_exception, so preserve the same negative offsets in __cxa_exception, by moving the padding to the beginning of the struct. The assumption here is that if the ABI is not aware of the padding before unwindHeader and put the referenceCount/primaryException in there, no padding should exist before unwindHeader. This should make libreoffice's custom exception handling mechanisms work correctly, even if it was built against an older cxxabi.h/unwind.h pair. PR: 263370 Approved by: re (gjb, early MFC) MFC after: immediately (cherry picked from commit 434215c26da3c6acf2423ab93ff2b41b2d823cc8) contrib/libcxxrt/cxxabi.h | 7 +++++++ 1 file changed, 7 insertions(+)
A commit in branch releng/13.1 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=07bbb50f59ae4c42ab82c11ccda7b25aeea4f7d7 commit 07bbb50f59ae4c42ab82c11ccda7b25aeea4f7d7 Author: Dimitry Andric <dim@FreeBSD.org> AuthorDate: 2022-04-19 16:11:11 +0000 Commit: Dimitry Andric <dim@FreeBSD.org> CommitDate: 2022-04-19 20:23:20 +0000 Merge libcxxrt commit 45ca8b1942090226ba9368caeeeabc0d4ee41ad6 Insert padding in __cxa_exception struct for compatibility Similar to https://github.com/llvm/llvm-project/commit/f2a436058fcb, the addition of __attribute__((__aligned__)) to _Unwind_Exception (in commit b9616964) causes implicit padding to be inserted before the unwindHeader field in __cxa_exception. Applications attempt to get at the earlier fields in __cxa_exception, so preserve the same negative offsets in __cxa_exception, by moving the padding to the beginning of the struct. The assumption here is that if the ABI is not aware of the padding before unwindHeader and put the referenceCount/primaryException in there, no padding should exist before unwindHeader. This should make libreoffice's custom exception handling mechanisms work correctly, even if it was built against an older cxxabi.h/unwind.h pair. PR: 263370 Approved by: re (gjb) MFC after: immediately (cherry picked from commit 434215c26da3c6acf2423ab93ff2b41b2d823cc8) (cherry picked from commit 06394327dd1fd77c66af06f6f89713c5142fe1b2) contrib/libcxxrt/cxxabi.h | 7 +++++++ 1 file changed, 7 insertions(+)
This can be marked as closed now, correct?
I believe so; it would be great if Tijl can confirm.
Created attachment 233364 [details] devel/libunwind patch There's just devel/libunwind to deal with now. Here's a patch for that. - Add --disable-cxx-exceptions so libunwind does not implement the _Unwind_* API and does not use its internal unwind.h. The base system libgcc_s provides this API. This is already the upstream default for most architectures. - Add --disable-unwind-header to prevent installation of unwind.h. Instead, create a symbolic link on installation if necessary to make the base system unwind.h visible.
dim and I discussed on IRC and there's still an issue with i386, this introduces an ABI breakage from 13.0. dim installed 13.0 libreoffice and then replaced libcxxrt, and libreoffice crashes. We're discussing a patch to restore the layout of __cxa_exception and _Unwind_Exception. This change would be for 32-bit 13.x only, would not be applied to main. (Or maybe even only i386 13.x) contrib/libcxxrt/unwind-itanium.h contrib/llvm-project/libunwind/include/unwind.h
(In reply to Tijl Coosemans from comment #7) Is setting these options unconditionally compatible with 12.x / 13.0?
(In reply to Ed Maste from comment #8) On 32 bit architectures adding __aligned__ to _Unwind_Exception increases its size. That's not a problem for _Unwind_Exception because nothing accesses the extra padding. It is a problem for __cxa_exception because that also increases in size and that affects negative offsets from the end of that struct. I assumed that 32 bit architectures aren't that important and that this ABI breakage is ok. If necessary you can make libreoffice run with newer libcxxrt by patching deleteException in bridges/source/cpp_uno/gcc3_linux_intel/except.cxx to make it do something similar to what deleteException in bridges/source/cpp_uno/gcc3_linux_x86-64/except.cxx does. Note that libreoffice built with default options on i386 will *always* crash with new libcxxrt (16 byte alignment) because of the incorrect definition of _Unwind_Exception in devel/libunwind unwind.h (8 byte alignment). The definition is also wrong for old libcxxrt (4 byte alignment). I believe it doesn't crash there only by accident.
(In reply to Ed Maste from comment #9) The patch doesn't work. There would have to be symlinks to unwind-arm.h and unwind-itanium.h as well, but I'm working on an actual file instead of symlinks. I noticed now though that clang and gcc never include the new /usr/include/unwind.h. They include their own internal unwind.h (from /usr/lib/clang, /usr/local/llvm*/lib/clang, or /usr/local/lib/gcc*/gcc). Should we perhaps not install these internal headers? That would also have to be merged to releng/13.1 then.
(In reply to Tijl Coosemans from comment #11) In the base system we have historically never installed those headers, since they conflict with our own versions. For the full list, see https://cgit.freebsd.org/src/tree/lib/clang/headers/Makefile#n142 : # Headers which possibly conflict with our own versions: .ifdef INSTALL_CONFLICTING_CLANG_HEADERS INCS+= float.h INCS+= intrin.h INCS+= inttypes.h INCS+= iso646.h INCS+= limits.h INCS+= stdalign.h INCS+= stdarg.h INCS+= stdatomic.h INCS+= stdbool.h INCS+= stddef.h INCS+= stdint.h INCS+= stdnoreturn.h INCS+= tgmath.h INCS+= unwind.h INCS+= varargs.h .endif # INSTALL_CONFLICTING_CLANG_HEADERS The llvm ports have a different method to disable some of these, but not exactly the same ones, unfortunately: https://cgit.freebsd.org/ports/tree/devel/llvm13/files/patch-clang_lib_Headers_CMakeLists.txt Effectively, this stops the following from being installed: - limits.h - stdalign.h - stdarg.h - stdatomic.h - stdbool.h - stddef.h - stdint.h - stdnoreturn.h - varargs.h but not unwind.h! So yet *another* incompatible header, as clang's internal unwind.h does not include the #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 block.
(In reply to Dimitry Andric from comment #12) On FreeBSD we could make devel/llvm* not install unwind.h, but I think the DragonflyBSD people want to keep it. The header has this at the beginning: #if defined(__APPLE__) && __has_include_next(<unwind.h>) /* Darwin (from 11.x on) provide an unwind.h. If that's available, * use it. defined(__FreeBSD__) could be added here. The missing block with the "reserved" field at the end is not that important. The __aligned__ attribute already implies it. The problem is the presence of this __aligned__ for old libcxxrt.
Created attachment 233373 [details] devel/libunwind patch2 Updated patch for devel/libunwind.
For stable/13 and 13.1 I think we want to preserve the ABI and drop the extra padding and aligned attribute. This is a version of the patch dim and I discussed: https://github.com/emaste/freebsd/commit/4a42b3aa9a194203e7ddf43f88de28a81dfd5041 contrib/libcxxrt/unwind-itanium.h - #if __SIZEOF_POINTER__ == 4 + #if __SIZEOF_POINTER__ == 4 && defined(USE_UPSTREAM_ABI_NOT_FREEBSD_13_0_ABI) uint32_t reserved[3]; #endif - } __attribute__((__aligned__)); + } + #if __SIZEOF_POINTER__ != 4 || defined(USE_UPSTREAM_ABI_NOT_FREEBSD_13_0_ABI) + __attribute__((__aligned__)) + #endif + ; and similar in contrib/llvm-project/libunwind/include/unwind.h
(In reply to Ed Maste from comment #15) LGTM
I've been digging deeper into libcxxrt and now I think preserving negative offsets in __cxa_exception isn't enough. Positive offsets also need to be preserved meaning that it isn't safe to add fields to the beginning like the comments say. Both libreoffice and openoffice look at __cxa_get_globals()->caughtExceptions->adjustedPtr, where caughtExceptions is a pointer to __cxa_exception as known by libcxxrt so if libreoffice/openoffice have been compiled with an older __cxa_exception definition it won't work correctly. Libreoffice has another hack for this but it's behind an ifdef _LIBCPPABI_VERSION so it's not currently enabled. I'm afraid that to preserve ABI all changes need to be reverted in stable/13 and releng/13.1.
(In reply to commit-hook from comment #2) Dimitry, the libc++abi commit also added "void* reserve" to __cxa_dependent_exception. In libcxxrt that struct is defined in exception.cc. It needs to be added there as well because it needs to have the same layout as __cxa_exception.
Created attachment 233450 [details] devel/libunwind patch3 rebased + install unwind.h on stable/13 as well (if changes are reverted on stable/13, for i386 or all archs, then only on FreeBSD 14 are all implementations of unwind.h (base, lang/gcc*, devel/llvm*) compatible with each other and no wrapper unwind.h is needed) Po-Chuan, this should be the final version of the patch. Can it be committed?
(In reply to Tijl Coosemans from comment #17) > Libreoffice has another hack for this but it's behind an ifdef >_LIBCPPABI_VERSION so it's not currently enabled. libreoffice extends the built-in LIBCBBABI hack as of: commit f88f34d1b4a00d94c00aae1b3097c9c637e36397 Author: Dimitry Andric <dim@FreeBSD.org> Date: Wed Mar 9 10:28:27 2022 +0300 editors/libreoffice: work around changed alignment of __cxa_exception LibreOffice has special detection for the change in alignment and size of struct cxa_exception, when using libc++abi. However, this updated alignment also applies to libunwind and upstream libcxxrt, and will soon apply to our libcxxrt too. Enable the special detection unconditionally for x86_64 and aarch64, so libreoffice packages built on 13.0-R (with the old alignment) will seamlessly work on 13.1-R (which will have the new alignment). PR: 262008 I don't think this was done for OpenOffice though.
(In reply to Tijl Coosemans from comment #19) If you all agree with this patch, feel free to commit it. Thanks!
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=975a5c1057579603c4041c2b8e3ac0933ea0eb89 commit 975a5c1057579603c4041c2b8e3ac0933ea0eb89 Author: Tijl Coosemans <tijl@FreeBSD.org> AuthorDate: 2022-04-27 16:51:01 +0000 Commit: Tijl Coosemans <tijl@FreeBSD.org> CommitDate: 2022-04-27 16:53:02 +0000 devel/libunwind: use base system unwind.h - Add --disable-cxx-exceptions so libunwind does not implement the _Unwind_* API and does not use its internal unwind.h. The base system libgcc_s provides this API. This was already the upstream default for most architectures. - Add --disable-unwind-header to prevent installation of unwind.h. Instead, install an unwind.h that forwards to the base system unwind.h, but not on FreeBSD 14 where all implementations of unwind.h (base, lang/gcc*, and devel/llvm*) are compatible. PR: 263370 Approved by: sunpoet devel/libunwind/Makefile | 11 ++++++++++- devel/libunwind/files/unwind.h (new) | 26 ++++++++++++++++++++++++++ devel/libunwind/pkg-plist | 1 - 3 files changed, 36 insertions(+), 2 deletions(-)
A commit in branch 2022Q2 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=f041aca7adf2d5ee3742d2c3e98fdc4e7c045fe5 commit f041aca7adf2d5ee3742d2c3e98fdc4e7c045fe5 Author: Tijl Coosemans <tijl@FreeBSD.org> AuthorDate: 2022-04-27 16:51:01 +0000 Commit: Tijl Coosemans <tijl@FreeBSD.org> CommitDate: 2022-04-27 16:59:02 +0000 devel/libunwind: use base system unwind.h - Add --disable-cxx-exceptions so libunwind does not implement the _Unwind_* API and does not use its internal unwind.h. The base system libgcc_s provides this API. This was already the upstream default for most architectures. - Add --disable-unwind-header to prevent installation of unwind.h. Instead, install an unwind.h that forwards to the base system unwind.h, but not on FreeBSD 14 where all implementations of unwind.h (base, lang/gcc*, and devel/llvm*) are compatible. PR: 263370 Approved by: sunpoet (cherry picked from commit 975a5c1057579603c4041c2b8e3ac0933ea0eb89) devel/libunwind/Makefile | 11 ++++++++++- devel/libunwind/files/unwind.h (new) | 26 ++++++++++++++++++++++++++ devel/libunwind/pkg-plist | 1 - 3 files changed, 36 insertions(+), 2 deletions(-)