From patchwork Tue Mar 25 17:25:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 333622 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 500741400AC for ; Wed, 26 Mar 2014 04:26:10 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=a9cIJKQkvdbwLVXLq jf8TPedPoHgjO6OliMcsOR/ni8+Z+JSoFc1T/836TNVo1dAobKVFsUlZmfqKoJ2p dabcxolmd85JyB+6emDONsOvbYHjlwAY86ALyGeGb47/DNlvv7d04gavDo9nZ778 9gj3/SEHQccuZBbHbOLEEUVzeY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=WMfDwgWX6sFFQv7YjVKhVwl 49Ng=; b=SAdU0sBBHfSjkgYgSI2ECj8LVg3612gavEzVhHqlhyR6UV89rGidwL2 o/Kla6qYJwMzPR36asXbmwKWHAkbHnUZbvnPGp5WGkTOA5AIPx7Nw1U0g4EfJwAW JQhetqk20Q5WRt+DMhTtWLJk07Gu2O+py9OZfN9j1kU5bBl3kk3Y= Received: (qmail 15544 invoked by alias); 25 Mar 2014 17:26:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 15525 invoked by uid 89); 25 Mar 2014 17:26:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Mar 2014 17:25:57 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2PHPrxx008414 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 25 Mar 2014 13:25:54 -0400 Received: from localhost (vpn1-6-230.ams2.redhat.com [10.36.6.230]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s2PHPqJ4017823; Tue, 25 Mar 2014 13:25:52 -0400 Date: Tue, 25 Mar 2014 17:25:52 +0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Cc: sebastian.redl@getdesigned.at Subject: Re: std::rethrow_exception is broken Message-ID: <20140325172551.GD8266@redhat.com> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 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. 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 + constexpr std::size_t unwindhdr() + { return offsetof(Ex, unwindHeader); } + +template + constexpr std::size_t termHandler() + { return unwindhdr() - offsetof(Ex, terminateHandler); } + +static_assert( termHandler<__cxa_exception>() + == termHandler<__cxa_dependent_exception>(), + "__cxa_dependent_exception::termHandler layout is correct" ); + +#ifndef __ARM_EABI_UNWINDER__ +template + constexpr std::ptrdiff_t adjptr() + { return unwindhdr() - 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 +// . + +// PR libstdc++/60612 + +#include +#include + +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 +// . + +// PR libstdc++/60612 + +#include +#include + +void unexpected() { _Exit(0); } + +void f() throw() +{ + try { + throw 1; + } catch (...) { + std::set_unexpected(unexpected); + std::rethrow_exception(std::current_exception()); + } +} + +int main() +{ + f(); +}