From dc13ac02800220a33828ac3da629f382ca7e308d Mon Sep 17 00:00:00 2001 From: Vy Nguyen Date: Sat, 4 Jul 2020 11:29:08 -0400 Subject: [PATCH] Revert "[libcxx] Put clang::trivial_abi on std::unique_ptr, std::shared_ptr, and std::weak_ptr" This reverts commit 5cde3c9633fd071c90e9f9ce54a002e78fdd9df9. The tests were reported failing on clang10 --- libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst | 149 --------------------- libcxx/docs/index.rst | 1 - libcxx/include/__config | 4 - libcxx/include/memory | 22 +-- .../memory/trivial_abi/shared_ptr_arg.pass.cpp | 48 ------- .../memory/trivial_abi/unique_ptr_arg.pass.cpp | 50 ------- .../memory/trivial_abi/unique_ptr_array.pass.cpp | 52 ------- .../unique_ptr_destruction_order.pass.cpp | 59 -------- .../memory/trivial_abi/unique_ptr_ret.pass.cpp | 49 ------- .../memory/trivial_abi/weak_ptr_ret.pass.cpp | 52 ------- 10 files changed, 5 insertions(+), 481 deletions(-) delete mode 100644 libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst delete mode 100644 libcxx/test/libcxx/memory/trivial_abi/shared_ptr_arg.pass.cpp delete mode 100644 libcxx/test/libcxx/memory/trivial_abi/unique_ptr_arg.pass.cpp delete mode 100644 libcxx/test/libcxx/memory/trivial_abi/unique_ptr_array.pass.cpp delete mode 100644 libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp delete mode 100644 libcxx/test/libcxx/memory/trivial_abi/unique_ptr_ret.pass.cpp delete mode 100644 libcxx/test/libcxx/memory/trivial_abi/weak_ptr_ret.pass.cpp diff --git a/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst b/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst deleted file mode 100644 index a0f260a..0000000 --- a/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst +++ /dev/null @@ -1,149 +0,0 @@ -============================================= -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 smart_arg) { ... } - - Foo* raw_ptr_retval() { ... } - std::unique_ptr 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, A) {} }; - C c{{}, make_unique, {}}; - - - 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 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(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``. The following code will no longer compile. - - .. code-block:: cpp - - class Foo; - void func(std::unique_ptr 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); - void func() { - std::unique_ptr 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))); - ^ - // <<`` ends earlier. - - Spot the bug: - - .. code-block:: cpp - - std::unique_ptr create_and_subscribe(Bar* subscriber) { - auto foo = std::make_unique(); - 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 75d5b82..c60a72d 100644 --- a/libcxx/docs/index.rst +++ b/libcxx/docs/index.rst @@ -164,7 +164,6 @@ Design Documents DesignDocs/FileTimeType DesignDocs/FeatureTestMacros DesignDocs/ExtendedCXX03Support - DesignDocs/UniquePtrTrivialAbi * ` design `_ * ` design `_ diff --git a/libcxx/include/__config b/libcxx/include/__config index e040a7a..7e4c3743 100644 --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -105,10 +105,6 @@ // 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 4806925..1f9f36c 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 void reset(U) = delete; + template void reset(U) = delete; void swap(unique_ptr& u) noexcept; }; @@ -2316,14 +2316,8 @@ 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 __atribute__((trivial_abi)) -#else -# define _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI -#endif - template > -class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr { +class _LIBCPP_TEMPLATE_VIS unique_ptr { public: typedef _Tp element_type; typedef _Dp deleter_type; @@ -2531,7 +2525,7 @@ public: template -class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp> { +class _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp> { public: typedef _Tp element_type; typedef _Dp deleter_type; @@ -3543,14 +3537,8 @@ 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 _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr +class _LIBCPP_TEMPLATE_VIS shared_ptr { public: #if _LIBCPP_STD_VER > 14 @@ -4538,7 +4526,7 @@ get_deleter(const shared_ptr<_Tp>& __p) _NOEXCEPT #endif // _LIBCPP_NO_RTTI template -class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS weak_ptr +class _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 deleted file mode 100644 index cdbf779..0000000 --- a/libcxx/test/libcxx/memory/trivial_abi/shared_ptr_arg.pass.cpp +++ /dev/null @@ -1,48 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// - -// Test shared_ptr with trivial_abi as parameter type. - -// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI - -#include -#include - -__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) { - 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(&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 deleted file mode 100644 index 3f0db82..0000000 --- a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_arg.pass.cpp +++ /dev/null @@ -1,50 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// - -// Test unique_ptr with trivial_abi as parameter type. - -// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI - -#include -#include - -__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) { - 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(&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 deleted file mode 100644 index 415837f..0000000 --- a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_array.pass.cpp +++ /dev/null @@ -1,52 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// - -// Test unique_ptr with trivial_abi as parameter type. - -// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI - -#include -#include - -__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) { - 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(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 deleted file mode 100644 index 3c30e97..0000000 --- a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_destruction_order.pass.cpp +++ /dev/null @@ -1,59 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// - -// Test arguments destruction order involving unique_ptr with trivial_abi. -// Note: Unlike other tests in this directory, this is the only test that -// exhibits a difference between the two modes in Microsft ABI. - -// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI - -#include -#include - -__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 a_struct, std::unique_ptr b, - C c_struct) { - 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(shared_buf, &cur_idx), - C(shared_buf, &cur_idx)); - - // With trivial_abi, the std::unique_ptr 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 deleted file mode 100644 index ba0af5b..0000000 --- a/libcxx/test/libcxx/memory/trivial_abi/unique_ptr_ret.pass.cpp +++ /dev/null @@ -1,49 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// - -// Test unique_ptr with trivial_abi as return-type. - -// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI - -#include -#include - -__attribute__((noinline)) void call_something() { asm volatile(""); } - -struct Node { - explicit Node() {} - ~Node() {} -}; - -__attribute__((noinline)) std::unique_ptr make_val(void** local_addr) { - call_something(); - - auto ret = std::make_unique(); - - // 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 different 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 deleted file mode 100644 index 983faf2..0000000 --- a/libcxx/test/libcxx/memory/trivial_abi/weak_ptr_ret.pass.cpp +++ /dev/null @@ -1,52 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// - -// Test weak_ptr with trivial_abi as return-type. - -// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI - -#include -#include - -__attribute__((noinline)) void call_something() { asm volatile(""); } - -struct Node { - explicit Node() {} - ~Node() {} -}; - -__attribute__((noinline)) std::weak_ptr -make_val(std::shared_ptr& sptr, void** local_addr) { - call_something(); - - std::weak_ptr 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(&shared); - std::weak_ptr 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 different from &ret. - assert((void*)&ret != local_addr); - - return 0; -} -- 2.7.4