diff mbox

PR libstdc++/80553 don't allow destroying non-destructible types

Message ID 20170428125629.GA14095@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely April 28, 2017, 12:56 p.m. UTC 
We optimize _Destroy and _Destroy_n to do nothing when the type has a
trivial destructor, which means we do nothing (instead of giving an
error) when trying to destroy types with deleted destructors.

Because deleted destructors are trivial. Because C++.

This adds static assertions to reject attempts to destroy
indestructible objects. I put them in the top-level functions, not the
specialization for trivial destructors, which means we also get the
static assertion for types with a private destructor. It's debatable
whether that's better: you get a failed "type is destructible"
assertion instead of an error saying the destructor is private, which
is a bit more precise. If people prefer the compiler error about a
private destructor we could move the assertions into the
specializations for types with trivial destructors.

	PR libstdc++/80553
	* include/bits/stl_construct.h (_Destroy, _Destroy_n): Add static
	assertions to ensure type is destructible.
	(destroy_at, destroy, destroy_n): Move from stl_uninitialized.h.
	* include/bits/stl_uninitialized.h (destroy_at, destroy, destroy_n):
	Move to stl_construct.h.
	* testsuite/20_util/specialized_algorithms/memory_management_tools/
	destroy_neg.cc: New test.
	* testsuite/23_containers/vector/cons/destructible_neg.cc: New test.

Tested powerpc64le-linux, committed to trunk.
commit 2b0c41d7cc1182382582842ccc70f6effc997d6f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Apr 28 12:34:27 2017 +0100

    PR libstdc++/80553 don't allow destroying non-destructible types
    
    	PR libstdc++/80553
    	* include/bits/stl_construct.h (_Destroy, _Destroy_n): Add static
    	assertions to ensure type is destructible.
    	(destroy_at, destroy, destroy_n): Move from stl_uninitialized.h.
    	* include/bits/stl_uninitialized.h (destroy_at, destroy, destroy_n):
    	Move to stl_construct.h.
    	* testsuite/20_util/specialized_algorithms/memory_management_tools/
    	destroy_neg.cc: New test.
    	* testsuite/23_containers/vector/cons/destructible_neg.cc: New test.

Comments

Jonathan Wakely May 2, 2017, 9:16 a.m. UTC | #1 
On 28/04/17 13:56 +0100, Jonathan Wakely wrote:
>We optimize _Destroy and _Destroy_n to do nothing when the type has a
>trivial destructor, which means we do nothing (instead of giving an
>error) when trying to destroy types with deleted destructors.

I wonder if this optimisation should even exist. The compiler should
be able to optimise away a loop that just calls trivial destructors,
without help from the library.
Jonathan Wakely May 2, 2017, 9:46 a.m. UTC | #2 
On 02/05/17 10:16 +0100, Jonathan Wakely wrote:
>On 28/04/17 13:56 +0100, Jonathan Wakely wrote:
>>We optimize _Destroy and _Destroy_n to do nothing when the type has a
>>trivial destructor, which means we do nothing (instead of giving an
>>error) when trying to destroy types with deleted destructors.
>
>I wonder if this optimisation should even exist. The compiler should
>be able to optimise away a loop that just calls trivial destructors,
>without help from the library.

The compiler can indeed do that optimisation, even for destructors
like ~T() { } that are empty, but not trivial according to the
language rules. The libstdc++ optimisation does make a difference at
-O0 though. If we get any more bugs in that code I think we should
just remove it though, and let the compiler do the right thing.
Marc Glisse May 2, 2017, 3:30 p.m. UTC | #3 
On Tue, 2 May 2017, Jonathan Wakely wrote:

> On 02/05/17 10:16 +0100, Jonathan Wakely wrote:
>> On 28/04/17 13:56 +0100, Jonathan Wakely wrote:
>>> We optimize _Destroy and _Destroy_n to do nothing when the type has a
>>> trivial destructor, which means we do nothing (instead of giving an
>>> error) when trying to destroy types with deleted destructors.
>> 
>> I wonder if this optimisation should even exist. The compiler should
>> be able to optimise away a loop that just calls trivial destructors,
>> without help from the library.
>
> The compiler can indeed do that optimisation, even for destructors
> like ~T() { } that are empty, but not trivial according to the
> language rules. The libstdc++ optimisation does make a difference at
> -O0 though. If we get any more bugs in that code I think we should
> just remove it though, and let the compiler do the right thing.

