[libcxx-commits] [libcxx] dc13ac0 - 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
Sat Jul 4 08:31:41 PDT 2020
Author: Vy Nguyen
Date: 2020-07-04T11:29:08-04:00
New Revision: dc13ac02800220a33828ac3da629f382ca7e308d
URL: https://github.com/llvm/llvm-project/commit/dc13ac02800220a33828ac3da629f382ca7e308d
DIFF: https://github.com/llvm/llvm-project/commit/dc13ac02800220a33828ac3da629f382ca7e308d.diff
LOG: 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
Added:
Modified:
libcxx/docs/index.rst
libcxx/include/__config
libcxx/include/memory
Removed:
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
################################################################################
diff --git a/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst b/libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst
deleted file mode 100644
index a0f260a44e81..000000000000
--- 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<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 75d5b8226b4c..c60a72d49724 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
* `<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 e040a7a3e503..7e4c37431ea4 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 48069258513f..1f9f36c5bbbb 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,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 _Tp, class _Dp = default_delete<_Tp> >
-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 _Tp, class _Dp>
-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 _Tp>
-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 _Tp>
-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 cdbf779751e9..000000000000
--- 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
-//
-//===----------------------------------------------------------------------===//
-
-// <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> 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<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
deleted file mode 100644
index 3f0db82141a0..000000000000
--- 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
-//
-//===----------------------------------------------------------------------===//
-
-// <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> 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<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
deleted file mode 100644
index 415837fdd88a..000000000000
--- 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
-//
-//===----------------------------------------------------------------------===//
-
-// <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[]> 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<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
deleted file mode 100644
index 3c30e974b69d..000000000000
--- 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
-//
-//===----------------------------------------------------------------------===//
-
-// <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 a_struct, std::unique_ptr<B> 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<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
deleted file mode 100644
index ba0af5b0b0ef..000000000000
--- 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
-//
-//===----------------------------------------------------------------------===//
-
-// <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
deleted file mode 100644
index 983faf2ce22d..000000000000
--- 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
-//
-//===----------------------------------------------------------------------===//
-
-// <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>(&shared);
- 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