[libcxx-commits] [libcxx] f706b01 - Revert "Revert "[libcxx] Put clang::trivial_abi on std::unique_ptr, std::shared_ptr, and std::weak_ptr""
Vy Nguyen via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Jul 5 10:45:24 PDT 2020
Author: Vy Nguyen
Date: 2020-07-05T13:44:42-04:00
New Revision: f706b01a00676ef0e7aefb253316c6418f022fa2
URL: https://github.com/llvm/llvm-project/commit/f706b01a00676ef0e7aefb253316c6418f022fa2
DIFF: https://github.com/llvm/llvm-project/commit/f706b01a00676ef0e7aefb253316c6418f022fa2.diff
LOG: Revert "Revert "[libcxx] Put clang::trivial_abi on std::unique_ptr, std::shared_ptr, and std::weak_ptr""
This reverts commit dc13ac02800220a33828ac3da629f382ca7e308d.
Rolling forward + fix typos and unused variables in tests
Differential Revision: https://reviews.llvm.org/D82490
Added:
libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst
libcxx/test/libcxx/memory/trivial_abi/shared_ptr_arg.pass.cpp
libcxx/test/libcxx/memory/trivial_abi/unique_ptr_arg.pass.cpp
libcxx/test/libcxx/memory/trivial_abi/unique_ptr_array.pass.cpp
libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp
libcxx/test/libcxx/memory/trivial_abi/unique_ptr_ret.pass.cpp
libcxx/test/libcxx/memory/trivial_abi/weak_ptr_ret.pass.cpp
Modified:
libcxx/docs/index.rst
libcxx/include/__config
libcxx/include/memory
Removed:
################################################################################
diff --git a/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst b/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst
new file mode 100644
index 000000000000..a0f260a44e81
--- /dev/null
+++ b/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst
@@ -0,0 +1,149 @@
+=============================================
+Enable std::unique_ptr [[clang::trivial_abi]]
+=============================================
+
+Background
+==========
+
+Consider the follow snippets
+
+
+.. code-block:: cpp
+
+ void raw_func(Foo* raw_arg) { ... }
+ void smart_func(std::unique_ptr<Foo> smart_arg) { ... }
+
+ Foo* raw_ptr_retval() { ... }
+ std::unique_ptr<Foo*> smart_ptr_retval() { ... }
+
+
+
+The argument ``raw_arg`` could be passed in a register but ``smart_arg`` could not, due to current
+implementation.
+
+Specifically, in the ``smart_arg`` case, the caller secretly constructs a temporary ``std::unique_ptr``
+in its stack-frame, and then passes a pointer to it to the callee in a hidden parameter.
+Similarly, the return value from ``smart_ptr_retval`` is secretly allocated in the caller and
+passed as a secret reference to the callee.
+
+
+Goal
+===================
+
+``std::unique_ptr`` is passed directly in a register.
+
+Design
+======
+
+* Annotate the two definitions of ``std::unique_ptr`` with ``clang::trivial_abi`` attribute.
+* Put the attribuate behind a flag because this change has potential compilation and runtime breakages.
+
+
+This comes with some side effects:
+
+* ``std::unique_ptr`` parameters will now be destroyed by callees, rather than callers.
+ It is worth noting that destruction by callee is not unique to the use of trivial_abi attribute.
+ In most Microsoft's ABIs, arguments are always destroyed by the callee.
+
+ Consequently, this may change the destruction order for function parameters to an order that is non-conforming to the standard.
+ For example:
+
+
+ .. code-block:: cpp
+
+ struct A { ~A(); };
+ struct B { ~B(); };
+ struct C { C(A, unique_ptr<B>, A) {} };
+ C c{{}, make_unique<B>, {}};
+
+
+ In a conforming implementation, the destruction order for C::C's parameters is required to be ``~A(), ~B(), ~A()`` but with this mode enabled, we'll instead see ``~B(), ~A(), ~A()``.
+
+* Reduced code-size.
+
+
+Performance impact
+------------------
+
+Google has measured performance improvements of up to 1.6% on some large server macrobenchmarks, and a small reduction in binary sizes.
+
+This also affects null pointer optimization
+
+Clang's optimizer can now figure out when a `std::unique_ptr` is known to contain *non*-null.
+(Actually, this has been a *missed* optimization all along.)
+
+
+.. code-block:: cpp
+
+ struct Foo {
+ ~Foo();
+ };
+ std::unique_ptr<Foo> make_foo();
+ void do_nothing(const Foo&)
+
+ void bar() {
+ auto x = make_foo();
+ do_nothing(*x);
+ }
+
+
+With this change, ``~Foo()`` will be called even if ``make_foo`` returns ``unique_ptr<Foo>(nullptr)``.
+The compiler can now assume that ``x.get()`` cannot be null by the end of ``bar()``, because
+the deference of ``x`` would be UB if it were ``nullptr``. (This dereference would not have caused
+a segfault, because no load is generated for dereferencing a pointer to a reference. This can be detected with ``-fsanitize=null``).
+
+
+Potential breakages
+-------------------
+
+The following breakages were discovered by enabling this change and fixing the resulting issues in a large code base.
+
+- Compilation failures
+
+ - Function definitions now require complete type ``T`` for parameters with type ``std::unique_ptr<T>``. The following code will no longer compile.
+
+ .. code-block:: cpp
+
+ class Foo;
+ void func(std::unique_ptr<Foo> arg) { /* never use `arg` directly */ }
+
+ - Fix: Remove forward-declaration of ``Foo`` and include its proper header.
+
+- Runtime Failures
+
+ - Lifetime of ``std::unique_ptr<>`` arguments end earlier (at the end of the callee's body, rather than at the end of the full expression containing the call).
+
+ .. code-block:: cpp
+
+ util::Status run_worker(std::unique_ptr<Foo>);
+ void func() {
+ std::unique_ptr<Foo> smart_foo = ...;
+ Foo* owned_foo = smart_foo.get();
+ // Currently, the following would "work" because the argument to run_worker() is deleted at the end of func()
+ // With the new calling convention, it will be deleted at the end of run_worker(),
+ // making this an access to freed memory.
+ owned_foo->Bar(run_worker(std::move(smart_foo)));
+ ^
+ // <<<Crash expected here
+ }
+
+ - Lifetime of local *returned* ``std::unique_ptr<>`` ends earlier.
+
+ Spot the bug:
+
+ .. code-block:: cpp
+
+ std::unique_ptr<Foo> create_and_subscribe(Bar* subscriber) {
+ auto foo = std::make_unique<Foo>();
+ subscriber->sub([&foo] { foo->do_thing();} );
+ return foo;
+ }
+
+ One could point out this is an obvious stack-use-after return bug.
+ With the current calling convention, running this code with ASAN enabled, however, would not yield any "issue".
+ So is this a bug in ASAN? (Spoiler: No)
+
+ This currently would "work" only because the storage for ``foo`` is in the caller's stackframe.
+ In other words, ``&foo`` in callee and ``&foo`` in the caller are the same address.
+
+ASAN can be used to detect both of these.
diff --git a/libcxx/docs/index.rst b/libcxx/docs/index.rst
index c60a72d49724..75d5b8226b4c 100644
--- a/libcxx/docs/index.rst
+++ b/libcxx/docs/index.rst
@@ -164,6 +164,7 @@ Design Documents
DesignDocs/FileTimeType
DesignDocs/FeatureTestMacros
DesignDocs/ExtendedCXX03Support
+ DesignDocs/UniquePtrTrivialAbi
* `<atomic> design <http://libcxx.llvm.org/atomic_design.html>`_
* `<type_traits> design <http://libcxx.llvm.org/type_traits_design.html>`_
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 7e4c37431ea4..e040a7a3e503 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -105,6 +105,10 @@
// Re-worked external template instantiations for std::string with a focus on
// performance and fast-path inlining.
# define _LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION
+// Enable clang::trivial_abi on std::unique_ptr.
+# define _LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI
+// Enable clang::trivial_abi on std::shared_ptr and std::weak_ptr
+# define _LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI
#elif _LIBCPP_ABI_VERSION == 1
# if !defined(_LIBCPP_OBJECT_FORMAT_COFF)
// Enable compiling copies of now inline methods into the dylib to support
diff --git a/libcxx/include/memory b/libcxx/include/memory
index 1f9f36c5bbbb..e38e80568eb9 100644
--- a/libcxx/include/memory
+++ b/libcxx/include/memory
@@ -338,7 +338,7 @@ public:
pointer release() noexcept;
void reset(pointer p = pointer()) noexcept;
void reset(nullptr_t) noexcept;
- template <class U> void reset(U) = delete;
+ template <class U> void reset(U) = delete;
void swap(unique_ptr& u) noexcept;
};
@@ -2316,8 +2316,14 @@ struct __unique_ptr_deleter_sfinae<_Deleter&> {
typedef false_type __enable_rval_overload;
};
+#if defined(_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI)
+# define _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __attribute__((trivial_abi))
+#else
+# define _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI
+#endif
+
template <class _Tp, class _Dp = default_delete<_Tp> >
-class _LIBCPP_TEMPLATE_VIS unique_ptr {
+class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
public:
typedef _Tp element_type;
typedef _Dp deleter_type;
@@ -2525,7 +2531,7 @@ public:
template <class _Tp, class _Dp>
-class _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp> {
+class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp> {
public:
typedef _Tp element_type;
typedef _Dp deleter_type;
@@ -3537,8 +3543,14 @@ struct __compatible_with
: is_convertible<_Tp*, _Up*> {};
#endif // _LIBCPP_STD_VER > 14
+#if defined(_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI)
+# define _LIBCPP_SHARED_PTR_TRIVIAL_ABI __attribute__((trivial_abi))
+#else
+# define _LIBCPP_SHARED_PTR_TRIVIAL_ABI
+#endif
+
template<class _Tp>
-class _LIBCPP_TEMPLATE_VIS shared_ptr
+class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
{
public:
#if _LIBCPP_STD_VER > 14
@@ -4526,7 +4538,7 @@ get_deleter(const shared_ptr<_Tp>& __p) _NOEXCEPT
#endif // _LIBCPP_NO_RTTI
template<class _Tp>
-class _LIBCPP_TEMPLATE_VIS weak_ptr
+class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS weak_ptr
{
public:
typedef _Tp element_type;
diff --git a/libcxx/test/libcxx/memory/trivial_abi/shared_ptr_arg.pass.cpp b/libcxx/test/libcxx/memory/trivial_abi/shared_ptr_arg.pass.cpp
new file mode 100644
index 000000000000..c8f6e0ce3b8d
--- /dev/null
+++ b/libcxx/test/libcxx/memory/trivial_abi/shared_ptr_arg.pass.cpp
@@ -0,0 +1,48 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Test shared_ptr<T> with trivial_abi as parameter type.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI
+
+#include <memory>
+#include <cassert>
+
+__attribute__((noinline)) void call_something() { asm volatile(""); }
+
+struct Node {
+ int* shared_val;
+
+ explicit Node(int* ptr) : shared_val(ptr) {}
+ ~Node() { ++(*shared_val); }
+};
+
+__attribute__((noinline)) bool get_val(std::shared_ptr<Node> /*unused*/) {
+ call_something();
+ return true;
+}
+
+__attribute__((noinline)) void expect_1(int* shared, bool /*unused*/) {
+ assert(*shared == 1);
+}
+
+int main(int, char**) {
+ int shared = 0;
+
+ // Without trivial-abi, the shared_ptr is deleted at the end of this
+ // statement; expect_1 will see shared == 0 because it's not incremented (in
+ // ~Node()) until expect_1 returns.
+ //
+ // With trivial-abi, expect_1 will see shared == 1 because shared_val is
+ // incremented before get_val returns.
+ expect_1(&shared, get_val(std::make_shared<Node>(&shared)));
+
+ return 0;
+}
diff --git a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_arg.pass.cpp b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_arg.pass.cpp
new file mode 100644
index 000000000000..642bfa88d578
--- /dev/null
+++ b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_arg.pass.cpp
@@ -0,0 +1,50 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Test unique_ptr<T> with trivial_abi as parameter type.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI
+
+#include <memory>
+#include <cassert>
+
+__attribute__((noinline)) void call_something() { asm volatile(""); }
+
+struct Node {
+ int* shared_val;
+
+ explicit Node(int* ptr) : shared_val(ptr) {}
+ ~Node() { ++(*shared_val); }
+};
+
+__attribute__((noinline)) bool get_val(std::unique_ptr<Node> /*unused*/) {
+ call_something();
+ return true;
+}
+
+__attribute__((noinline)) void expect_1(int* shared, bool /*unused*/) {
+ assert(*shared == 1);
+}
+
+int main(int, char**) {
+ int shared = 0;
+
+ // Without trivial-abi, the unique_ptr is deleted at the end of this
+ // statement; expect_1 will see shared == 0 because it's not incremented (in
+ // ~Node()) until expect_1 returns.
+ //
+ // With trivial-abi, expect_1 will see shared == 1 because shared_val is
+ // incremented before get_val returns.
+ expect_1(&shared, get_val(std::make_unique<Node>(&shared)));
+
+ // Check that the shared-value is still 1.
+ expect_1(&shared, true);
+ return 0;
+}
diff --git a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_array.pass.cpp b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_array.pass.cpp
new file mode 100644
index 000000000000..ef77dde6271b
--- /dev/null
+++ b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_array.pass.cpp
@@ -0,0 +1,52 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Test unique_ptr<T[]> with trivial_abi as parameter type.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI
+
+#include <memory>
+#include <cassert>
+
+__attribute__((noinline)) void call_something() { asm volatile(""); }
+
+struct Node {
+ int* shared_val;
+
+ explicit Node(int* ptr) : shared_val(ptr) {}
+ ~Node() { ++(*shared_val); }
+};
+
+__attribute__((noinline)) bool get_val(std::unique_ptr<Node[]> /*unused*/) {
+ call_something();
+ return true;
+}
+
+__attribute__((noinline)) void expect_3(int* shared, bool /*unused*/) {
+ assert(*shared == 3);
+}
+
+int main(int, char**) {
+ int shared = 0;
+
+ // Without trivial-abi, the unique_ptr is deleted at the end of this
+ // statement, expect_3 will see shared == 0 because it's not incremented (in
+ // ~Node()) until the end of this statement.
+ //
+ // With trivial-abi, shared_val is incremented 3 times before get_val returns
+ // because ~Node() was called 3 times.
+ expect_3(&shared, get_val(std::unique_ptr<Node[]>(new Node[3]{
+ Node(&shared), Node(&shared), Node(&shared)})));
+
+ // Check that shared_value is still 3 (ie., ~Node() isn't called again by the end of the full-expression above)
+ expect_3(&shared, true);
+
+ return 0;
+}
diff --git a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp
new file mode 100644
index 000000000000..46ce6adade95
--- /dev/null
+++ b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp
@@ -0,0 +1,59 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Test arguments destruction order involving unique_ptr<T> with trivial_abi.
+// Note: Unlike other tests in this directory, this is the only test that
+// exhibits a
diff erence between the two modes in Microsft ABI.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI
+
+#include <memory>
+#include <cassert>
+
+__attribute__((noinline)) void call_something() { asm volatile(""); }
+
+struct Base {
+ char* shared_buff;
+ int* cur_idx;
+ const char id;
+
+ explicit Base(char* buf, int* idx, char ch)
+ : shared_buff(buf), cur_idx(idx), id(ch) {}
+ ~Base() { shared_buff[(*cur_idx)++] = id; }
+};
+
+struct A : Base {
+ explicit A(char* buf, int* idx) : Base(buf, idx, 'A') {}
+};
+
+struct B : Base {
+ explicit B(char* buf, int* idx) : Base(buf, idx, 'B') {}
+};
+
+struct C : Base {
+ explicit C(char* buf, int* idx) : Base(buf, idx, 'C') {}
+};
+
+__attribute__((noinline)) void func(A /*unused*/, std::unique_ptr<B> /*unused*/,
+ C /*unused*/) {
+ call_something();
+}
+
+int main(int, char**) {
+ char shared_buf[3] = {'0', '0', '0'};
+ int cur_idx = 0;
+
+ func(A(shared_buf, &cur_idx), std::make_unique<B>(shared_buf, &cur_idx),
+ C(shared_buf, &cur_idx));
+
+ // With trivial_abi, the std::unique_ptr<B> arg is always destructed first.
+ assert(shared_buf[0] == 'B');
+ return 0;
+}
diff --git a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_ret.pass.cpp b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_ret.pass.cpp
new file mode 100644
index 000000000000..ba0af5b0b0ef
--- /dev/null
+++ b/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_ret.pass.cpp
@@ -0,0 +1,49 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Test unique_ptr<T> with trivial_abi as return-type.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI
+
+#include <memory>
+#include <cassert>
+
+__attribute__((noinline)) void call_something() { asm volatile(""); }
+
+struct Node {
+ explicit Node() {}
+ ~Node() {}
+};
+
+__attribute__((noinline)) std::unique_ptr<Node> make_val(void** local_addr) {
+ call_something();
+
+ auto ret = std::make_unique<Node>();
+
+ // Capture the local address of ret.
+ *local_addr = &ret;
+
+ return ret;
+}
+
+int main(int, char**) {
+ void* local_addr = nullptr;
+ auto ret = make_val(&local_addr);
+ assert(local_addr != nullptr);
+
+ // Without trivial_abi, &ret == local_addr because the return value
+ // is allocated here in main's stackframe.
+ //
+ // With trivial_abi, local_addr is the address of a local variable in
+ // make_val, and hence
diff erent from &ret.
+ assert((void*)&ret != local_addr);
+
+ return 0;
+}
diff --git a/libcxx/test/libcxx/memory/trivial_abi/weak_ptr_ret.pass.cpp b/libcxx/test/libcxx/memory/trivial_abi/weak_ptr_ret.pass.cpp
new file mode 100644
index 000000000000..fcf878612a05
--- /dev/null
+++ b/libcxx/test/libcxx/memory/trivial_abi/weak_ptr_ret.pass.cpp
@@ -0,0 +1,52 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Test weak_ptr<T> with trivial_abi as return-type.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI
+
+#include <memory>
+#include <cassert>
+
+__attribute__((noinline)) void call_something() { asm volatile(""); }
+
+struct Node {
+ explicit Node() {}
+ ~Node() {}
+};
+
+__attribute__((noinline)) std::weak_ptr<Node>
+make_val(std::shared_ptr<Node>& sptr, void** local_addr) {
+ call_something();
+
+ std::weak_ptr<Node> ret;
+ ret = sptr;
+
+ // Capture the local address of ret.
+ *local_addr = &ret;
+
+ return ret;
+}
+
+int main(int, char**) {
+ void* local_addr = nullptr;
+ auto sptr = std::make_shared<Node>();
+ std::weak_ptr<Node> ret = make_val(sptr, &local_addr);
+ assert(local_addr != nullptr);
+
+ // Without trivial_abi, &ret == local_addr because the return value
+ // is allocated here in main's stackframe.
+ //
+ // With trivial_abi, local_addr is the address of a local variable in
+ // make_val, and hence
diff erent from &ret.
+ assert((void*)&ret != local_addr);
+
+ return 0;
+}
More information about the libcxx-commits
mailing list