[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 21 13:30:20 PST 2018
rjmccall added inline comments.
================
Comment at: include/clang/AST/Type.h:1108
+ PCK_ARCStrong, // objc strong pointer.
+ PCK_Struct // non-trivial C struct.
+ };
----------------
These should all be /// documentation comments, and they mostly shouldn't talk about fields since this is a query on QualType, not FieldDecl. I would suggest something like:
for Trivial - The type does not fall into any of the following categories. Note that this case is zero-valued so that values of this enum can be used as a boolean condition for non-triviality.
for VolatileTrivial - The type would be trivial except that it is volatile-qualified. Types that fall into one of the other non-trivial cases may additionally be volatile-qualified.
for ARCStrong - The type is an Objective-C retainable pointer type that is qualified with the ARC __strong qualifier.
for Struct - The type is a struct containing a field whose type is not PCK_Trivial. Note that a C++ struct type does not necessarily match this; C++ copying semantics are too complex to express here, in part because they depend on the exact constructor or assignment operator that is chosen by overload resolution to do the copy.
================
Comment at: include/clang/AST/Type.h:1121
+ /// after it is moved, as opposed to a truely destructive move in which the
+ /// source object is placed in an uninitialized state.
+ PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;
----------------
"truly"
Hmm. Now that I'm thinking more about it, I'm not sure there's any point in tracking non-triviality of a C++-style destructive move separately from the non-triviality of a copy. It's hard to imagine that there would ever be a non-C++ type that primitively has non-trivial copies but trivial C++-style moves or vice-versa. Type-based destructors imply that the type represents some kind of resource, and a C++-style move will always be non-trivial for resource types because ownership of the resource needs to be given up by the old location. Otherwise, a type might be non-trivial to copy but not destroy because there's something special about how it's stored (like volatility), but it's hard to imagine what could possibly cause it to be non-trivial to destroy but not copy.
If we were tracking non-triviality of an *unsafe* destructive move, one that leaves the source in an uninitialized state, that's quite different.
I think there are three reasonable options here:
- Ignore the argument I just made about the types that we're *likely* to care about modeling and generalize your tracking to also distinguish construction from assignment. In such an environment, I think you can absolutely make an argument that it's still interesting to track C++-style moves separately from copies.
- Drop the tracking of destructive moves completely. If you want to keep the method around, find, but it can just call `isNonTrivialToPrimitiveCopy()`.
- Change the tracking of *destructive* moves to instead track *deinitializing* moves. The implementation would stop considering `__strong` types to be non-trivial to move.
But as things stand today, I do not see any point in separately tracking triviality of C++-style destructive moves.
================
Comment at: lib/AST/Type.cpp:2231
+
+ Qualifiers::ObjCLifetime Lifetime = getQualifiers().getObjCLifetime();
+ if (Lifetime == Qualifiers::OCL_Strong)
----------------
You can re-use this call to getQualifiers() in the check for `volatile` below.
================
Comment at: lib/AST/Type.cpp:3945
+ if (RT->getDecl()->isNonTrivialToPrimitiveDestroy())
+ return DK_nontrivial_c_struct;
+
----------------
Please combine this check with the getAsCXXRecordDecl() check below. That is, (1) check whether it's a record type once, and if so, (2) it's a CXXRecordDecl, ask about its destructor, and (3) if it's not a CXXRecordDecl, ask whether it's non-trivial to primitive destroy.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1779
+ break;
}
----------------
This entire switch can be within an `#ifndef NDEBUG` if you really feel it's important to assert.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1792
+ return std::make_pair(BlockCaptureEntityKind::BlockObject,
+ getBlockFieldFlagsForObjCObjectPointer(CI, T));
----------------
I think you should leave the byref case of that function here, but you can make it local to this if-clause.
================
Comment at: lib/CodeGen/CGDecl.cpp:1421
+ destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+ cleanupKind = getARCCleanupKind();
+ break;
----------------
rjmccall wrote:
> 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.
Ping about this again.
================
Comment at: lib/CodeGen/CGDecl.cpp:1423
+ if (capturedByInit)
+ drillIntoBlockVariable(*this, lvalue, cast<VarDecl>(D));
+
----------------
ahatanak wrote:
> 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?
>
There's an idiom of copying `__block` variables of *block pointer* type in their own initializers, as a way of constructing a recursive block. I don't know of any reason why someone would intentionally do it for some random struct type, but that's always a risk when locking down on something retroactively. Anyway, I agree that we can experiment to see if locking down on aggregate __block captures causes any actual problems.
================
Comment at: lib/CodeGen/CGExprAgg.cpp:255
+ Ty.isDestructedType() == QualType::DK_nontrivial_c_struct)
+ CGF.pushDestroy(Ty.isDestructedType(), src.getAggregateAddress(), Ty);
+
----------------
ahatanak wrote:
> 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.
Okay. I'm just a little worried about what this implies about when we push destructor cleanups in general. I know it's rather ad-hoc for C++ destructors right now.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+
+ TrivialFieldIsVolatile |= FT.isVolatileQualified();
+ if (Start == End)
----------------
ahatanak wrote:
> 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.
Okay, great! I like the name.
Does this mean we're now copying all structs that contain volatile fields with one of these helper functions? If so, please add a C test case testing just that. Also, you should retitle this review and stress that we're changing how *all* non-trivial types are copied, and that that includes both volatile and ARC-qualified fields.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:110
+ case QualType::PCK_ARCStrong:
+ return asDerived().visitStrong(FT, std::forward<Ts>(Args)...);
+ case QualType::PCK_Struct:
----------------
Please call these methods visitARCStrong to go with the new PCK name.
https://reviews.llvm.org/D41228
More information about the cfe-commits
mailing list