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