[libcxx-commits] [libcxx] [libcxx] Remove empty ~__no_destroy (PR #89882)
Vitaly Buka via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 25 21:31:35 PDT 2024
https://github.com/vitalybuka updated https://github.com/llvm/llvm-project/pull/89882
>From 91a4e94a8b6db9a9867a3fdc4288237a9304a677 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/9] [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 7af3423de2c49655d664135dcde57ac34cb705f9 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/9] 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 8b72be78462729a6cba0fc5939a601e9b63152dd 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/9] 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 3101c5db1b450d7b82f0601db2ec750df831e322 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/9] 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 5acfb99ab2a42da983b22c3e40f558c7c4ee3af3 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/9] 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 699f60a2664c05e0d69bec04a2dad320a05e0945 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/9] 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) {
>From b37628659220fc2d80991c4cb58dd02075697a1a Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 12:59:32 -0700
Subject: [PATCH 7/9] construct with 0
---
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 cdae201b761753..d99e15f00c82e8 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -33,7 +33,7 @@ struct __no_destroy {
_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);
+ std::construct_at(__obj_ + __i, 0);
}
}
>From ae1511af569e9c838533ee79ba11d8bf3000951b Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 13:57:44 -0700
Subject: [PATCH 8/9] fix build
---
libcxx/include/__utility/no_destroy.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index d99e15f00c82e8..f87a168ac94949 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -10,6 +10,7 @@
#define _LIBCPP___UTILITY_NO_DESTROY_H
#include <__config>
+#include <__memory/construct_at.h>
#include <__type_traits/is_constant_evaluated.h>
#include <__utility/forward.h>
#include <new>
@@ -33,7 +34,7 @@ struct __no_destroy {
_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, 0);
+ std::__construct_at(__obj_ + __i);
}
}
>From dd0d9f220326a8fa4a1700b5a9029f9aa7b97c58 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Thu, 25 Apr 2024 16:05:59 -0700
Subject: [PATCH 9/9] =?UTF-8?q?undo=20=5F=5Flibcpp=5Fis=5Fconstant=5Fevalu?=
=?UTF-8?q?ated=20to=20fix=20=E2=80=98constexpr=E2=80=99=20constructor=20d?=
=?UTF-8?q?oes=20not=20have=20empty=20body?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
libcxx/include/__utility/no_destroy.h | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index f87a168ac94949..8edd194577d7c7 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -10,7 +10,6 @@
#define _LIBCPP___UTILITY_NO_DESTROY_H
#include <__config>
-#include <__memory/construct_at.h>
#include <__type_traits/is_constant_evaluated.h>
#include <__utility/forward.h>
#include <new>
@@ -31,12 +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) {
- if (__libcpp_is_constant_evaluated()) {
- for (size_t __i = 0; __i != sizeof(__obj_); ++__i)
- std::__construct_at(__obj_ + __i);
- }
- }
+ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __no_destroy(__uninitialized_tag) : __obj_() {}
template <class... _Args>
_LIBCPP_HIDE_FROM_ABI explicit __no_destroy(_Args&&... __args) {
More information about the libcxx-commits
mailing list