Does the compiler manage it for all containers, even those with iterators 
much more complicated than vector's? I'd rather keep the special code in 
the library, if it doesn't cause too much trouble.
Jonathan Wakely May 2, 2017, 3:57 p.m. UTC | #4 
On 02/05/17 17:30 +0200, Marc Glisse wrote:
>On Tue, 2 May 2017, Jonathan Wakely wrote:
>
>>On 02/05/17 10:16 +0100, Jonathan Wakely wrote:
>>>On 28/04/17 13:56 +0100, Jonathan Wakely wrote:
>>>>We optimize _Destroy and _Destroy_n to do nothing when the type has a
>>>>trivial destructor, which means we do nothing (instead of giving an
>>>>error) when trying to destroy types with deleted destructors.
>>>
>>>I wonder if this optimisation should even exist. The compiler should
>>>be able to optimise away a loop that just calls trivial destructors,
>>>without help from the library.
>>
>>The compiler can indeed do that optimisation, even for destructors
>>like ~T() { } that are empty, but not trivial according to the
>>language rules. The libstdc++ optimisation does make a difference at
>>-O0 though. If we get any more bugs in that code I think we should
>>just remove it though, and let the compiler do the right thing.
>
>Does the compiler manage it for all containers, even those with 
>iterators much more complicated than vector's?

It seems to for std::deque (not very complicated) and std::map
(moderately complicated). I didn't try for something like a
directory_iterator which almost certainly wouldn't get optimised away!

>I'd rather keep the 
>special code in the library, if it doesn't cause too much trouble.

Yes, assuming the code's correct now then we might as well keep it,
but if it's a source of too many more bugs then it starts to cause too
much trouble.
diff mbox

Patch

diff --git a/libstdc++-v3/include/bits/stl_construct.h b/libstdc++-v3/include/bits/stl_construct.h
index c1504e9..71483b4 100644
--- a/libstdc++-v3/include/bits/stl_construct.h
+++ b/libstdc++-v3/include/bits/stl_construct.h
@@ -128,6 +128,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       typedef typename iterator_traits<_ForwardIterator>::value_type
                        _Value_type;
+#if __cplusplus >= 201103L
+      // A deleted destructor is trivial, this ensures we reject such types:
+      static_assert(is_destructible<_Value_type>::value,
+		    "value type is destructible");
+#endif
       std::_Destroy_aux<__has_trivial_destructor(_Value_type)>::
 	__destroy(__first, __last);
     }
@@ -151,10 +156,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       template<typename _ForwardIterator, typename _Size>
         static _ForwardIterator
         __destroy_n(_ForwardIterator __first, _Size __count)
