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