[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/Decl.h:3619
+    return NonTrivialToPrimitiveDestructiveMove;
+  }
+
----------------
Please document that this means a C++-style destructive move that leaves the source object in a validly-initialized state.


================
Comment at: include/clang/AST/Type.h:1094
+    PCK_Trivial, // a field of a trivial type.
+    PCK_Strong,  // objc strong pointer.
+    PCK_Struct   // non-trivial C struct.
----------------
Please call this ARCStrong, it'll be much clearer (and easier to visually distinguish from "struct').


================
Comment at: include/clang/AST/Type.h:1096
+    PCK_Struct   // non-trivial C struct.
+  };
+
----------------
Nit-pick: please declare this with the methods that use it.


================
Comment at: lib/AST/ASTContext.cpp:5795
+      // These cases should have been taken care of when checking the type's
+      // non-triviality
       case Qualifiers::OCL_Weak:
----------------
Nit-pick: period.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1770
   if (T->isBlockPointerType())
     Flags = BLOCK_FIELD_IS_BLOCK;
 
----------------
Should we just have a helper function like `getBlockFieldFlagsForObjCObjectPointer(QualType);`?  You could just call it from all the object-specific places instead of having this Flags variable that's not valid in half the cases.


================
Comment at: lib/CodeGen/CGDecl.cpp:1421
+    destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+    cleanupKind = getARCCleanupKind();
+    break;
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > This can only be getARCCleanupKind() if it's non-trivial to destroy solely because of __strong members.  Since the setup here is significantly more general than ARC, I think you need to default to the more-correct behavior; if you want to add a special-case check for only __strong members, you ought to do that explicitly.
> > I added a DestructionKind (DK_c_struct_strong_field) that is used just for structs that have strong fields (but doesn't have weak fields).
> Sure, that's a simple enough way to do it, and I think it's fairly warranted.
Okay, since you don't have a DK_c_struct_strong_field anymore, you either need to stop using getARCCleanupKind() or do the test for just __strong fields.


================
Comment at: lib/CodeGen/CGDecl.cpp:1423
+      if (capturedByInit)
+        drillIntoBlockVariable(*this, lvalue, cast<VarDecl>(D));
+
----------------
The "delay" is that we generally handle `__block` captures within initializers by delaying the drill-into until we're ready to actually store into the variable.  Not delaying has the advantage of allowing us to just project out the stack storage instead of actually chasing the forwarding pointer, but  this isn't safe if the forwarding pointer might no longer point to the stack storage, which is what happens when a block capturing a `__block` variable is copied to the heap.

Delaying the drill-into is easy when the stores can be completely separated from the initializer, as is true for scalar and complex types.  It's not easy for aggregates because we need to emit the initializer in place, but it's possible that a block capturing the `__block` variable will be copied during the emission of that initializer (e.g. during a later element of a braced-init-list or within a C++ constructor call), meaning that we might be copying an object that's only partly initialized.  This is particularly dangerous if there are destructors involved.

Probably the best solution is to forbid capturing `__block` variables of aggregate type within their own initializer.  It's possible that that might break existing code that's not doing the specific dangerous things, though.

The more complex solution is to make IRGen force the `__block` variable to the heap before actually initializing it.  (It can do this by directly calling `_Block_object_assign`.) We would then initialize the heap copy in-place, safely knowing that there is no possibility that it will move again.  Effectively, the stack copy of the variable would never exist as an object; only its header would matter, and only enough to allow the runtime to "copy" it to the heap.  We'd have to hack the __block variable's copy helper to not try to call the move-constructor, and we'd need to suppress the cleanup that destroys the stack copy of the variable.  We already push a cleanup to release the `__block` variable when it goes out of scope, and that would stay.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:82
+  void EmitFinalDestCopy(QualType type, const LValue &src,
+                         bool SrcIsRValue = false);
   void EmitFinalDestCopy(QualType type, RValue src);
----------------
Please use a boolean enum for this instead of a raw bool.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:255
+      Ty.isDestructedType() == QualType::DK_nontrivial_c_struct)
+    CGF.pushDestroy(Ty.isDestructedType(), src.getAggregateAddress(), Ty);
+
----------------
Hrm.  Okay, I guess.  This is specific to ignored results because otherwise we assume that the caller is going to manage the lifetime of the object?


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:63
+template <class Derived, class RetTy = void>
+struct DefaultInitializeTypeVisitor {
+  template <class... Ts> RetTy visit(QualType FT, Ts &&... Args) {
----------------
DefaultInitialize*d*TypeVisitor would be better, I think.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:77
+    case QualType::PDIK_Strong:
+      asDerived().visitStrong(FT, std::forward<Ts>(Args)...);
+      break;
----------------
Use returns, please.  And you can add an llvm_unreachable after the switch (in all of the visitors).


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:92
+template <class Derived, bool IsMove, class RetTy = void>
+struct CopyTypeVisitor {
+  template <class... Ts> RetTy visit(QualType FT, Ts &&... Args) {
----------------
CopiedTypeVisitor, I guess.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:138
+      QualType FT = FD->getType();
+      unsigned FStartInBits = RL.getFieldOffset(FieldNo++);
+      unsigned FEndInBits = FStartInBits + getFieldSize(FD, Ctx);
----------------
These computations should be in uint64_t.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:143
+      CharUnits FStart =
+          Offset + CharUnits::fromQuantity(FStartInBits / Ctx.getCharWidth());
+      // Compute one past the last byte of the field.
----------------
ASTContext has a toCharUnitsFromBits method.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:184
+    if (PCK)
+      StructVisitor<Derived>::asDerived().flushTrivialFields(
+          std::forward<Ts>(Args)...);
----------------
You can add `using StructVisitor<Derived>::asDerived;` to the class to avoid having to write this a bunch.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+
+    TrivialFieldIsVolatile |= FT.isVolatileQualified();
+    if (Start == End)
----------------
I feel like maybe volatile fields should be individually copied instead of being aggregated into a multi-field memcpy.  This is a more natural interpretation of the C volatile rules than we currently do.  In fact, arguably we should really add a PrimitiveCopyKind enumerator for volatile fields (that are otherwise trivially-copyable) and force all copies of structs with volatile fields into this path precisely so that we can make a point of copying the volatile fields this way.  (Obviously that part is not something that's your responsibility to do.)

To get that right with bit-fields, you'll need to propagate the actual FieldDecl down.  On the plus side, that should let you use EmitLValueForField to do the field projection in the common case.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:341
+
+// Helper function that creates CGFunctionInfo for an N-aray special function.
+template <size_t N>
----------------
"N-ary" is the usual spelling.


https://reviews.llvm.org/D41228