[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

Tue Jan 18 19:05:36 PST 2022

Quuxplusone added a comment.

In D114732#3253142 <https://reviews.llvm.org/D114732#3253142>, @devin.jeanpierre wrote:

> OK, while I'm struggling to set up a new Windows machine so I can make sure this works on Windows...  @Quuxplusone, after this is merged, do you want to rebase D67524 <https://reviews.llvm.org/D67524> on top of this, or should I? I can review it -- I think when I looked at it, I only had two ideas for changes:
>
> 1. May need to guard all of these optimizations on the allocator being the standard allocator, since otherwise the change in the number of constructor/destructor calls would be observable to more than just the annotated type.

My intent is that the type-traits `__has_trivial_construct` and `__has_trivial_destroy` already handle that. The standard `std::allocator` has a trivial `construct` and `destroy`, but so do lots of user-defined allocators.

> 2. changing `std::swap` to correctly handle potentially-overlapping-objects. My thought is we could test that there's no reusable tail padding.
>
> First draft: `has_unique_object_representations` is conservative -- on the Itanium ABI, "POD for the purposes of layout" types can have padding which isn't reused when it's a potentially overlapping subobject.

I'd //like// something that triggers successfully even when there are internal padding bytes, e.g. `struct TR { int x; std::string y; }`. (But anything is better than nothing. I'm going to keep maintaining my branch for the foreseeable future, so I can keep doing my thing no matter what half-measure makes it into trunk.)

> Second draft: check by hand:
>
>       
>   struct TestStruct {
>     [[no_unique_address]] T x;
>     // not sure if this needs to be a bitfield or anything like that, but the idea is this.
>     char extra_byte;
>   };
>   bool has_padding = sizeof(TestStruct) == sizeof(T);

If this works, then either my mental model of `[[no_unique_address]]` is wrong or my mental model of tail padding is wrong. I would expect more like

  template<class T> struct DerivedFrom : public T { char extra_byte; };
  template<class T> requires is_final_v<T> struct DerivedFrom<T> { char x[sizeof(T)+1]; }; // definitely no objects in the tail padding if it's final
  bool has_padding = sizeof(DerivedFrom<T>) == sizeof(T);

If `T` is final, then it can't have stuff in its tail padding, I think: https://godbolt.org/z/69sjf15MY

Anyway, that seems like a reasonable path in the name of getting something merged. My //personal// druthers is that the Standard should just admit that `std::swap` was never meant to work on polymorphic objects, so therefore it can assume it's never swapping `Derived` objects via `Base&`, so therefore it can assume tail padding is not an issue, so therefore no metaprogramming is needed and it can just always do the efficient thing (which is therefore what I've implemented in D67524 <https://reviews.llvm.org/D67524>). But I know that's unlikely to ever happen.

> (D63620 <https://reviews.llvm.org/D63620> could in some form also be ported over, but it needs to be guarded behind ABI stability, since `[[clang::trivial_abi]]` is an ABI breaking change. For example, the same way it was done for unique_ptr <https://libcxx.llvm.org//DesignDocs/UniquePtrTrivialAbi.html>, with the same benefits.)

The point of `[[trivially_relocatable]]` is that it doesn't change the ABI (and therefore also doesn't cause the lifetime-related pitfalls described on that web page). So I won't attempt to merge it with `[[trivial_abi]]`, and I'm also //personally// uninterested in pursuing a `[[trivial_abi]] std::string` etc., because that gives no benefits to people stuck on libc++ ABIv1 (which is everyone, AFAICT). But if someone //wants// to implement `_LIBCPP_ABI_ENABLE_STRING_TRIVIAL_ABI`, `_LIBCPP_ABI_ENABLE_VECTOR_TRIVIAL_ABI`, etc. etc., for the benefit of ABIv2 users... hey, go for it.

================
Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:9-11
+// On Windows, trivial relocatability depends only on a trivial copy constructor existing.
+// In this case, it is implicitly deleted. Similar concerns apply to later tests.
+static_assert(!__is_trivially_relocatable(a<int>), "");
----------------
(1) Why should Windows be different from everyone else here?
(2) AFAIK, a user-defined move ctor means the copy ctor is //not implicitly declared//, but it's not //deleted//; so I think this comment is slightly wrong.

================
Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:123-127
+#ifdef _WIN32
+static_assert(!__is_trivially_relocatable(CopyDeleted), "");
+#else
+static_assert(__is_trivially_relocatable(CopyDeleted), "");
+#endif
----------------
Again, I don't see why Windows should be any different. `CopyDeleted` is trivial to move and trivial to destroy; that means (by definition) it's trivial to relocate.

================
Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:146
+#ifdef _WIN32
+static_assert(!__is_trivially_relocatable(CopyDeleted), "");
+#else
----------------
I think you meant `s/CopyDeleted/S20/` here.

================
Comment at: clang/test/SemaCXX/type-traits.cpp:2862
+static_assert(__is_trivially_relocatable(int), "");
+static_assert(__is_trivially_relocatable(int[]), "");
+
----------------
It's mildly surprising that an incomplete type can be trivially relocatable; but it doesn't really matter, since nobody can be depending on this answer. (`int[]` is not movable or destructible, so it's not relocatable — never mind //trivially// relocatable.)

================
Comment at: clang/test/SemaObjCXX/objc-weak-type-traits.mm:213-214
+// __is_trivially_relocatable
+TRAIT_IS_TRUE(__is_trivially_relocatable, __strong id);
+TRAIT_IS_FALSE(__is_trivially_relocatable, __weak id);
+TRAIT_IS_TRUE(__is_trivially_relocatable, __autoreleasing id);
----------------
IIUC (which I probably don't): Both `__strong id` and `__weak id` //are// trivially relocatable in the p1144 sense, but only `__strong id` is trivial-for-purposes-of-ABI, and that's why only `__strong id` is being caught here. Yes/no?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732