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

[via libcxx-commits]

Author: Vitaly Buka
Date: 2024-04-26T22:27:16-07:00
New Revision: 85a9528aa1f2d54379bf972908e12ee2a6f07b4b

URL: https://github.com/llvm/llvm-project/commit/85a9528aa1f2d54379bf972908e12ee2a6f07b4b
DIFF: https://github.com/llvm/llvm-project/commit/85a9528aa1f2d54379bf972908e12ee2a6f07b4b.diff

LOG: [libcxx] Remove empty ~__no_destroy (#89882)

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

Added: 
    libcxx/test/libcxx/utilities/no_destroy.pass.cpp

Modified: 
    libcxx/include/__utility/no_destroy.h
    libcxx/test/libcxx/transitive_includes/cxx20.csv

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__utility/no_destroy.h b/libcxx/include/__utility/no_destroy.h
index f9c1eb7bed4569..8edd194577d7c7 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) : __obj_() {}
 
   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) char __obj_[sizeof(_Tp)];
 };
 
 _LIBCPP_END_NAMESPACE_STD

diff  --git a/libcxx/test/libcxx/transitive_includes/cxx20.csv b/libcxx/test/libcxx/transitive_includes/cxx20.csv
index 6b80790a9d19b5..7d31ba160ee1fb 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx20.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx20.csv
@@ -129,6 +129,7 @@ chrono cwchar
 chrono forward_list
 chrono limits
 chrono locale
+chrono new
 chrono optional
 chrono ostream
 chrono ratio

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..9a874a64075372
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/no_destroy.pass.cpp
@@ -0,0 +1,31 @@
+//===----------------------------------------------------------------------===//
+//
+// 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"
+
+#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); }
+
+  int* ptr;
+} last;
+
+static std::__no_destroy<int> nd_int(5);
+
+int main(int, char**) {
+  last.ptr = &nd_int.__get();
+
+  return 0;
+}