From patchwork Tue May 26 14:46:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 476496 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 D84A2140081 for ; Wed, 27 May 2015 00:47:02 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=VUoSqrG4; dkim-atps=neutral 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=Xm+r3apNj49yvV+Z7 uZ4mUMm73t9dpAT8GKx5vNH+Vdqk2Wuhoms4WU7Fa99L731ixXQPYwBrJbDSq6ng JmPlcZ8+8DZMo4zD3vnjIfnGF0DzzZ3KL+hAQrVy3iapiv5JmfTwyxiYp3rBsWGo wwouAkaTgU5+UZReL1xcb4d6oA= 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=yCtY4BYMzabJEIM320upUcP Zdkc=; b=VUoSqrG4HQlg9gydbug82/h6do7ppT90leYTT2gfJ3ZG0//8nQJ1UAc x76QFCgGjoCokdpUu49GPSwZNqalogiZdaSJNSHUBAdruZ/eWJCCmk3xti1bAPvo G9WBRhvdmLp1FcQ8klKGYQMcA3hPav23Kf8mEtEYg1Q+Qj2mDIvE= Received: (qmail 80297 invoked by alias); 26 May 2015 14:46:44 -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 80273 invoked by uid 89); 26 May 2015 14:46:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.1 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 26 May 2015 14:46:41 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4QEkbZ4018550 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 26 May 2015 10:46:38 -0400 Received: from localhost (ovpn-116-76.ams2.redhat.com [10.36.116.76]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4QEkavD020756; Tue, 26 May 2015 10:46:37 -0400 Date: Tue, 26 May 2015 15:46:35 +0100 From: Jonathan Wakely To: Jakub Jelinek Cc: Jason Merrill , libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [patch] libstdc++/66017 Avoid bad casts and fix alignment of _Rb_tree_node::_M_storage Message-ID: <20150526144635.GF2985@redhat.com> References: <20150522141510.GN30202@redhat.com> <20150522142944.GM10247@tucnak.redhat.com> <20150522145947.GO30202@redhat.com> <20150522151342.GN10247@tucnak.redhat.com> <20150522152149.GQ30202@redhat.com> <20150522174808.GS30202@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150522174808.GS30202@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) On 22/05/15 18:48 +0100, Jonathan Wakely wrote: >On 22/05/15 16:21 +0100, Jonathan Wakely wrote: >>On 22/05/15 17:13 +0200, Jakub Jelinek wrote: >>>On Fri, May 22, 2015 at 03:59:47PM +0100, Jonathan Wakely wrote: >>>>>>+ alignas(alignof(_Tp2)) unsigned char _M_storage[sizeof(_Tp)]; >>>>> >>>>>Is alignof(_Tp2) always the same as alignof(_Tp2::_M_t) on all targets >>>>>(I mean, won't some target align the structure more than its only field)? >>>> >>>>Hmm, maybe. I don't know. >>>> >>>>>Wouldn't it be safer to use alignof(_Tp2::_M_t) here? >>>> >>>>Yes. >>>> >>>>>Though, apparently that is a GNU extension, so you'd need to use __alignof__ >>>>>instead. >>>> >>>>Yes, that's what I did in an earlier version of the patch, so I'll go >>>>back to that. >>> >>>Just grepped around, and e.g. on powerpc64le-linux -std=c++11 -malign-power -O2 >>>typedef double _Tp; >>>struct _Tp2 { _Tp _M_t; }; >>>extern _Tp2 tp2e; >>>int a = alignof(_Tp2); >>>int b = __alignof__(_Tp2::_M_t); >>>int c = alignof(_Tp); >>>int d = __alignof__(tp2e._M_t); >>>int e = alignof(_Tp2::_M_t); >>> >>>we have a = 8, b = 4, c = 8, d = 4, e = 4. >> >>OK, thanks. >> >>>Note clang++ with -pedantic-errors errors out on alignof(_Tp2::_M_t) though. >> >>It allows __alignof__ though. > >Revised patches attached, as two separate commits because the first >should be backported but the second doesn't need to be. > >This includes the necessary changes for the Python printers. The change to __aligned_buffer (which makes _Rb_tree_node consistent in c++98 and c++11 modes) also affects some other C++11-only types. Compiling the attached program with -std=gnu++11 -m32 before and after the patch produces these results: Before: future shared state: alignment: 8 size: 24 shared_ptr control block: alignment: 8 size: 24 forward_list node: alignment: 8 size: 16 unordered_set node: alignment: 8 size: 16 After: future shared state: alignment: 4 size: 20 shared_ptr control block: alignment: 4 size: 20 forward_list node: alignment: 4 size: 12 unordered_set node: alignment: 4 size: 12 The fix for _Rb_tree_node is a bug fix and necessary for consistency with existing c++98 code, which is more important than consistency with existing c++11 code using 5.1 or earlier releases. But changing the other types as well would make 5.2 inconsistent with 5.1 for those types. We could just make that change and deal with it, or I could keep __aligned_buffer unchanged and add a new __aligned_buffer_mem for use in _Rb_tree_node, so we only change the one type that is currently inconsistent between c++98 and c++11 modes. The attached patch makes that smaller change (the second patch in my last mail remains unchanged). It's a shame to waste some space in the other types using __aligned_buffer, and to have to maintain both __aligned_buffer and __aligned_buffer_mem, but I think this is safer. #include #include #include #include #include using namespace std; template void f(const char* s) { cout << s << ": alignment: " << alignof(T) << " size: " << sizeof(T) << '\n'; } int main() { f<__future_base::_Result>("future shared state"); f<_Sp_counted_ptr_inplace, __default_lock_policy>>("shared_ptr control block"); f<_Fwd_list_node>("forward_list node"); f<__detail::_Hash_node>("unordered_set node"); } commit 9a896b5d29b40186424b0a7b2b1717db483dc8e6 Author: Jonathan Wakely Date: Thu May 21 14:41:16 2015 +0100 PR libstdc++/66017 * include/bits/stl_tree.h (_Rb_tree_node): Use __aligned_buffer_mem. (_Rb_tree_iterator, _Rb_tree_const_iterator): Support construction from _Base_ptr. (_Rb_tree_const_iterator::_M_const_cast): Remove static_cast. (_Rb_tree::begin, _Rb_tree::end): Remove static_cast. * include/ext/aligned_buffer.h (__aligned_buffer_mem): New type using alignment of _Tp as a member subobject, not as a complete object. * python/libstdcxx/v6/printers.py (StdRbtreeIteratorPrinter): Lookup _Link_type manually as it might not be in the debug info. diff --git a/libstdc++-v3/include/bits/stl_tree.h b/libstdc++-v3/include/bits/stl_tree.h index 5ca8e28..ab0f754 100644 --- a/libstdc++-v3/include/bits/stl_tree.h +++ b/libstdc++-v3/include/bits/stl_tree.h @@ -146,7 +146,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_valptr() const { return std::__addressof(_M_value_field); } #else - __gnu_cxx::__aligned_buffer<_Val> _M_storage; + __gnu_cxx::__aligned_buffer_mem<_Val> _M_storage; _Val* _M_valptr() @@ -188,7 +188,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION : _M_node() { } explicit - _Rb_tree_iterator(_Link_type __x) _GLIBCXX_NOEXCEPT + _Rb_tree_iterator(_Base_ptr __x) _GLIBCXX_NOEXCEPT : _M_node(__x) { } reference @@ -260,7 +260,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION : _M_node() { } explicit - _Rb_tree_const_iterator(_Link_type __x) _GLIBCXX_NOEXCEPT + _Rb_tree_const_iterator(_Base_ptr __x) _GLIBCXX_NOEXCEPT : _M_node(__x) { } _Rb_tree_const_iterator(const iterator& __it) _GLIBCXX_NOEXCEPT @@ -268,8 +268,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION iterator _M_const_cast() const _GLIBCXX_NOEXCEPT - { return iterator(static_cast - (const_cast(_M_node))); } + { return iterator(const_cast(_M_node)); } reference operator*() const _GLIBCXX_NOEXCEPT @@ -868,28 +867,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION iterator begin() _GLIBCXX_NOEXCEPT - { - return iterator(static_cast<_Link_type> - (this->_M_impl._M_header._M_left)); - } + { return iterator(this->_M_impl._M_header._M_left); } const_iterator begin() const _GLIBCXX_NOEXCEPT - { - return const_iterator(static_cast<_Const_Link_type> - (this->_M_impl._M_header._M_left)); - } + { return const_iterator(this->_M_impl._M_header._M_left); } iterator end() _GLIBCXX_NOEXCEPT - { return iterator(static_cast<_Link_type>(&this->_M_impl._M_header)); } + { return iterator(&this->_M_impl._M_header); } const_iterator end() const _GLIBCXX_NOEXCEPT - { - return const_iterator(static_cast<_Const_Link_type> - (&this->_M_impl._M_header)); - } + { return const_iterator(&this->_M_impl._M_header); } reverse_iterator rbegin() _GLIBCXX_NOEXCEPT diff --git a/libstdc++-v3/include/ext/aligned_buffer.h b/libstdc++-v3/include/ext/aligned_buffer.h index 01f5729..4db5099 100644 --- a/libstdc++-v3/include/ext/aligned_buffer.h +++ b/libstdc++-v3/include/ext/aligned_buffer.h @@ -73,6 +73,38 @@ namespace __gnu_cxx { return static_cast(_M_addr()); } }; + template + struct __aligned_buffer_mem + { + // Target macro ADJUST_FIELD_ALIGN can produce different alignment for + // types when used as class members. __aligned_buffer_mem is intended + // for use as a class member, so align the buffer as for a class member. + struct _Tp2 { _Tp _M_t; }; + + alignas(__alignof__(_Tp2::_M_t)) unsigned char _M_storage[sizeof(_Tp)]; + + __aligned_buffer_mem() = default; + + // Can be used to avoid value-initialization + __aligned_buffer_mem(std::nullptr_t) { } + + void* + _M_addr() noexcept + { return static_cast(&_M_storage); } + + const void* + _M_addr() const noexcept + { return static_cast(&_M_storage); } + + _Tp* + _M_ptr() noexcept + { return static_cast<_Tp*>(_M_addr()); } + + const _Tp* + _M_ptr() const noexcept + { return static_cast(_M_addr()); } + }; + } // namespace #endif /* _ALIGNED_BUFFER_H */ diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py index c6f96d7..2b6e409 100644 --- a/libstdc++-v3/python/libstdcxx/v6/printers.py +++ b/libstdc++-v3/python/libstdcxx/v6/printers.py @@ -456,11 +456,12 @@ class StdRbtreeIteratorPrinter: def __init__ (self, typename, val): self.val = val + valtype = self.val.type.template_argument(0).strip_typedefs() + nodetype = gdb.lookup_type('std::_Rb_tree_node<' + str(valtype) + '>') + self.link_type = nodetype.strip_typedefs().pointer() def to_string (self): - typename = str(self.val.type.strip_typedefs()) + '::_Link_type' - nodetype = gdb.lookup_type(typename).strip_typedefs() - node = self.val.cast(nodetype).dereference() + node = self.val['_M_node'].cast(self.link_type).dereference() return get_value_from_Rb_tree_node(node) class StdDebugIteratorPrinter: