-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[libcxx] Remove empty ~__no_destroy #89882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libcxx] Remove empty ~__no_destroy #89882
Conversation
@llvm/pr-subscribers-libcxx Author: Vitaly Buka (vitalybuka) ChangesEmpty destructor will register __cxa_atexit. We can not remove the destructor with union where But we can remove destructor if we use in-place Full diff: https://github.com/llvm/llvm-project/pull/89882.diff 1 Files Affected:
diff --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index f9c1eb7bed4569..833e16116c42d7 100644
--- a/libcxx/include/__utility/no_destroy.h
+++ b/libcxx/include/__utility/no_destroy.h
@@ -29,33 +29,25 @@ 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_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI 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_CONSTEXPR _LIBCPP_HIDE_FROM_ABI explicit __no_destroy(_Args&&... __args) {
+ new (&__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_;
+ return *(new (&__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_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI _Tp& __get() { return *reinterpret_cast<_Tp*>(__obj_); }
+ _LIBCPP_CONSTEXPR_SINCE_CXX14 _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(_Tp) unsigned char __obj_[sizeof(_Tp)] = {};
};
_LIBCPP_END_NAMESPACE_STD
|
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: #89882
1161ecf
to
430f9d8
Compare
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: #89882
1824358
to
90a2082
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
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: #89882
dd0d9f2
to
2a4d746
Compare
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: #89882
2a4d746
to
bd0af57
Compare
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: #89882
bd0af57
to
5487aaf
Compare
Primary motivation: is that after #84651 msan will
complain if fields accessed after ~__no_destroy.
My understanding of the https://eel.is/c++draft/basic.life#10
Static object with trivial destruction has program lifetime.
Static object with empty destuctor has implicit lifetime, and
accessing the object after lifetime is UB.
It was UB before #84651, it's just msan ignored union members.
Existing code with unions uses empty destructor, so accessing after
the main() can cause UB.
"placement new" version can have trivial destructor, so there is no end
of lifetime.
Secondary motivation: 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.
New test fails without the patch on
https://lab.llvm.org/buildbot/#/builders/sanitizer-x86_64-linux-bootstrap-msan