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

[Devin Jeanpierre via Phabricator via cfe-commits]

devin.jeanpierre added a comment.

CI test finished successfully before windows setup did 😢. My workplace's Windows VMs are a bit hosed at the moment...

In D114732#3253416 <https://reviews.llvm.org/D114732#3253416>, @Quuxplusone wrote:

> 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.

Fair, I'm sorry for missing that detail.

>> 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.)

Well, we must do something. In particular, consider this code:

  // class to make it non-POD for the purpose of layout
  class X { int64_t a; int8_t b;};
  class Y : public X {int8_t c;};
  
  Y y1 = ...;
  Y y2 = ....;
  X& x1 = y1;
  X& x2 = y2;
  
  std::swap(x1, x2);

Here, at least in some implementations, `c` will live inside the tail padding of `X`. Without any special guarding, if `swap` used a `memcpy` with `sizeof(x1)` etc., then it would overwrite the padding bytes, which include `z`. I don't think swap should do this.

>> 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

Unfortunately, even if `T` is final, it can have things in its tail padding: `[[no_unique_address]]` allows all the same optimizations as a base class, including reuse of tail padding and ECBO. See e.g. the cppreference page <https://en.cppreference.com/w/cpp/language/attributes/no_unique_address>.

The reason your godbolt link shows them having the same size is that the `A` struct is "POD for the purpose of layout". In the Itanium ABI, POD types are laid out as normal, but their tail padding isn't reused in any circumstances (not for subclasses either -- so try e.g. removing final and using inheritance, and the assertion still passes: https://godbolt.org/z/MWrMoEbvb). If you make that a `class` instead of a `struct`, it isn't POD for the purpose of layout (due to private members), and the assertion fails now that the tail padding can be reused: https://godbolt.org/z/5edfqrK47

However, I think you're right that this isn't enough -- I don't believe it's guaranteed that inheritance and `[[no_unique_address]]` use padding in the exact same way, so you probably need to try both just to be safe.

> 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.

I don't think it matters if they're polymorphic or not -- swapping the derived class's member variables when you're swapping the base class subobject (or a `[[no_unique_address]]` member) is unexpected and an observable change in behavior.

>> (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.

Yeah, I'm probably going to want to do that later, FWIW.



================
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>), "");
----------------
Quuxplusone wrote:
> (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.
> (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.
Sorry, I get that confused a lot. :)

> (1) Why should Windows be different from everyone else here?
In this change so far, an object is only considered trivially relocatable if it's trivial for calls, and Windows is non-conforming wrt what it considers trivial for calls: it won't consider something trivial for calls if it isn't trivially copyable.

Code: https://clang.llvm.org/doxygen/SemaDeclCXX_8cpp_source.html#l06558

It surprised me too. This is a good motivator IMO to support something like your proposal, and have types be trivially relocatable that aren't trivial for calls. This allows, as you mention elsewhere, for optimizations that aren't ABI-breaking, and for e.g. non-conforming platforms like this to still take advantage of trivial relocatability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732