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

[Devin Jeanpierre via Phabricator via cfe-commits]

devin.jeanpierre added a comment.

(Sorry for delayed reply -- I made the mistake of signing up to phabricator with my personal email, which I don't check very well, apparently!)



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215
+The compiler will pass and return a trivially relocatable type using the C ABI
+for the underlying type, even when the type would otherwise be considered
+non-trivially-relocatable. If a type is trivially relocatable, has a
+non-trivial destructor, and is passed as an argument by value, the convention
+is that the callee will destroy the object before returning.
----------------
rsmith wrote:
> I think this documentation change has mixed together two different things. The revised wording says that `[[trivial_abi]]` implies trivially-relocatable and that trivially-relocatable implies passing using the C ABI, which is wrong: the second implication does not hold. What we should say is that `[[trivial_abi]]` (if we're not in the "has no effect" case described below) implies both that the type is trivially-relocatable, and that it is passed using the C ABI (that is, that we trivially relocate in argument passing).
> 
> Instead of the wording changes you have here, perhaps we could leave the old wording alone and add a paragraph that says that a type with the attribute is assumed to be trivially-relocatable for the purpose of `__is_trivially_relocatable`, but that Clang doesn't yet take advantage of this fact anywhere other than argument passing.
Yeah, this was too aggressive a wording change, and might easily change later when `[[trivially_relocatable]]` is added.

I did drop the "Clang doesn't yet take advantage of this fact anywhere other than argument passing.", because I feel like -- does that sort of sentiment belong instead in the docs for `__is_trivially_relocatable`? Maybe I should change `__is_trivially_relocatable` to note that this is never taken advantage of anywhere except in by-value argument passing (when the type is trivial for the purpose of calls) and library code.


================
Comment at: clang/lib/AST/Type.cpp:2500
+    return BaseElementType.isTriviallyCopyableType(Context) &&
+           !BaseElementType.isDestructedType();
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732