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

[Louis Dionne via Phabricator via libcxx-commits]

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.
+#  ifdef _LIBCPP_ABI_ENABLE_UNIQUE_PTR_WITH_TRIVIAL_ABI
+#    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> >
class 
#if defined(_LIBCPP_ABI_UNIQUE_PTR_TRIVIAL)
    __attribute__((trivial_abi))
#endif
_LIBCPP_TEMPLATE_VIS unique_ptr {
...
```

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82490/new/

https://reviews.llvm.org/D82490