Skip to content

[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

Merged

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Apr 24, 2024

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

@vitalybuka vitalybuka requested a review from a team as a code owner April 24, 2024 07:03
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 24, 2024

@llvm/pr-subscribers-libcxx

Author: Vitaly Buka (vitalybuka)

Changes

Empty destructor will register __cxa_atexit.
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.


Full diff: https://github.com/llvm/llvm-project/pull/89882.diff

1 Files Affected:

  • (modified) libcxx/include/__utility/no_destroy.h (+10-18)
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

@vitalybuka vitalybuka changed the title [libcxx] Avoid __cxa_atexit with -O0 [libcxx] Remove empty ~__no_destroy Apr 24, 2024
frederick-vs-ja

This comment was marked as resolved.

@vitalybuka vitalybuka requested review from frederick-vs-ja, ldionne and EricWF and removed request for frederick-vs-ja April 24, 2024 16:55
vitalybuka added a commit that referenced this pull request Apr 25, 2024
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
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/libcxx-avoid-__cxa_atexit-with-o0 branch from 1161ecf to 430f9d8 Compare April 25, 2024 18:06
@vitalybuka vitalybuka requested a review from ldionne April 25, 2024 18:07
vitalybuka added a commit that referenced this pull request Apr 25, 2024
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
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/libcxx-avoid-__cxa_atexit-with-o0 branch from 1824358 to 90a2082 Compare April 25, 2024 23:06
Copy link

github-actions bot commented Apr 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

vitalybuka added a commit that referenced this pull request Apr 26, 2024
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
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/libcxx-avoid-__cxa_atexit-with-o0 branch 2 times, most recently from dd0d9f2 to 2a4d746 Compare April 26, 2024 04:33
@ldionne ldionne self-assigned this Apr 26, 2024
vitalybuka added a commit that referenced this pull request Apr 26, 2024
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
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/libcxx-avoid-__cxa_atexit-with-o0 branch from 2a4d746 to bd0af57 Compare April 26, 2024 18:07
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
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/libcxx-avoid-__cxa_atexit-with-o0 branch from bd0af57 to 5487aaf Compare April 26, 2024 22:25
@vitalybuka vitalybuka merged commit 85a9528 into main Apr 27, 2024
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/libcxx-avoid-__cxa_atexit-with-o0 branch April 27, 2024 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants