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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1423
+      if (capturedByInit)
+        drillIntoBlockVariable(*this, lvalue, cast<VarDecl>(D));
+
----------------
rjmccall wrote:
> 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.
Ah I see. I'll see if I can implement the first solution you suggested in a separate patch. If it turns out that doing so breaks too much code, we can consider implementing the second solution.

Is this something users do commonly?



================
Comment at: lib/CodeGen/CGExprAgg.cpp:255
+      Ty.isDestructedType() == QualType::DK_nontrivial_c_struct)
+    CGF.pushDestroy(Ty.isDestructedType(), src.getAggregateAddress(), Ty);
+
----------------
rjmccall wrote:
> 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?
Yes, this is specific to ignored results. This is needed because otherwise function return values that are ignored won't get destructed by the caller.


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+
+    TrivialFieldIsVolatile |= FT.isVolatileQualified();
+    if (Start == End)
----------------
rjmccall wrote:
> 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.
I added method visitVolatileTrivial that copies volatile fields individually. Please see test case test_copy_constructor_Bitfield1 in test/CodeGenObjC/strong-in-c-struct.m.


https://reviews.llvm.org/D41228