Bug 38732 - [4.4 Regression] Openoffice.org segfaults with runtime libs built from GCC trunk
Summary: [4.4 Regression] Openoffice.org segfaults with runtime libs built from GCC trunk
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.4.0
: P1 blocker
Target Milestone: 4.4.0
Assignee: Jakub Jelinek
URL:
Keywords: ABI
: 38631 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-01-05 19:58 UTC by Matthias Klose
Modified: 2009-01-08 08:31 UTC (History)
11 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-01-07 12:34:26


Attachments
gcc44-pr38732.patch (3.32 KB, patch)
2009-01-07 13:56 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Klose 2009-01-05 19:58:57 UTC
[forwarded from http://bugs.debian.org/504323]

Stephan Bergmann writes:

Reproducing with libstdc++.so.6 and libgcc_s.so.1 from gcc-4.4-20090102 
in unxlngi6.pro DEV300m38 OOo, the problem is that OOo at 
<http://svn.services.openoffice.org/ooo/tags/DEV300_m38/bridges/source/cpp_uno/gcc3_linux_intel/share.hxx> 
l. 52 assumes a layout of struct __cxa_exception as given in section 
2.2.1 of <http://www.codesourcery.com/public/cxx-abi/abi-eh.html>, while 
GCC 4.4 added a new member

  _Atomic_word referenceCount;

to the start of the struct at 
gcc-4.4-20090102/libstc++-v3/libsupc++/unwind-cxx.h l. 57.  OOo tries to 
get at information stored in the __cxa_exception header in the 
destructor function passed to __cxa_throw (function deleteException at 
<http://svn.services.openoffice.org/ooo/tags/DEV300_m38/bridges/source/cpp_uno/gcc3_linux_intel/except.cxx> 
l. 213), which now fails.

Please somebody clarify if GCC 4.4 adding a new member to struct 
__cxa_exception (and thus deviating from 
<http://www.codesourcery.com/public/cxx-abi/abi-eh.html>) is intended or 
is a mistake.

If it is intended, the OOo code needs to be changed (the information 
deleteException is now trying to retrieve from the __cxa_exception 
header must instead be encoded in the deleteException function itself, 
by dynamically creating instances of deleteException as is already done 
in the OOo bridges for Solaris).  If it is a mistake, the OOo code can 
stay as is and the GCC 4.4 code needs to be changed instead.
Comment 1 Andrew Pinski 2009-01-05 20:10:14 UTC
2008-08-23  Sebastian Redl <sebastian.redl@getdesigned.at>
        
        Add (again) exception propagation support as per N2179.  Feature is
        available only when _GLIBCXX_ATOMIC_BUILTINS_4 is defined.

This is to support C++0x.
Comment 2 Andrew Pinski 2009-01-05 20:19:45 UTC
*** Bug 38631 has been marked as a duplicate of this bug. ***
Comment 3 Andrew Pinski 2009-01-05 20:20:07 UTC
Confirmed.
Comment 4 Andreas Schwab 2009-01-05 21:31:58 UTC
From the ABI document (2.2.1 C++ Exception Objects):
By convention, a __cxa_exception pointer points at the C++ object representing the exception being thrown, immediately following the header. The header structure is accessed at a negative offset from the __cxa_exception pointer.  This layout allows consistent treatment of exception objects from different languages (or different implementations of the same language), and allows future extensions of the header structure while maintaining binary compatibility.

Thus there should be no ABI breakage, unless there is an alignment issue.
Comment 5 Andreas Schwab 2009-01-05 22:16:22 UTC
There is padding between adjustedPtr and unwindHeader because the latter is forced to be maximally aligned.  Due to the additional member the padding was reduced.  Also, the alignment of _Unwind_Exception depends on -mavx.
Comment 6 Andrew Pinski 2009-01-05 22:19:32 UTC
>Also, the alignment of _Unwind_Exception depends on -mavx.

That is a target issue and should be filed as separately.
Comment 7 H.J. Lu 2009-01-05 22:51:50 UTC
(In reply to comment #5)
> There is padding between adjustedPtr and unwindHeader because the latter is
> forced to be maximally aligned.  Due to the additional member the padding was
> reduced.  Also, the alignment of _Unwind_Exception depends on -mavx.
> 

I don't see any need for extra alignment on _Unwind_Exception:

struct _Unwind_Exception
{
  _Unwind_Exception_Class exception_class;
  _Unwind_Exception_Cleanup_Fn exception_cleanup;
  _Unwind_Word private_1;
  _Unwind_Word private_2;

  /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
     Taking that literally does not make much sense generically.  Instead we
     provide the maximum alignment required by any type for the machine.  */
} __attribute__((__aligned__));

I think it should have a fixed alignment for a given target.
Comment 8 H.J. Lu 2009-01-05 22:53:29 UTC
There are

i386/i386.h:#define BIGGEST_ALIGNMENT (TARGET_AVX ? 256: 128)
m68k/m68k.h:#define BIGGEST_ALIGNMENT (TARGET_ALIGN_INT ? 32 : 16)
mcore/mcore.h:#define BIGGEST_ALIGNMENT  (TARGET_8ALIGN ? 64 : 32)
sh/sh.h:#define BIGGEST_ALIGNMENT  (TARGET_ALIGN_DOUBLE ? 64 : 32)
sparc/sparc.h:#define BIGGEST_ALIGNMENT (TARGET_ARCH64 ? 128 : 64)
Comment 9 Andrew Pinski 2009-01-05 22:55:16 UTC
(In reply to comment #8)
> There are
> 
> i386/i386.h:#define BIGGEST_ALIGNMENT (TARGET_AVX ? 256: 128)
> m68k/m68k.h:#define BIGGEST_ALIGNMENT (TARGET_ALIGN_INT ? 32 : 16)
> mcore/mcore.h:#define BIGGEST_ALIGNMENT  (TARGET_8ALIGN ? 64 : 32)
> sh/sh.h:#define BIGGEST_ALIGNMENT  (TARGET_ALIGN_DOUBLE ? 64 : 32)
> sparc/sparc.h:#define BIGGEST_ALIGNMENT (TARGET_ARCH64 ? 128 : 64)
> 

Yes but if you look at the options that the other targets uses for BIGGEST_ALIGNMENT are ABI changing options and not just processor changing options.
Comment 10 Andrew Pinski 2009-01-05 22:59:09 UTC
(In reply to comment #8)
> There are

I filed the x86 issue with -mavx as PR 38736.  This is a regression and really needs to be fixed for before the release of 4.4.
Comment 11 H.J. Lu 2009-01-05 23:08:59 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > There is padding between adjustedPtr and unwindHeader because the latter is
> > forced to be maximally aligned.  Due to the additional member the padding was
> > reduced.  Also, the alignment of _Unwind_Exception depends on -mavx.
> > 
> 
> I don't see any need for extra alignment on _Unwind_Exception:
> 
> struct _Unwind_Exception
> {
>   _Unwind_Exception_Class exception_class;
>   _Unwind_Exception_Cleanup_Fn exception_cleanup;
>   _Unwind_Word private_1;
>   _Unwind_Word private_2;
> 
>   /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>      Taking that literally does not make much sense generically.  Instead we
>      provide the maximum alignment required by any type for the machine.  */
> } __attribute__((__aligned__));
> 
> I think it should have a fixed alignment for a given target.
> 

The ia64 psABI says _Unwind_Exception should be aligned at
double-word. But it isn't very clear what is the size of
double-word.  It does say "long long" is 8 byte doubleword.
We have

[hjl@gnu-6 config]$ grep BIGGEST_ALIGNMENT ia64/ia64.h
#define BIGGEST_ALIGNMENT 128
[hjl@gnu-6 config]$ 

We can't change it now no matter what the size of
double-word is.
Comment 12 Stephan Bergmann 2009-01-06 09:12:52 UTC
I was a bit sloppy in what I wrote before (i.e., in what Matthias turned into the description of this bug); it is the change of offsets of __cxa_exception members caused by the addition of referenceCount that causes problems for OOo:  In the OOo-supplied exception destructor function (which receives a pointer P pointing just past the __cxa_exception header), OOo assumes to access the __cxa_exception member exceptionType at P - 80 bytes.  From my reading of <http://www.codesourcery.com/public/cxx-abi/abi-eh.html> I think OOo is correct in making this assumption, if GCC conforms to that ABI.  However, the addition of a referenceCount member to the front of GCC's __cxa_exception unfortunately causes the "backwards offset" of exceptionType to change from -80 to -76 bytes (as the four bytes of referenceCount cancel out a previous four padding bytes just before the unwindHeader member coming later in the struct).

From the comments to this bug so far, I am not sure whether the GCC maintainers
(a) acknowledge this and will fix it (in which case I do not need to modify OOo), or
(b) acknowledge this but consider OOo to be using undocumented internals of GCC that legitimately changed (in which case I will need to modify OOo), or rather
(c) consider alignment issues of unwindHeader instead (which I think are irrelevant to my problem).
Comment 13 Jakub Jelinek 2009-01-06 13:35:36 UTC
So, referenceCount needs to be in any case nuked from struct __cxa_exception.
IMHO, either we define (probably in a different namespace)
struct __cxa_exception_with_refcount
{
  _Atomic_word referenceCount;
  struct __cxa_exception exc;
};
and corresponding __get_exception_header_with_refcount_from_{obj,ue}
and use this one where needed (at least in eh_alloc.c, eh_ptr.cc and eh_throw.cc), or we just allocate space for one _Atomic_word before the struct
(i.e. allocate extra MAX (__alignof__ (__cxa_exception), sizeof (_Atomic_word))
before it) and use a macro (and or inline) to get address of referenceCount _Atomic_word before the __cxa_exception struct.
Comment 14 Paolo Carlini 2009-01-07 12:23:36 UTC
Hi. Definitely, I like the first approach better. Jakub, are you willing to work on it? Honestly, because I don't think it's the right time to play with ABI issues, I'm ready to revert completely this piece of C++0x work for 4.4.0.
Comment 15 Jakub Jelinek 2009-01-07 12:34:26 UTC
Working on it.
Comment 16 Jakub Jelinek 2009-01-07 13:56:56 UTC
Created attachment 17047 [details]
gcc44-pr38732.patch

Patch I'm going to bootstrap/regtest.
Comment 17 Sebastian Redl 2009-01-07 21:14:36 UTC
I have no idea how a prepended member can change the negative offset of the other struct members. If I did, the bug wouldn't be there.

Is wrapping the struct in another struct the best way to ensure that the layout stays the same? If so, the patch looks good to me.

The OOo.org guys should perhaps add a note for future reference, that if they ever use std::current_exception(), they *will* have to change the code to cope with dependent exceptions.
Comment 18 Jakub Jelinek 2009-01-07 22:50:55 UTC
Subject: Bug 38732

Author: jakub
Date: Wed Jan  7 22:50:42 2009
New Revision: 143170

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=143170
Log:
	PR libstdc++/38732
	* libsupc++/unwind-cxx.h (__cxxabiv1::__cxa_exception): Remove
	referenceCount field again.
	(__cxxabiv1::__cxa_refcounted_exception): New struct.
	(__cxxabiv1::__get_refcounted_exception_header_from_obj,
	__cxxabiv1::__get_refcounted_exception_header_from_ue): New static
	inline functions.
	* libsupc++/eh_alloc.cc (__cxxabiv1::__cxa_allocate_exception,
	__cxxabiv1::__cxa_free_exception): Use __cxa_refcounted_exception
	instead of __cxa_exception.
	* libsupc++/eh_throw.cc (__gxx_exception_cleanup,
	__cxxabiv1::__cxa_throw): Likewise.
	* libsupc++/eh_ptr.cc (std::rethrow_exception,
	std::__exception_ptr::exception_ptr::_M_addref,
	std::__exception_ptr::exception_ptr::_M_release,
	__gxx_dependent_exception_cleanup): Likewise.
	* testsuite/18_support/exception/38732.cc: New test.

Added:
    trunk/libstdc++-v3/testsuite/18_support/exception/38732.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/libsupc++/eh_alloc.cc
    trunk/libstdc++-v3/libsupc++/eh_ptr.cc
    trunk/libstdc++-v3/libsupc++/eh_throw.cc
    trunk/libstdc++-v3/libsupc++/unwind-cxx.h

Comment 19 Jakub Jelinek 2009-01-07 22:54:53 UTC
Fixed.
Comment 20 Stephan Bergmann 2009-01-08 08:31:37 UTC
re <#c17>:

"prepended member can change the negative offset of the other struct members": see "the four bytes of referenceCount cancel out a previous four padding bytes just before the unwindHeader member coming later in the struct" (<#c12>)

"if they ever use std::current_exception(), they *will* have to change the code to cope with dependent exceptions": can you please elaborate (by direct mail, if considered more appropriate)