This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Use abi_tag attribute on std::list

On 03/10/14 15:49 +0100, Jonathan Wakely wrote:

On 03/10/14 16:25 +0200, Marc Glisse wrote:

On Fri, 3 Oct 2014, Jonathan Wakely wrote:


This is the patch I intend to commit to make std::list::size() O(1) as
required by C++11.

This is an ABI change, so std::list will get tagged with
abi_tag("cxx11") so that it mangles differently.
Assuming a future where we have both _GLIBCXX_ABI_TAG_CXX11 and _GLIBCXX_ABI_TAG_CXX17, I don't really see how _GLIBCXX_DEFAULT_ABI_TAG is supposed to work. We don't want to define _GLIBCXX_DEFAULT_ABI_TAG to _GLIBCXX_ABI_TAG_CXX17 and suddenly have std::list change mangling. 

True.
Should it be called _GLIBCXX_DEFAULT_ABI_TAG_CXX11, meaning _GLIBCXX_ABI_TAG_CXX11_IF_ENABLED_AND_NOTHING_OTHERWISE? 

I suppose so ... or _GLIBCXX_MAYBE_ABI_TAG_CXX11 ?
Defining a dummy _M_distance in the old abi case is a bit strange (we could protect the single use with #if _GLIBCXX_USE_CXX11_ABI), but why not... 

Yeah, I was trying to minimise the preprocessor conditionals, but
maybe that's one step too far.
Do you mind if I move (in a future patch once yours is committed) _M_size into _M_impl::_M_node as suggested in PR 61347? 

Gah, that's where I had it until earlier this week, and I looked at it
and wondered why it was in the _List_impl class (because you only need
one member in there to benefit from the empty base-class
optimisation).

I will move it back there, since I already have that code on another
branch, so there's no point making you change the code to match
something I've already got!

Thanks for your useful comments (as always).


I've committed the attached patch, which I think is the same as the
one I posted a week ago.

Marc, I agree with your comments, but chose to ignore them ;-) For
now, anyway. I want to get something committed, we can improve it in
stage 3, or when we need to worry about a C++17 ABI change.

Tested x86_64-linux, committed to trunk.

Documentation for the abi_tag("cxx11") changes will follow ASAP, but
probably during stage 3, once all the changes are committed.

commit 3ac61312eb285d125da8af4c6cdaad1b1ce7d235
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Aug 7 01:13:17 2014 +0100

    	PR libstdc++/49561
    	* acinclude.m4 (GLIBCXX_ENABLE_LIBSTDCXX_CXX11_ABI): Define.
    	* configure.ac: Use GLIBCXX_ENABLE_LIBSTDCXX_CXX11_ABI.
    	* configure: Regenerate.
    	* include/Makefile.am (stamp-cxx11-abi): New target.
    	(c++config.h): Set _GLIBCXX_USE_CXX11_ABI macro.
    	* include/Makefile.in: Regenerate.
    	* include/bits/c++config: Add _GLIBCXX_USE_CXX11_ABI placeholder and
    	define _GLIBCXX_DEFAULT_ABI_TAG.
    	* include/bits/list.tcc (list::emplace(const_iterator, _Args&...)):
    	Increment size.
    	(list::emplace(const_iterator, const value_type&)): Likewise.
    	(list::merge(list&), list::merge(list&, _StrictWeakOrdering)): Adjust
    	list sizes.
    	* include/bits/stl_list.h (_List_base, list): Add ABI tag macro.
    	(_List_base::_M_size): New data member in cxx11 ABI mode.
    	(_List_base::_S_distance(_List_node_base*, _List_node_base*)): New
    	function.
    	(_List_base::_M_get_size(), _List_base::_M_set_size(size_t),
    	_List_base::_M_inc_size(size_t), _List_base::_M_dec_size(size_t),
    	_List_base::_M_distance, _List_base::_M_node_count): New functions for
    	accessing list size correctly for the ABI mode.
    	(_List_base::_List_base(_List_base&&)): Copy size and reset source.
    	(_List_base::_M_init()): Initialize size member.
    	(list::size()): Use _List_base::_M_node_count.
    	(list::swap(list&)): Swap sizes.
    	(list::splice(iterator, list&)): Update sizes.
    	(list::splice(iterator, list&, iterator)): Likewise.
    	(list::insert(iterator, const value_type&)): Update size.
    	(list::insert(iterator, _Args&&...)): Likewise.
    	(list::_M_erase(iterator)): Likewise.
    	* testsuite/23_containers/list/requirements/dr438/assign_neg.cc:
    	Adjust.
    	* testsuite/23_containers/list/requirements/dr438/constructor_1_neg.cc:
    	Adjust.
    	* testsuite/23_containers/list/requirements/dr438/constructor_2_neg.cc:
    	Adjust.
    	* testsuite/23_containers/list/requirements/dr438/insert_neg.cc:
    	Adjust.
    	* testsuite/ext/profile/mutex_extensions_neg.cc: Adjust.

diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 2650a5a..0229609 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -3831,6 +3831,25 @@ AC_DEFUN([GLIBCXX_CHECK_SDT_H], [
   AC_MSG_RESULT($glibcxx_cv_sys_sdt_h)
 ])
 
+dnl
+dnl Check if the user wants the new C++11-conforming ABI.
+dnl
+dnl --disable-libstdcxx-cxx11-abi will use old ABI for all types.
+dnl
+dnl Defines:
+dnl  _GLIBCXX_USE_ABI_TAG (always defined, either to 1 or 0)
+dnl
+AC_DEFUN([GLIBCXX_ENABLE_LIBSTDCXX_CXX11_ABI], [
+  AC_ARG_ENABLE([libstdcxx-cxx11-abi],
+    AC_HELP_STRING([--disable-libstdcxx-cxx11-abi],
+		   [disable the C++11-conforming ABI]),,
+		   [enable_libstdcxx_cxx11_abi=yes])
+  if test x"$enable_libstdcxx_cxx11_abi" != xyes; then
+    AC_MSG_NOTICE([C++11-conforming ABI is disabled])
+  fi
+  GLIBCXX_CONDITIONAL(ENABLE_CXX11_ABI, test $enable_libstdcxx_cxx11_abi = yes)
+])
+
 # Macros from the top-level gcc directory.
 m4_include([../config/gc++filt.m4])
 m4_include([../config/tls.m4])
diff --git a/libstdc++-v3/configure.ac b/libstdc++-v3/configure.ac
index 1de0f6c..eb826e4 100644
--- a/libstdc++-v3/configure.ac
+++ b/libstdc++-v3/configure.ac
@@ -365,6 +365,8 @@ AC_SUBST(libtool_VERSION)
 
 GLIBCXX_ENABLE_LIBSTDCXX_VISIBILITY([yes])
 
+GLIBCXX_ENABLE_LIBSTDCXX_CXX11_ABI([yes])
+
 ac_ldbl_compat=no
 case "$target" in
   powerpc*-*-linux* | \
diff --git a/libstdc++-v3/include/Makefile.am b/libstdc++-v3/include/Makefile.am
index dee4a3f..e3aed84 100644
--- a/libstdc++-v3/include/Makefile.am
+++ b/libstdc++-v3/include/Makefile.am
@@ -1122,6 +1122,14 @@ stamp-visibility:
 	echo 0 > stamp-visibility
 endif
 
+if ENABLE_CXX11_ABI
+stamp-cxx11-abi:
+	echo 1 > stamp-cxx11-abi
+else
+stamp-cxx11-abi:
+	echo 0 > stamp-cxx11-abi
+endif
+
 # NB: The non-empty default ldbl_compat works around an AIX sed
 # oddity, see libstdc++/31957 for details.
 ${host_builddir}/c++config.h: ${CONFIG_HEADER} \
