Containers default initialization
Jonathan Wakely
jwakely@redhat.com
Thu Jun 8 13:22:00 GMT 2017
On 07/06/17 22:44 +0200, François Dumont wrote:
>On 05/06/2017 13:31, Jonathan Wakely wrote:
>>On 04/06/17 22:26 +0200, François Dumont wrote:
>>>Hi
>>>
>>> I have eventually adapt the test to all containers and the
>>>result is successful for map/set/unordered_map/unordered_set. It
>>>is failing for deque/list/forward_list/vector/vector<bool>.
>>>
>>> I even try to change the test to look at the difference between
>>>an explicit call to the default constructor done through the
>>>placement new call and an implicit call done on normal
>>>declaration. I wondered if we would have the same kind of
>>>difference we have between a int i; and a int i(); I tried to set
>>>the stack to ~0 before declaring the instance. I know there is no
>>>guarantee on the content of the stack for the following
>>>declaration but do you think it is reliable enough to commit it ?
>>>
>>> Ok to commit the successful tests ?
>>
>>
>>No, I'm seeing failures for some of these if I add
>>// { dg-options "-O0" }
>>
>>> Franckly I don't understand the result of those tests. I would
>>>have expect map/set to fail and others to succeed. We might need
>>>help from compiler guys, no ?
>>
>>I think your tests are just insufficient. With optimisation enabled
>>(the testsuite uses -O2 by default) the compiler can remove the memset
>>just before the __aligned_buffer goes out of scope, because it is
>>unobservable in a correct program. This is similar to the situation
>>described at https://gcc.gnu.org/gcc-6/porting_to.html#flifetime-dse
>>
>>If I change the placement new-expressions to use default-init instead
>>of value-init and use -O0 then I see all four tests FAIL here:
>>
>>test_type *tmp = ::new(buf._M_addr()) test_type; // not test_type()
>
>Ok, I didn't know we could do this.
>
>So I have added value_init.cc tests showing the problem for all containers.
>
>The patch also contains the fix for rb_tree so that map/set are now
>successful.
>
>Looks there is really a misunderstanding on what the compiler is
>doing. If container calls _Node_allocator() it will be transform by
>compiler into default initialization if container default container is
>being called or into value init if container is value initialized
Oh dear, we have a compiler bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65816
>which takes place only if I call this:
>
>test_type *tmp = ::new(buf._M_addr()) test_type {};
>
>To force default/value init looks like gcc forces you to explicitly
>build an allocator instance like in the attached patch.
I'm getting uncomfortable with all this churn just to be able to use
=default, but requiring more new base classes and #if blocks. We have
one known regression already, which needs your patch to fix it.
(Admittedly, it's not a very serious regression, because I think real
world allocators that are stateful but don't have user-provided
default constructors are probably very very unlikely).
I'm leaning towards just make the exception specifications correct
using _GLIBCXX_NOEXCEPT_IF and not doing any refactoring.
That also has the benefit that the code might help guide the future
direction of the standard, by telling us what conditions could be used
if we add noexcept to these constructors in the standard.
>Index: include/bits/stl_tree.h
>===================================================================
>--- include/bits/stl_tree.h (revision 248855)
>+++ include/bits/stl_tree.h (working copy)
>@@ -687,9 +687,17 @@
>
> #if __cplusplus < 201103L
> _Rb_tree_impl()
>+ : _Node_allocator()
> { }
> #else
>- _Rb_tree_impl() = default;
>+ _Rb_tree_impl()
>+ noexcept(
>+ noexcept(_Node_allocator()) && noexcept(_Base_key_compare()) )
>+ : _Rb_tree_impl(_Key_compare(), _Node_allocator())
If we didn't have the PR65816 bug this should be simply:
_Rb_tree_impl()
_GLIBCXX_NOEXCEPT_IF(
is_nothrow_default_constructible<_Key_compare>::value
&& is_nothrow_default_constructible<_Node_allocator>::value
)
: _Node_allocator()
{ }
i.e. the same for C++98 and other modes. Because of the compiler bug
that doesn't work. I think we should fix the compiler.
>+ { }
>+#endif
>+
>+#if __cplusplus >= 201103L
There's no need for an #endif/#if here, the previous block is already
C++11-and-later code.
> _Rb_tree_impl(_Rb_tree_impl&&) = default;
> #endif
>
>Index: testsuite/23_containers/map/allocator/default_init.cc
>===================================================================
>--- testsuite/23_containers/map/allocator/default_init.cc (nonexistent)
>+++ testsuite/23_containers/map/allocator/default_init.cc (working copy)
>@@ -0,0 +1,52 @@
>+// Copyright (C) 2017 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/>.
>+
>+// { dg-do run { target c++11 } }
>+// { dg-options "-O0" }
>+
>+#include <map>
>+#include <testsuite_hooks.h>
>+#include <testsuite_allocator.h>
>+
>+#include <ext/aligned_buffer.h>
>+
>+using T = int;
>+
>+using __gnu_test::default_init_allocator;
>+
>+void test01()
>+{
>+ typedef default_init_allocator<std::pair<const T, T>> alloc_type;
>+ typedef std::map<T, T, std::less<T>, alloc_type> test_type;
>+
>+ __gnu_cxx::__aligned_buffer<test_type> buf;
>+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
>+
>+ VERIFY( buf._M_ptr()->get_allocator().state != 0 );
This is undefined. There is no object here, so you can't dereference
the value returned by bug._M_ptr().
>+ test_type *tmp = ::new(buf._M_addr()) test_type();
This file is called default_init.cc but this is value-initialization.
>+ VERIFY( tmp->get_allocator().state == 0 );
>+
>+ tmp->~test_type();
>+}
>+
>+int main()
>+{
>+ test01();
>+ return 0;
>+}
>Index: testsuite/23_containers/map/allocator/value_init.cc
>===================================================================
>--- testsuite/23_containers/map/allocator/value_init.cc (nonexistent)
>+++ testsuite/23_containers/map/allocator/value_init.cc (working copy)
>@@ -0,0 +1,52 @@
>+// Copyright (C) 2017 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/>.
>+
>+// { dg-do run { target c++11 } }
>+// { dg-options "-O0" }
>+
>+#include <map>
>+#include <testsuite_hooks.h>
>+#include <testsuite_allocator.h>
>+
>+#include <ext/aligned_buffer.h>
>+
>+using T = int;
>+
>+using __gnu_test::default_init_allocator;
>+
>+void test01()
>+{
>+ typedef default_init_allocator<std::pair<const T, T>> alloc_type;
>+ typedef std::map<T, T, std::less<T>, alloc_type> test_type;
>+
>+ __gnu_cxx::__aligned_buffer<test_type> buf;
>+ __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
>+
>+ VERIFY( buf._M_ptr()->get_allocator().state != 0 );
>+
>+ test_type *tmp = ::new(buf._M_addr()) test_type;
This file is called value_init.cc but this is default-initialization.
Same problem in the other new tests as well.
Can't we just have one file per container type (maybe just called
default_init.cc) which tests default-initialization in test01() and
value-initialization in a test02() function?
More information about the Gcc-patches
mailing list