[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