diff mbox

libstdc++/66017 Avoid bad casts and fix alignment of _Rb_tree_node<long long>::_M_storage

Message ID 20150526144635.GF2985@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely May 26, 2015, 2:46 p.m. UTC 
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<long long>
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<long long> shared state: alignment: 8 size: 24
  shared_ptr<long long> control block: alignment: 8 size: 24
  forward_list<long long> node: alignment: 8 size: 16
  unordered_set<long long> node: alignment: 8 size: 16

After:

  future<long long> shared state: alignment: 4 size: 20
  shared_ptr<long long> control block: alignment: 4 size: 20
  forward_list<long long> node: alignment: 4 size: 12
  unordered_set<long long> node: alignment: 4 size: 12

The fix for _Rb_tree_node<long long> 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 <iostream>
#include <future>
#include <memory>
#include <forward_list>
#include <unordered_set>

using namespace std;

template<typename T>
void f(const char* s)
{
  cout << s << ": alignment: " << alignof(T) << " size: " << sizeof(T) << '\n';
}

int main()
{

  f<__future_base::_Result<long long>>("future<long long> shared state");

  f<_Sp_counted_ptr_inplace<long long, allocator<long long>, __default_lock_policy>>("shared_ptr<long long> control block");

  f<_Fwd_list_node<long long>>("forward_list<long long> node");

  f<__detail::_Hash_node<long long, false>>("unordered_set<long long> node");
}
diff mbox

Patch

commit 9a896b5d29b40186424b0a7b2b1717db483dc8e6
Author: Jonathan Wakely <jwakely@redhat.com>
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<typename iterator::_Link_type>
-			(const_cast<typename iterator::_Base_ptr>(_M_node))); }
+      { return iterator(const_cast<typename iterator::_Base_ptr>(_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<const _Tp*>(_M_addr()); }
     };
 
+  template<typename _Tp>
+    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<void*>(&_M_storage); }
+
+      const void*
+      _M_addr() const noexcept
+      { return static_cast<const void*>(&_M_storage); }
+
+      _Tp*
+      _M_ptr() noexcept
+      { return static_cast<_Tp*>(_M_addr()); }
+
+      const _Tp*
+      _M_ptr() const noexcept
+      { return static_cast<const _Tp*>(_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: