diff mbox

std::rethrow_exception is broken

Message ID 20140325172551.GD8266@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely March 25, 2014, 5:25 p.m. UTC 
On 24/03/14 19:19 +0000, Jonathan Wakely wrote:
>There is a lot of code in libsupc++/eh_* that relies on
>__cxa_exception and __cxa_dependent_exception having similar layouts,
>so tricks like this work:
>
>static inline void*
>__gxx_caught_object(_Unwind_Exception* eo)
>{
>  // Bad as it looks, this actually works for dependent exceptions too.
>  __cxa_exception* header = __get_exception_header_from_ue (eo);
>  return header->adjustedPtr;
>}
>
>The idea is that although the eo might be a pointer to the
>unwindHeader member  of either __cxa_exception or
>__cxa_dependent_exception, the adjustedPtr member will be always be at
>the same location relative to the unwindHeader member, so it Just
>Works.
>
>Except it doesn't.
>
>offsetof(__cxa_exception, unwindHeader) == 80
>offsetof(__cxa_dependent_exception, unwindHeader) == 80
>offsetof(__cxa_exception, adjustedPtr) == 72
>offsetof(__cxa_dependent_exception, adjustedPtr) == 64

Here's the output of GDB's pahole on __cxa_exception

(gdb) pahole struct __cxxabiv1::__cxa_exception
                struct __cxxabiv1::__cxa_exception {
  /*   0   8 */    std::type_info * exceptionType
  /*   8   8 */    void (*)(void *) exceptionDestructor
  /*  16   8 */    void (*)(void) unexpectedHandler
  /*  24   8 */    void (*)(void) terminateHandler
  /*  32   8 */    __cxxabiv1::__cxa_exception * nextException
  /*  40   4 */    int handlerCount
  /*  44   4 */    int handlerSwitchValue
  /*  48   8 */    const unsigned char * actionRecord
  /*  56   8 */    const unsigned char * languageSpecificData
  /*  64   8 */    unsigned long catchTemp
  /*  72   8 */    void * adjustedPtr
  /*  80  32 */   struct _Unwind_Exception {
  /*   0   8 */      unsigned long exception_class
  /*   8   8 */      void (*)(_Unwind_Reason_Code, _Unwind_Exception *) exception_cleanup
  /*  16   8 */      unsigned long private_1
  /*  24   8 */      unsigned long private_2
                  } unwindHeader
                } 
  
And here's the current definition of __cxa_dependent_exception:

(gdb) pahole struct __cxxabiv1::__cxa_dependent_exception 
                struct __cxxabiv1::__cxa_dependent_exception {
  /*   0   8 */    void * primaryException
  /*   8   8 */    void (*)(void) unexpectedHandler
  /*  16   8 */    void (*)(void) terminateHandler
  /*  24   8 */    __cxxabiv1::__cxa_exception * nextException
  /*  32   4 */    int handlerCount
  /*  36   4 */    int handlerSwitchValue
  /*  40   8 */    const unsigned char * actionRecord
  /*  48   8 */    const unsigned char * languageSpecificData
  /*  56   8 */    unsigned long catchTemp
  /*  64   8 */    void * adjustedPtr
   /* XXX 64 bit hole, try to pack */
  /*  80  32 */   struct _Unwind_Exception {
  /*   0   8 */      unsigned long exception_class
  /*   8   8 */      void (*)(_Unwind_Reason_Code, _Unwind_Exception *) exception_cleanup
  /*  16   8 */      unsigned long private_1
  /*  24   8 */      unsigned long private_2
                  } unwindHeader
                } 

Notice the hole right before the unwindHeader member which is not
there in __cxa_exception. All the member which are supposed to be at
the same offsets as in __cxa_exception are 8 bytes earlier.

The attached patch adds a __padding member near the start so changes
that to:

(gdb) pahole struct __cxxabiv1::__cxa_dependent_exception 
                struct __cxxabiv1::__cxa_dependent_exception {
  /*   0   8 */    void * primaryException
  /*   8   8 */    void (*)(void *) __padding
  /*  16   8 */    void (*)(void) unexpectedHandler
  /*  24   8 */    void (*)(void) terminateHandler
  /*  32   8 */    __cxxabiv1::__cxa_exception * nextException
  /*  40   4 */    int handlerCount
  /*  44   4 */    int handlerSwitchValue
  /*  48   8 */    const unsigned char * actionRecord
  /*  56   8 */    const unsigned char * languageSpecificData
  /*  64   8 */    unsigned long catchTemp
  /*  72   8 */    void * adjustedPtr
  /*  80  32 */   struct _Unwind_Exception {
  /*   0   8 */      unsigned long exception_class
  /*   8   8 */      void (*)(_Unwind_Reason_Code, _Unwind_Exception *) exception_cleanup
  /*  16   8 */      unsigned long private_1
  /*  24   8 */      unsigned long private_2
                  } unwindHeader
                } 

That change allows all the pointer tricks in libsupc++/eh_*.cc to
continue working.  It's a change to the layout of a low-level type,
which is not visible to users linking to libsupc++.so, but anyone
linking statically will not be able to mix code using GCC 4.9's
libsupc++.a with the libsupc++.a from previous versions if they use
std::rethrow_exception() anywhere.

Tested x86_64-linux, I plan to commit this to trunk soon.
commit 06a845f80204947afd6866109db58cc85dc87117
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Mar 25 14:42:45 2014 +0000

    	PR libstdc++/60612
    	* libsupc++/eh_ptr.cc: Assert __cxa_dependent_exception layout is
    	compatible with __cxa_exception.
    	* libsupc++/unwind-cxx.h (__cxa_dependent_exception): Add padding.
    	Fix typos in comments.
    	* testsuite/18_support/exception_ptr/60612-terminate.cc: New.
    	* testsuite/18_support/exception_ptr/60612-unexpected.cc: New.

Comments

Jonathan Wakely March 27, 2014, 6:10 p.m. UTC | #1 
On 25/03/14 17:25 +0000, Jonathan Wakely wrote:
>Tested x86_64-linux, I plan to commit this to trunk soon.
>

>commit 06a845f80204947afd6866109db58cc85dc87117
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Tue Mar 25 14:42:45 2014 +0000
>
>    	PR libstdc++/60612
>    	* libsupc++/eh_ptr.cc: Assert __cxa_dependent_exception layout is
>    	compatible with __cxa_exception.
>    	* libsupc++/unwind-cxx.h (__cxa_dependent_exception): Add padding.
>    	Fix typos in comments.
>    	* testsuite/18_support/exception_ptr/60612-terminate.cc: New.
>    	* testsuite/18_support/exception_ptr/60612-unexpected.cc: New.

Committed to trunk.
diff mbox

Patch

diff --git a/libstdc++-v3/libsupc++/eh_ptr.cc b/libstdc++-v3/libsupc++/eh_ptr.cc
index 6508749..8c25a81 100644
--- a/libstdc++-v3/libsupc++/eh_ptr.cc
+++ b/libstdc++-v3/libsupc++/eh_ptr.cc
@@ -35,6 +35,32 @@ 
 
 using namespace __cxxabiv1;
 
+// Verify assumptions about member layout in exception types
+namespace
+{
+template<typename Ex>
+  constexpr std::size_t unwindhdr()
+  { return offsetof(Ex, unwindHeader); }
+
+template<typename Ex>
+  constexpr std::size_t termHandler()
+  { return unwindhdr<Ex>() - offsetof(Ex, terminateHandler); }
+
+static_assert( termHandler<__cxa_exception>()
+	       == termHandler<__cxa_dependent_exception>(),
+	       "__cxa_dependent_exception::termHandler layout is correct" );
+
+#ifndef __ARM_EABI_UNWINDER__
+template<typename Ex>
+  constexpr std::ptrdiff_t adjptr()
+  { return unwindhdr<Ex>() - offsetof(Ex, adjustedPtr); }
+
+static_assert( adjptr<__cxa_exception>()
+	       == adjptr<__cxa_dependent_exception>(),
+	       "__cxa_dependent_exception::adjustedPtr layout is correct" );
+#endif
+}
+
 std::__exception_ptr::exception_ptr::exception_ptr() _GLIBCXX_USE_NOEXCEPT
 : _M_exception_object(0) { }
 
diff --git a/libstdc++-v3/libsupc++/unwind-cxx.h b/libstdc++-v3/libsupc++/unwind-cxx.h
index a7df603..f57c536 100644
--- a/libstdc++-v3/libsupc++/unwind-cxx.h
+++ b/libstdc++-v3/libsupc++/unwind-cxx.h
@@ -81,7 +81,7 @@  struct __cxa_exception
   // Stack of exceptions in cleanups.
   __cxa_exception* nextPropagatingException;
 
-  // The nuber of active cleanup handlers for this exception.
+  // The number of active cleanup handlers for this exception.
   int propagationCount;
 #else
   // Cache parsed handler data from the personality routine Phase 1
@@ -114,6 +114,11 @@  struct __cxa_dependent_exception
   // The primary exception this thing depends on.
   void *primaryException;
 
+  // Unused member to get similar layout to __cxa_exception, otherwise the
+  // alignment requirements of _Unwind_Exception would require padding bytes
+  // before the unwindHeader member.
+  void (_GLIBCXX_CDTOR_CALLABI *__padding)(void *);
+
   // The C++ standard has entertaining rules wrt calling set_terminate
   // and set_unexpected in the middle of the exception cleanup process.
   std::unexpected_handler unexpectedHandler;
@@ -130,7 +135,7 @@  struct __cxa_dependent_exception
   // Stack of exceptions in cleanups.
   __cxa_exception* nextPropagatingException;
 
-  // The nuber of active cleanup handlers for this exception.
+  // The number of active cleanup handlers for this exception.
   int propagationCount;
 #else
   // Cache parsed handler data from the personality routine Phase 1
diff --git a/libstdc++-v3/testsuite/18_support/exception_ptr/60612-terminate.cc b/libstdc++-v3/testsuite/18_support/exception_ptr/60612-terminate.cc
new file mode 100644
index 0000000..ec5940d
--- /dev/null
+++ b/libstdc++-v3/testsuite/18_support/exception_ptr/60612-terminate.cc
@@ -0,0 +1,41 @@ 
+// { dg-options "-std=gnu++11" }
+// { dg-require-atomic-builtins "" }
+
+// Copyright (C) 2014 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// PR libstdc++/60612
+
+#include <exception>
+#include <stdlib.h>
+
+void terminate() { _Exit(0); }
+
+void f() noexcept
+{
+  try {
+    throw 1;
+  } catch (...) {
+    std::set_terminate(terminate);
+    std::rethrow_exception(std::current_exception());
+  }
+}
+
+int main()
+{
+  f();
+}
diff --git a/libstdc++-v3/testsuite/18_support/exception_ptr/60612-unexpected.cc b/libstdc++-v3/testsuite/18_support/exception_ptr/60612-unexpected.cc
new file mode 100644
index 0000000..3f7e2cf
--- /dev/null
+++ b/libstdc++-v3/testsuite/18_support/exception_ptr/60612-unexpected.cc
@@ -0,0 +1,41 @@ 
+// { dg-options "-std=gnu++11" }
+// { dg-require-atomic-builtins "" }
+
+// Copyright (C) 2014 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// PR libstdc++/60612
+
+#include <exception>
+#include <stdlib.h>
+
+void unexpected() { _Exit(0); }
+
+void f() throw()
+{
+  try {
+    throw 1;
+  } catch (...) {
+    std::set_unexpected(unexpected);
+    std::rethrow_exception(std::current_exception());
+  }
+}
+
+int main()
+{
+  f();
+}