[libcxx-commits] [libcxx] [libcxx] Remove empty ~__no_destroy (PR #89882)

Vitaly Buka via libcxx-commits libcxx-commits at lists.llvm.org
Thu Apr 25 12:55:36 PDT 2024


https://github.com/vitalybuka updated https://github.com/llvm/llvm-project/pull/89882

>From 9f35dfcea56c75d1700897ba664511702a75a6bb Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Tue, 23 Apr 2024 23:52:42 -0700
Subject: [PATCH 1/6] [libcxx] Remove empty ~__no_destroy

Primary motivation is that after #84651 msan will
complain if fields accessed after ~__no_destroy.

Previously msan assumed that __obj_ will have own
destructor, were we will poison the field, but it never
happened before.

After #84651 msan will complain on any field access
after `~__no_destroy`.

As is Msan does validate fields destruction order for
classes with trivial destructor.

Additionally empty destructor will register __cxa_atexit with -O0.
https://gcc.godbolt.org/z/hce587b65

We can not remove the destructor with union where
_Tp can have non-trivial destructor.

But we can remove destructor if we use in-place
new instead of union.
https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0.

Reviewers: #reviewers-libcxx

Pull Request: https://github.com/llvm/llvm-project/pull/89882
---
 libcxx/include/__utility/no_destroy.h         | 29 +++++++------------
 .../test/libcxx/utilities/no_destroy.pass.cpp | 28 ++++++++++++++++++
 2 files changed, 38 insertions(+), 19 deletions(-)
 create mode 100644 libcxx/test/libcxx/utilities/no_destroy.pass.cpp

diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index f9c1eb7bed4569..e5212cd4c40415 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -12,6 +12,7 @@
 #include <__config>
 #include <__type_traits/is_constant_evaluated.h>
 #include <__utility/forward.h>
+#include <new>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -29,33 +30,23 @@ struct __uninitialized_tag {};
 // initialization using __emplace.
 template <class _Tp>
 struct __no_destroy {
-  _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI explicit __no_destroy(__uninitialized_tag) : __dummy_() {
-    if (__libcpp_is_constant_evaluated()) {
-      __dummy_ = char();
-    }
-  }
-  _LIBCPP_HIDE_FROM_ABI ~__no_destroy() {
-    // nothing
-  }
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __no_destroy(__uninitialized_tag) {}
 
   template <class... _Args>
-  _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI explicit __no_destroy(_Args&&... __args)
-      : __obj_(std::forward<_Args>(__args)...) {}
+  _LIBCPP_HIDE_FROM_ABI explicit __no_destroy(_Args&&... __args) {
+    ::new ((void*)__obj_) _Tp(std::forward<_Args>(__args)...);
+  }
 
   template <class... _Args>
-  _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI _Tp& __emplace(_Args&&... __args) {
-    new (&__obj_) _Tp(std::forward<_Args>(__args)...);
-    return __obj_;
+  _LIBCPP_HIDE_FROM_ABI _Tp& __emplace(_Args&&... __args) {
+    return *(::new ((void*)__obj_) _Tp(std::forward<_Args>(__args)...));
   }
 
-  _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI _Tp& __get() { return __obj_; }
-  _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI _Tp const& __get() const { return __obj_; }
+  _LIBCPP_HIDE_FROM_ABI _Tp& __get() { return *reinterpret_cast<_Tp*>(__obj_); }
+  _LIBCPP_HIDE_FROM_ABI _Tp const& __get() const { return *reinterpret_cast<const _Tp*>(__obj_); }
 
 private:
-  union {
-    _Tp __obj_;
-    char __dummy_; // so we can initialize a member even with __uninitialized_tag for constexpr-friendliness
-  };
+  _ALIGNAS_TYPE(_Tp) unsigned char __obj_[sizeof(_Tp)] = {};
 };
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
new file mode 100644
index 00000000000000..35ca71661dbe46
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
@@ -0,0 +1,28 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <__utility/no_destroy.h>
+#include <cassert>
+
+#include "test_macros.h"
+
+struct DestroyLast {
+  ~DestroyLast() { assert(*ptr == 5); }
+
+  int* ptr;
+} last;
+
+static std::__no_destroy<int> nd_int(5);
+
+void test() { last.ptr = &nd_int.__get(); }
+
+int main(int, char**) {
+  test();
+
+  return 0;
+}

>From 7a65205fcb200819e4d9d7a09b4f5c18a2c2e07d Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 11:00:17 -0700
Subject: [PATCH 2/6] Remove "unsigned"

---
 libcxx/include/__utility/no_destroy.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index e5212cd4c40415..1eb6919d22b042 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -46,7 +46,7 @@ struct __no_destroy {
   _LIBCPP_HIDE_FROM_ABI _Tp const& __get() const { return *reinterpret_cast<const _Tp*>(__obj_); }
 
 private:
-  _ALIGNAS_TYPE(_Tp) unsigned char __obj_[sizeof(_Tp)] = {};
+  _ALIGNAS_TYPE(_Tp) char __obj_[sizeof(_Tp)] = {};
 };
 
 _LIBCPP_END_NAMESPACE_STD

>From a7d789a17767e2f63c0a4773599d97e6759ec381 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 11:03:17 -0700
Subject: [PATCH 3/6] Move field initialized and add constinit test

---
 libcxx/include/__utility/no_destroy.h            | 4 ++--
 libcxx/test/libcxx/utilities/no_destroy.pass.cpp | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index 1eb6919d22b042..8edd194577d7c7 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -30,7 +30,7 @@ struct __uninitialized_tag {};
 // initialization using __emplace.
 template <class _Tp>
 struct __no_destroy {
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __no_destroy(__uninitialized_tag) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __no_destroy(__uninitialized_tag) : __obj_() {}
 
   template <class... _Args>
   _LIBCPP_HIDE_FROM_ABI explicit __no_destroy(_Args&&... __args) {
@@ -46,7 +46,7 @@ struct __no_destroy {
   _LIBCPP_HIDE_FROM_ABI _Tp const& __get() const { return *reinterpret_cast<const _Tp*>(__obj_); }
 
 private:
-  _ALIGNAS_TYPE(_Tp) char __obj_[sizeof(_Tp)] = {};
+  _ALIGNAS_TYPE(_Tp) char __obj_[sizeof(_Tp)];
 };
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
index 35ca71661dbe46..86879f16fa7b83 100644
--- a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
+++ b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
@@ -26,3 +26,8 @@ int main(int, char**) {
 
   return 0;
 }
+#if TEST_STD_VER > 17
+// Test constexpr-constructibility.
+constinit std::__no_destroy<int> nd_int_const(std::__uninitialized_tag{});
+#endif
+

>From 36bffcebfeaff3207ad3a41d12b6bb44c2bc7dc8 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 11:03:37 -0700
Subject: [PATCH 4/6] Inline test

---
 libcxx/test/libcxx/utilities/no_destroy.pass.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
index 86879f16fa7b83..5d5d7934b5564e 100644
--- a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
+++ b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
@@ -19,13 +19,12 @@ struct DestroyLast {
 
 static std::__no_destroy<int> nd_int(5);
 
-void test() { last.ptr = &nd_int.__get(); }
-
 int main(int, char**) {
-  test();
+  last.ptr = &nd_int.__get();
 
   return 0;
 }
+
 #if TEST_STD_VER > 17
 // Test constexpr-constructibility.
 constinit std::__no_destroy<int> nd_int_const(std::__uninitialized_tag{});

>From 430f9d87d16ca4791b83db2ab09de1aa0ba848c0 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 11:04:36 -0700
Subject: [PATCH 5/6] reorder test

---
 libcxx/test/libcxx/utilities/no_destroy.pass.cpp | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
index 5d5d7934b5564e..9a874a64075372 100644
--- a/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
+++ b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
@@ -11,6 +11,11 @@
 
 #include "test_macros.h"
 
+#if TEST_STD_VER > 17
+// Test constexpr-constructibility.
+constinit std::__no_destroy<int> nd_int_const(std::__uninitialized_tag{});
+#endif
+
 struct DestroyLast {
   ~DestroyLast() { assert(*ptr == 5); }
 
@@ -24,9 +29,3 @@ int main(int, char**) {
 
   return 0;
 }
-
-#if TEST_STD_VER > 17
-// Test constexpr-constructibility.
-constinit std::__no_destroy<int> nd_int_const(std::__uninitialized_tag{});
-#endif
-

>From d438cbda537cd9487ea26592432f60bfbc415be2 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 12:54:20 -0700
Subject: [PATCH 6/6] init only for __libcpp_is_constant_evaluated

---
 libcxx/include/__utility/no_destroy.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index 8edd194577d7c7..cdae201b761753 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -30,7 +30,12 @@ struct __uninitialized_tag {};
 // initialization using __emplace.
 template <class _Tp>
 struct __no_destroy {
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __no_destroy(__uninitialized_tag) : __obj_() {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __no_destroy(__uninitialized_tag) {
+    if (__libcpp_is_constant_evaluated()) {
+      for (size_t __i = 0; __i != sizeof(__obj_); ++__i)
+        std::construct_at(__obj_ + __i);
+    }
+  }
 
   template <class... _Args>
   _LIBCPP_HIDE_FROM_ABI explicit __no_destroy(_Args&&... __args) {



More information about the libcxx-commits mailing list