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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