@@ -1130,11 +1138,13 @@ ${host_builddir}/c++config.h: ${CONFIG_HEADER} \
 			      ${toplevel_srcdir}/gcc/DATESTAMP \
 			      stamp-namespace-version \
 			      stamp-visibility \
-			      stamp-extern-template
+			      stamp-extern-template \
+			      stamp-cxx11-abi
 	@date=`cat ${toplevel_srcdir}/gcc/DATESTAMP` ;\
 	ns_version=`cat stamp-namespace-version` ;\
 	visibility=`cat stamp-visibility` ;\
 	externtemplate=`cat stamp-extern-template` ;\
+	cxx11abi=`cat stamp-cxx11-abi` ;\
 	ldbl_compat='s,g,g,' ;\
 	grep "^[	 ]*#[	 ]*define[	 ][	 ]*_GLIBCXX_LONG_DOUBLE_COMPAT[	 ][	 ]*1[	 ]*$$" \
 	${CONFIG_HEADER} > /dev/null 2>&1 \
@@ -1143,6 +1153,7 @@ ${host_builddir}/c++config.h: ${CONFIG_HEADER} \
 	-e "s,define _GLIBCXX_INLINE_VERSION, define _GLIBCXX_INLINE_VERSION $$ns_version," \
 	-e "s,define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY, define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY $$visibility," \
 	-e "s,define _GLIBCXX_EXTERN_TEMPLATE$$, define _GLIBCXX_EXTERN_TEMPLATE $$externtemplate," \
+	-e "s,define _GLIBCXX_USE_CXX11_ABI, define _GLIBCXX_USE_CXX11_ABI $$cxx11abi," \
 	-e "$$ldbl_compat" \
 	    < ${glibcxx_srcdir}/include/bits/c++config > $@ ;\
 	sed -e 's/HAVE_/_GLIBCXX_HAVE_/g' \
diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
index ff6afc8..bb58a9b 100644
--- a/libstdc++-v3/include/bits/c++config
+++ b/libstdc++-v3/include/bits/c++config
@@ -193,6 +193,17 @@ namespace std
 #endif
 }
 
+// Use abi_tag("cxx11")
+#ifndef _GLIBCXX_USE_CXX11_ABI
+#define _GLIBCXX_USE_CXX11_ABI
+#endif
+
+#if _GLIBCXX_USE_CXX11_ABI
+# define _GLIBCXX_DEFAULT_ABI_TAG _GLIBCXX_ABI_TAG_CXX11
+#else
+# define _GLIBCXX_DEFAULT_ABI_TAG
+#endif
+
 
 // Defined if inline namespaces are used for versioning.
 #define _GLIBCXX_INLINE_VERSION 
diff --git a/libstdc++-v3/include/bits/list.tcc b/libstdc++-v3/include/bits/list.tcc
index 42855a4..f99ec54 100644
--- a/libstdc++-v3/include/bits/list.tcc
+++ b/libstdc++-v3/include/bits/list.tcc
@@ -89,6 +89,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       {
 	_Node* __tmp = _M_create_node(std::forward<_Args>(__args)...);
 	__tmp->_M_hook(__position._M_const_cast()._M_node);
+	this->_M_inc_size(1);
 	return iterator(__tmp);
       }
 #endif
@@ -104,6 +105,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
     {
       _Node* __tmp = _M_create_node(__x);
       __tmp->_M_hook(__position._M_const_cast()._M_node);
+      this->_M_inc_size(1);
       return iterator(__tmp);
     }
 
@@ -353,6 +355,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	      ++__first1;
 	  if (__first2 != __last2)
 	    _M_transfer(__last1, __first2, __last2);
+
+	  this->_M_inc_size(__x._M_get_size());
+	  __x._M_set_size(0);
 	}
     }
 
@@ -387,6 +392,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 		++__first1;
 	    if (__first2 != __last2)
 	      _M_transfer(__last1, __first2, __last2);
+
+	    this->_M_inc_size(__x._M_get_size());
+	    __x._M_set_size(0);
 	  }
       }
 
diff --git a/libstdc++-v3/include/bits/stl_list.h b/libstdc++-v3/include/bits/stl_list.h
index dd9bba7..8e6567c 100644
--- a/libstdc++-v3/include/bits/stl_list.h
+++ b/libstdc++-v3/include/bits/stl_list.h
@@ -295,7 +295,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
   /// See bits/stl_deque.h's _Deque_base for an explanation.
   template<typename _Tp, typename _Alloc>
-    class _List_base
+    class _GLIBCXX_DEFAULT_ABI_TAG _List_base
     {
     protected:
       // NOTA BENE
@@ -316,6 +316,19 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
       typedef typename _Alloc::template rebind<_Tp>::other _Tp_alloc_type;
 
+      static size_t
+      _S_distance(const __detail::_List_node_base* __first,
+		  const __detail::_List_node_base* __last)
+      {
+	size_t __n = 0;
+	while (__first != __last)
+	  {
+	    __first = __first->_M_next;
+	    ++__n;
+	  }
+	return __n;
+      }
+
       struct _List_impl
       : public _Node_alloc_type
       {
@@ -338,6 +351,40 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
       _List_impl _M_impl;
 
+#if _GLIBCXX_USE_CXX11_ABI
+      size_t	 _M_size;
+
+      size_t _M_get_size() const { return _M_size; }
+
+      void _M_set_size(size_t __n) { _M_size = __n; }
+
+      void _M_inc_size(size_t __n) { _M_size += __n; }
+
+      void _M_dec_size(size_t __n) { _M_size -= __n; }
+
+      size_t
+      _M_distance(const __detail::_List_node_base* __first,
+		  const __detail::_List_node_base* __last) const
+      { return _S_distance(__first, __last); }
+
+      // return the stored size
+      size_t _M_node_count() const { return _M_size; }
+#else
+      // dummy implementations used when the size is not stored
+      size_t _M_get_size() const { return 0; }
+      void _M_set_size(size_t) { }
+      void _M_inc_size(size_t) { }
+      void _M_dec_size(size_t) { }
+      size_t _M_distance(const void*, const void*) const { return 0; }
+
+      // count the number of nodes
+      size_t _M_node_count() const
+      {
+	return _S_distance(_M_impl._M_node._M_next,
+			   std::__addressof(_M_impl._M_node));
+      }
+#endif
+
       _List_node<_Tp>*
       _M_get_node()
       { return _M_impl._Node_alloc_type::allocate(1); }
@@ -386,7 +433,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	    __node->_M_next = __xnode->_M_next;
 	    __node->_M_prev = __xnode->_M_prev;
 	    __node->_M_next->_M_prev = __node->_M_prev->_M_next = __node;
-	    __xnode->_M_next = __xnode->_M_prev = __xnode;
+	    _M_set_size(__x._M_get_size());
+	    __x._M_init();
 	  }
       }
 #endif
@@ -403,6 +451,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       {
         this->_M_impl._M_node._M_next = &this->_M_impl._M_node;
         this->_M_impl._M_node._M_prev = &this->_M_impl._M_node;
+	_M_set_size(0);
       }
     };
 
@@ -453,7 +502,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
    *  %empty. 
   */
   template<typename _Tp, typename _Alloc = std::allocator<_Tp> >
-    class list : protected _List_base<_Tp, _Alloc>
+    class _GLIBCXX_DEFAULT_ABI_TAG list : protected _List_base<_Tp, _Alloc>
     {
       // concept requirements
       typedef typename _Alloc::value_type                _Alloc_value_type;
@@ -893,7 +942,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       /**  Returns the number of elements in the %list.  */
       size_type
       size() const _GLIBCXX_NOEXCEPT
-      { return std::distance(begin(), end()); }
+      { return this->_M_node_count(); }
 
       /**  Returns the size() of the largest possible %list.  */
       size_type
@@ -1295,6 +1344,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	__detail::_List_node_base::swap(this->_M_impl._M_node, 
 					__x._M_impl._M_node);
 
+	size_t __xsize = __x._M_get_size();
+	__x._M_set_size(this->_M_get_size());
+	this->_M_set_size(__xsize);
+
 	// _GLIBCXX_RESOLVE_LIB_DEFECTS
 	// 431. Swapping containers with unequal allocators.
 	std::__alloc_swap<typename _Base::_Node_alloc_type>::
@@ -1339,6 +1392,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
 	    this->_M_transfer(__position._M_const_cast(),
 			      __x.begin(), __x.end());
+
+	    this->_M_inc_size(__x._M_get_size());
+	    __x._M_set_size(0);
 	  }
       }
 
@@ -1385,6 +1441,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
 	this->_M_transfer(__position._M_const_cast(),
 			  __i._M_const_cast(), __j);
+
+	this->_M_inc_size(1);
+	__x._M_dec_size(1);
       }
 
 #if __cplusplus >= 201103L
@@ -1443,6 +1502,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	    if (this != &__x)
 	      _M_check_equal_allocators(__x);
 
+	    size_t __n = this->_M_distance(__first._M_node, __last._M_node);
+	    this->_M_inc_size(__n);
+	    __x._M_dec_size(__n);
+
 	    this->_M_transfer(__position._M_const_cast(),
 			      __first._M_const_cast(),
 			      __last._M_const_cast());
@@ -1688,6 +1751,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       {
         _Node* __tmp = _M_create_node(__x);
         __tmp->_M_hook(__position._M_node);
+	this->_M_inc_size(1);
       }
 #else
      template<typename... _Args>
@@ -1696,6 +1760,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        {
 	 _Node* __tmp = _M_create_node(std::forward<_Args>(__args)...);
 	 __tmp->_M_hook(__position._M_node);
+	 this->_M_inc_size(1);
        }
 #endif
 
@@ -1703,6 +1768,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       void
       _M_erase(iterator __position) _GLIBCXX_NOEXCEPT
       {
+	this->_M_dec_size(1);
         __position._M_node->_M_unhook();
         _Node* __n = static_cast<_Node*>(__position._M_node);
 #if __cplusplus >= 201103L
diff --git a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/assign_neg.cc b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/assign_neg.cc
index 885e753..183753d 100644
--- a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/assign_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/assign_neg.cc
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1665 }
+// { dg-error "no matching" "" { target *-*-* } 1728 }
 
 #include <list>
 
diff --git a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_1_neg.cc b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_1_neg.cc
index ccd9d17..e81ff98 100644
--- a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_1_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_1_neg.cc
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1617 }
+// { dg-error "no matching" "" { target *-*-* } 1680 }
 
 #include <list>
 
diff --git a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_2_neg.cc b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_2_neg.cc
index 04966dc..c98aa0f 100644
--- a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_2_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_2_neg.cc
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1617 }
+// { dg-error "no matching" "" { target *-*-* } 1680 }
 
 #include <list>
 #include <utility>
diff --git a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/insert_neg.cc b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/insert_neg.cc
index 759dad6..1397796 100644
--- a/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/insert_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/insert_neg.cc
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1617 }
+// { dg-error "no matching" "" { target *-*-* } 1680 }
 
 #include <list>
 
diff --git a/libstdc++-v3/testsuite/ext/profile/mutex_extensions_neg.cc b/libstdc++-v3/testsuite/ext/profile/mutex_extensions_neg.cc
index 17df71a..5597c57 100644
--- a/libstdc++-v3/testsuite/ext/profile/mutex_extensions_neg.cc
+++ b/libstdc++-v3/testsuite/ext/profile/mutex_extensions_neg.cc
@@ -25,4 +25,4 @@
 
 #include <vector>
 
-// { dg-error "multiple inlined namespaces" "" { target *-*-* } 279 }
+// { dg-error "multiple inlined namespaces" "" { target *-*-* } 290 }
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]