[libcxx-commits] [PATCH] D82490: [libcxx] Put clang::trivial_abi on std::unique_ptr

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Jun 25 13:39:15 PDT 2020

ldionne requested changes to this revision.
ldionne marked 2 inline comments as done.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: dexonsmith.

Thanks a lot for the great document explaining the change.

I agree with @rsmith that we should add a few tests for this change. They should go in `libcxx/test/libcxx` because they are libc++ specific, not mandated by the Standard.

Can you also handle `shared_ptr` and `weak_ptr` in this patch? You can add different knobs for `unique_ptr` and `shared_ptr`/`weak_ptr` if that makes your life easier, but that would allow considering this patch as a strict superset of https://reviews.llvm.org/D63748.

Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:105
+ - Function definitions now require complete type ``T`` for parameters with type ``std::unique_ptr<T>``. The following code will no longer compile.
I think that's fine, since the Standard doesn't say whether `unique_ptr` should be destroyed in the callee or in the caller. All it says is that when a `unique_ptr<T, std::default_delete<T>>` is destroyed, `T` needs to be complete type.

Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:127
+        // making this an access to freed memory.
+        owned_foo->Bar(run_worker(std::move(smart_foo)));
+                  ^
I'll argue this is incredibly tricky -- you're using a pointer managed by a moved-from `unique_ptr`. I think it's reasonable to break this, actually it would be fine if it never worked in the first place.

Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:132
+ - Lifetime of local `returned` ``std::unique_ptr<>`` ends earlier.
Are you missing backticks around `returned`?

Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:145
+   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)
Nit: Multiple spaces between `not` and `yield`.

Comment at: libcxx/include/__config:109
+// Enable clang::trivial_abi on std::unique_ptr.
+#    define _LIBCPP_ABI_UNIQUE_PTR_TRIVIAL __attribute__((trivial_abi))
I would prefer if that `#define` was just a straight macro definition without any expansion. That would be more consistent with how we deal with other ABI macros.

What you could then do is say in `unique_ptr`:

template <class _Tp, class _Dp = default_delete<_Tp> >

Or something equivalent. But ideally, the knob for turning on that optimization should just be to `#define` a macro to nothing.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list