-      {
-	 std::advance(__first, __count);
-	 return __first;
-      }
+	{
+	  std::advance(__first, __count);
+	  return __first;
+	}
     };
 
   /**
@@ -168,6 +173,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       typedef typename iterator_traits<_ForwardIterator>::value_type
                        _Value_type;
+#if __cplusplus >= 201103L
+      // A deleted destructor is trivial, this ensures we reject such types:
+      static_assert(is_destructible<_Value_type>::value,
+		    "value type is destructible");
+#endif
       return std::_Destroy_n_aux<__has_trivial_destructor(_Value_type)>::
 	__destroy_n(__first, __count);
     }
@@ -196,6 +206,29 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Destroy(__first, __last);
     }
 
+#if __cplusplus > 201402L
+  template <typename _Tp>
+    inline void
+    destroy_at(_Tp* __location)
+    {
+      std::_Destroy(__location);
+    }
+
+  template <typename _ForwardIterator>
+    inline void
+    destroy(_ForwardIterator __first, _ForwardIterator __last)
+    {
+      std::_Destroy(__first, __last);
+    }
+
+  template <typename _ForwardIterator, typename _Size>
+    inline _ForwardIterator
+    destroy_n(_ForwardIterator __first, _Size __count)
+    {
+      return std::_Destroy_n(__first, __count);
+    }
+#endif
+
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
 
diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
index d0e2b2d..c2ba863 100644
--- a/libstdc++-v3/include/bits/stl_uninitialized.h
+++ b/libstdc++-v3/include/bits/stl_uninitialized.h
@@ -831,77 +831,54 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline void
     uninitialized_default_construct(_ForwardIterator __first,
 				    _ForwardIterator __last)
-  {
-    __uninitialized_default_novalue(__first, __last);
-  }
+    {
+      __uninitialized_default_novalue(__first, __last);
+    }
 
   template <typename _ForwardIterator, typename _Size>
     inline _ForwardIterator
     uninitialized_default_construct_n(_ForwardIterator __first, _Size __count)
-  {
-    return __uninitialized_default_novalue_n(__first, __count);
-  }
+    {
+      return __uninitialized_default_novalue_n(__first, __count);
+    }
 
   template <typename _ForwardIterator>
     inline void
     uninitialized_value_construct(_ForwardIterator __first,
 				  _ForwardIterator __last)
-  {
-    return __uninitialized_default(__first, __last);
-  }
+    {
+      return __uninitialized_default(__first, __last);
+    }
 
   template <typename _ForwardIterator, typename _Size>
     inline _ForwardIterator
     uninitialized_value_construct_n(_ForwardIterator __first, _Size __count)
-  {
-    return __uninitialized_default_n(__first, __count);
-  }
+    {
+      return __uninitialized_default_n(__first, __count);
+    }
 
   template <typename _InputIterator, typename _ForwardIterator>
     inline _ForwardIterator
     uninitialized_move(_InputIterator __first, _InputIterator __last,
 		       _ForwardIterator __result)
-  {
-    return std::uninitialized_copy
-      (_GLIBCXX_MAKE_MOVE_ITERATOR(__first),
-       _GLIBCXX_MAKE_MOVE_ITERATOR(__last), __result);
-  }
+    {
+      return std::uninitialized_copy
+	(_GLIBCXX_MAKE_MOVE_ITERATOR(__first),
+	 _GLIBCXX_MAKE_MOVE_ITERATOR(__last), __result);
+    }
 
   template <typename _InputIterator, typename _Size, typename _ForwardIterator>
     inline pair<_InputIterator, _ForwardIterator>
     uninitialized_move_n(_InputIterator __first, _Size __count,
 			 _ForwardIterator __result)
-  {
-    auto __res = std::__uninitialized_copy_n_pair
-      (_GLIBCXX_MAKE_MOVE_ITERATOR(__first),
-       __count, __result);
-    return {__res.first.base(), __res.second};
-  }
-
-  template <typename _Tp>
-    inline void
-    destroy_at(_Tp* __location)
-  {
-    std::_Destroy(__location);
-  }
-
-  template <typename _ForwardIterator>
-    inline void
-    destroy(_ForwardIterator __first, _ForwardIterator __last)
-  {
-    std::_Destroy(__first, __last);
-  }
-
-  template <typename _ForwardIterator, typename _Size>
-    inline _ForwardIterator
-    destroy_n(_ForwardIterator __first, _Size __count)
-  {
-    return std::_Destroy_n(__first, __count);
-  }
-
+    {
+      auto __res = std::__uninitialized_copy_n_pair
+	(_GLIBCXX_MAKE_MOVE_ITERATOR(__first),
+	 __count, __result);
+      return {__res.first.base(), __res.second};
+    }
 #endif
 
-
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
 
diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/memory_management_tools/destroy_neg.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/memory_management_tools/destroy_neg.cc
new file mode 100644
index 0000000..663b2c0
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/memory_management_tools/destroy_neg.cc
@@ -0,0 +1,50 @@ 
+// 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-options "-std=gnu++17" }
+// { dg-do compile { target c++1z } }
+
+#include <memory>
+
+// This has a trivial destructor, but should not be destructible!
+struct DeletedDtor {
+  ~DeletedDtor() = delete;
+};
+
+void
+test01()
+{
+  alignas(DeletedDtor) unsigned char buf[sizeof(DeletedDtor)];
+  auto p = ::new (buf) DeletedDtor();
+  std::destroy(p, p + 1);	// { dg-error "here" }
+  std::destroy_n(p, 1);		// { dg-error "here" }
+}
+
+class PrivateDtor {
+  ~PrivateDtor() { }
+};
+
+void
+test02()
+{
+  alignas(PrivateDtor) unsigned char buf[sizeof(PrivateDtor)];
+  auto p = ::new (buf) PrivateDtor();
+  std::destroy(p, p + 1);	// { dg-error "here" }
+  std::destroy_n(p, 1);		// { dg-error "here" }
+}
+
+// { dg-error "value type is destructible" "" { target *-*-* } 0 }
diff --git a/libstdc++-v3/testsuite/23_containers/vector/cons/destructible_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/cons/destructible_neg.cc
new file mode 100644
index 0000000..4898595
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/cons/destructible_neg.cc
@@ -0,0 +1,44 @@ 
+// 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 compile { target c++11 } }
+
+#include <vector>
+
+// PR libstdc++/80553
+
+struct DeletedDtor {
+  ~DeletedDtor() = delete;
+};
+
+class PrivateDtor {
+  ~PrivateDtor() { }
+};
+
+void
+test01()
+{
+  std::vector<DeletedDtor> v; // { dg-error "here" }
+}
+
+void
+test02()
+{
+  std::vector<PrivateDtor> v; // { dg-error "here" }
+}
+
+// { dg-error "value type is destructible" "" { target *-*-* } 0 }