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

[Devin Jeanpierre via Phabricator via cfe-commits]

devin.jeanpierre added a comment.

(Sorry, I think I'm doing threading wrong here due to lack of experience with phabricator. The reply buttons are grayed out!)

> 1. All trivial-abi types are in fact trivially relocated, specifically when they are passed to functions.
> 2. Therefore, all trivial-abi types are "trivially relocatable" in general.

This sounds mostly good so far. For example:

  class [[clang::trivial_abi]] MyType { ... };
  
  MyType Create() {
    MyType x;
    /* do stuff */
    return x;
  }
  
  void Consume(MyType) {}
  int main() {
    Consume(Create());
  }

Here, you can place `x` into any valid state for a `MyType`, and it will be trivially relocated in the call to `Consume`, without any intermediate steps, thanks to guaranteed copy elision. If it is not safe to trivially relocate in general, then this is not a sound use of `trivial_abi`.

So I think some version of 2 is correct: **safe** use of `[[clang::trivial_abi]]` requires that it be safe to relocate for all publicly reachable object states.

(This isn't //watertight//: perhaps a non-publicly reachable state, only achieved temporarily internally, is not trivially relocatable. Then such a type would still be safe to use via its public API, but not be trivially relocatable in general, and very dangerous to use within its private API. By adding the optimizations, we'd add more footguns to such a type inside these sections of code.)

> 3. Therefore, all trivial-abi types are safe to optimize in all the ways depicted in D67524 <https://reviews.llvm.org/D67524>.

This is not //quite// right. It might be safe to relocate, in that adding a new relocation operation does not change program behavior, but it doesn't quite follow that it's safe to stop calling the move constructor and destructor. So e.g. some type which relies on exactly paired counts of move constructors / destructors might get upset.

Also, there is that parenthetical above: if a type can sometimes reach states //internally// that are not trivially relocatable, and in addition it calls routines like `swap` etc. while in such a state, then a relocation optimization would be unsound.

Nonetheless, my hope is that these can be considered contrived or unsound cases which we will not support for `trivial_abi`.



================
Comment at: clang/lib/AST/Type.cpp:2500
+    return BaseElementType.isTriviallyCopyableType(Context) &&
+           !BaseElementType.isDestructedType();
+  }
----------------
rsmith wrote:
> devin.jeanpierre wrote:
> > rsmith wrote:
> > > You can simplify this a little by dropping this `isDestructedType()` check; trivially copyable implies trivially destructible.
> > > 
> > > It would be a little more precise to check for `!isNonTrivialToPrimitiveDestructiveMove() && !isDestructedType()` here; that'd treat `__autoreleasing` pointers as trivially-relocatable, but not `__strong` pointers (because "destructive move" there actually means "non-destructive move" and needs to leave the object in a valid but unspecified state, which means nulling out a moved-from `__strong` pointer). I think getting this "fully" right would require a bit more plumbing to properly handle the case of a `struct` containing a `__strong` pointer (which is trivially relocatable but not trivially anything else).
> > Oooh. `isNonTrivialToPrimitiveDestructiveMove` is perfect.
> > 
> > Actually, a struct containing a `__strong` pointer is already handled -- Clang [already](https://github.com/llvm/llvm-project/blob/603c1a62f8595f70c3e96ecbec8976d411c0cc08/clang/lib/AST/DeclCXX.cpp#L1037-L1059) ensures they're passed in registers, and does all the appropriate logic. It's just `__strong` pointers themselves that I'd need to handle the same way here. Unfortunately, I really, *really* don't understand the source code dealing with `__strong` pointers in structs. (In particular, I don't understand the difference between GC::Strong and ObjCLifetime::OCL_Strong, plus I'm uncomfy with the global language opts.)
> > 
> > It's tempting to use the return value of `isNonTrivialToPrimitiveDestructiveMove` and check for `Strong` -- but now that's some *third* thing, and not mirroring the logic of when it's in a struct as a data member.
> The existing handling only covers Objective-C++ and not (non-C++) Objective-C (the C and C++ codepaths for computing struct properties are entirely different, because the C++ side of things is primarily working out properties of special member functions). It looks like your test coverage doesn't cover the non-C++ side of this.
I //think// this is fine. The keyword `__is_trivially_relocatable` only works in (Objective) C++ source files (KEYCXX), and so in order for it to be callable, the struct must have been defined in a C++ source file to begin with, right? At least, assuming ObjC works the same as C -- where the struct is defined in a header that's interpreted as (objective) C++ when it's used by (objective) C++ code. I might not be understanding any number of things, though. What would the test you're imagining look like?


================
Comment at: clang/lib/AST/Type.cpp:2500
+    return BaseElementType.isTriviallyCopyableType(Context) &&
+           !BaseElementType.isDestructedType();
+  }
----------------
devin.jeanpierre wrote:
> rsmith wrote:
> > devin.jeanpierre wrote:
> > > rsmith wrote:
> > > > You can simplify this a little by dropping this `isDestructedType()` check; trivially copyable implies trivially destructible.
> > > > 
> > > > It would be a little more precise to check for `!isNonTrivialToPrimitiveDestructiveMove() && !isDestructedType()` here; that'd treat `__autoreleasing` pointers as trivially-relocatable, but not `__strong` pointers (because "destructive move" there actually means "non-destructive move" and needs to leave the object in a valid but unspecified state, which means nulling out a moved-from `__strong` pointer). I think getting this "fully" right would require a bit more plumbing to properly handle the case of a `struct` containing a `__strong` pointer (which is trivially relocatable but not trivially anything else).
> > > Oooh. `isNonTrivialToPrimitiveDestructiveMove` is perfect.
> > > 
> > > Actually, a struct containing a `__strong` pointer is already handled -- Clang [already](https://github.com/llvm/llvm-project/blob/603c1a62f8595f70c3e96ecbec8976d411c0cc08/clang/lib/AST/DeclCXX.cpp#L1037-L1059) ensures they're passed in registers, and does all the appropriate logic. It's just `__strong` pointers themselves that I'd need to handle the same way here. Unfortunately, I really, *really* don't understand the source code dealing with `__strong` pointers in structs. (In particular, I don't understand the difference between GC::Strong and ObjCLifetime::OCL_Strong, plus I'm uncomfy with the global language opts.)
> > > 
> > > It's tempting to use the return value of `isNonTrivialToPrimitiveDestructiveMove` and check for `Strong` -- but now that's some *third* thing, and not mirroring the logic of when it's in a struct as a data member.
> > The existing handling only covers Objective-C++ and not (non-C++) Objective-C (the C and C++ codepaths for computing struct properties are entirely different, because the C++ side of things is primarily working out properties of special member functions). It looks like your test coverage doesn't cover the non-C++ side of this.
> I //think// this is fine. The keyword `__is_trivially_relocatable` only works in (Objective) C++ source files (KEYCXX), and so in order for it to be callable, the struct must have been defined in a C++ source file to begin with, right? At least, assuming ObjC works the same as C -- where the struct is defined in a header that's interpreted as (objective) C++ when it's used by (objective) C++ code. I might not be understanding any number of things, though. What would the test you're imagining look like?
I switched to checking `PCK_ARCStrong` after all, since this is set by checking for `Qualifiers::OCL_Strong`, which is exactly the same as what sets structs to be passed in registers. (if we ignore the rest of the logic (and things like `GC::Strong`)). Thanks for the reassurance, @rjmccall !

That said, this still might not be what you would write if you were doing it, so let me know if this looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732