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.

*** Bug 38631 has been marked as a duplicate of this bug. ***

Confirmed.

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.

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.

>Also, the alignment of _Unwind_Exception depends on -mavx.

That is a target issue and should be filed as separately.

(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.

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)

(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.

(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.

(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.

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

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.

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.

Working on it.

Created attachment 17047 [details]
gcc44-pr38732.patch

Patch I'm going to bootstrap/regtest.

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.

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

Fixed.

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)

(In reply to Andrew Pinski from comment #1)
> 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.

For reference this is commit:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=30a333ceeb9141e67663c6151983a12678dcc329