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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1421
+    destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+    cleanupKind = getARCCleanupKind();
+    break;
----------------
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.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:315
+    }
+  }
 }
----------------
ahatanak wrote:
> rjmccall wrote:
> > Do these functions have a memcpy as a precondition?  I would expect them to do the full copy (for code size if nothing else).
> Yes, there should always be a call to memcpy before the copy/move special functions are called.  I don't think we want to fold the call to memcpy into the body of the special functions since another special function can be called from the body if the non-trivial C struct has a field whose type is a non-trivial C struct too (in which case, there will be a redundant memcpy to copy the C struct field).
> 
> For example, in the following code, there shouldn't be a call to memcpy to copy field "f0" of StrongOuter if there is already a memcpy that copies struct StrongOuter:
> 
> ```
> typedef struct {
>   int f0;
>   id f1;
> } Strong;
> 
> typedef struct {
>   Strong f0;
>   id f1;
> } StrongOuter;
> ```
> 
Well, I guess I was imagining something more C++-ish where you don't necessarily have a struct-wide memcpy, and instead you just memcpy the parts where that's profitable and otherwise do something type-specific, which would mean recursing for a struct.  Your approach is reasonable if the non-trivial copying is relatively sparse and the structure is large; on the other hand, if the non-trivial copying is dense, the memcpy itself might be mostly redundant.  And it does mean a bigger code-size hit in the original place that kicks off the copy.

IIRC we do already have some code in copy-constructor emission that's capable of emitting sequences of field copies with a memcpy, if you wanted to try the C++ style, you could probably take advantage of that pretty easily.  But I won't hold up the patch if you think the memcpy precondition is the right way to go.


================
Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}
----------------
ahatanak wrote:
> rjmccall wrote:
> > There's no need to put this in an anonymous namespace.
> > 
> > This enum makes me wonder if it would make more sense to create equivalents to QualType::DestructionKind for each of these operations and all of the different primitive types.  That would let you e.g. implement your only-strong-members check above much more easily, and it would also make it simpler to guarantee that all the places that need to exhaustively enumerate the reasons why something might need special initialization/copying logic don't miss an important new case.
> I'm not sure if I understood your point here. Do you mean there should be DestructionKinds for arrays or structs too or merge these enums into DestructionKind?
I was suggesting there should be equivalents to DestructionKind for some of the other operations, like a NonTrivialCopyKind, and queries on QualType analogous to isDestructedType(), e.g. isNonTrivialToCopy().  That way you can directly put this stuff in the AST, and then here you can just walk the struct fields and switch on the enum, at which point I think you don't need FieldKind at all.  Remember that some types are non-trivial in only a subset of these dimensions.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1144
+    // indirectly, the callee is responsible for destructing the argument.
+    if (Ty.hasNonTrivialToDestroyStruct()) {
+      AutoVarEmission Emission(*VD);
----------------
ahatanak wrote:
> rjmccall wrote:
> > You're not actually checking the "is not passed indirectly" part of this, but I think that's okay, because I don't think we actually want the ownership aspects of the ABI to vary based on how the argument is passed.  So you should just strike that part from the comment.
> > 
> > Also, this should really be done in EmitParmDecl.
> To make ARC and ARC++ compatible, I think we'll have to change the ABI of C++ functions that are passed structs containing weak fields if we want to destruct non-trivial C arguments destructed in the callee. I guess that's OK since we have to break the ABI for structs containing strong fields too.
I hadn't been thinking of __weak, just the difference between small and large structs, but this is a good point.  I think I agree with you, we should also change the destruction ABI for structs containing __weak fields; basically, the rule will be that only C++ destruction (under the Itanium ABI) requires destruction in the caller.


================
Comment at: lib/CodeGen/CodeGenFunction.h:3347
+                                  QualType QT, bool IsVolatile);
+  void callDestructor(Address DstPtr, QualType QT, bool IsVolatile);
+
----------------
ahatanak wrote:
> rjmccall wrote:
> > These methods should *definitely* be somehow namespaced for your new feature.
> I renamed these methods to make it clear that they are calling special functions for C structs. Is that what you mean by "namespaced"?
Yes, thank you.


https://reviews.llvm.org/D41228