[CodeGen] Emit destructor calls to destruct compound literals

[ahatanak]

Summary

This patch fixes a bug in IRGen where it wasn't calling the destructors to destruct compound literals.

I've split out the changes needed to emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects (https://reviews.llvm.org/D66094).

rdar://problem/51867864

[ahatanak]

lib/CodeGen/CGExprAgg.cpp

248

This change wasn't needed to fix the bugs, but I think isExternallyDestructed instead of isIgnored should be called to determine whether the returned value requires destruction.

[rjmccall]

lib/CodeGen/CGExpr.cpp

4100

Unfortunately, the lifetime of compound literals in C is not this simple; they're like blocks in that they're destroyed at the end of the enclosing scope rather than at the end of the current statement. (The cleanup here will be popped at the end of the full-expression if we've entered an ExprWithCleanups.) And the l-value case is exactly the case where this matters.

I think you need to do something like what we do with blocks, where we record all the blocks in the full-expression on the ExprWithCleanups so that we can push an inactive cleanup for them and then activate it when we emit the block.

lib/CodeGen/CGExprAgg.cpp

248

Agreed.

668

The cleanup shouldn't be pushed until after you've finished evaluating the initializer.

[ahatanak]

• Emit member access, compound literal, and call expressions as subexpressions of ExprWithCleanups if the expressions are of C struct types that require non-trivial destruction. This fixes the bug in IRGen where it was destructing the function return at the end of the enclosing scope rather than at the end of the full expression (see the changes made in test/CodeGenObjC/arc.m).
• Add compound literal expressions with automatic storage duration to the list of cleanup objects of ExprWithCleanups if the expressions have C struct types requiring non-trivial destruction. This enables IRGen to destruct the compound literals at the end of their enclosing scopes.

[rjmccall]

include/clang/AST/ExprCXX.h

3220 ↗

(On Diff #212942)

Might be worth spelling out and block-scoped compound literals here.

include/clang/Basic/DiagnosticSemaKinds.td

5171 ↗

(On Diff #212942)

Ah, good catch.

include/clang/Sema/Sema.h

587 ↗

(On Diff #212942)

I think the second sentence in this comment is no longer useful; you can just strike it.

lib/CodeGen/CGBlocks.cpp

863 ↗

(On Diff #212942)

This comment is no longer accurate. If you want to move the function, please do so in a separate commit, though.

lib/CodeGen/CGCleanup.cpp

1283 ↗

(On Diff #212942)

Please include in the comment here that this doesn't apply in C++, where the compound literal has temporary lifetime.

Can we clarify that we're talking about block-scope literals in all the new method names?

lib/CodeGen/CGDecl.cpp

555 ↗

(On Diff #212942)

It'd be nice to not shadow the enclosing variable.

lib/CodeGen/CGExpr.cpp

4100

Can we make the check here something like (1) this is a block-scope compound literal and (2) it has a non-trivially-destructed type (of any kind)? That way we're not conflating two potentially unrelated elements, the lifetime of the object and the kinds of types that can be constructed by the literal.

Oh, actually, there's a concrete reason to do this: C99 compound literals are not required to have struct type; they can have any object type, including arrays but also scalars. So we could, even without non-trivial C structs, have a block-scope compound of type __strong id[]; I guess we've always just gotten this wrong. Please add tests for this case. :)

4585

Does EmitCallExpr not enter a cleanup when it returns an aggregate that's not into an externally-destructed slot? That seems wrong and dangerous.

[ahatanak]

lib/CodeGen/CGExpr.cpp

4585

I'm going to split this patch into two parts, one for compound literals and the other for everything else. The patch is getting too large and I also found another bug: a cleanup isn't pushed for ObjC message send.

[martong]

test/Import/objc-arc/Inputs/cleanup-objects.m

6 ↗

(On Diff #212942)

Perhaps then this patch depends on another patch which implements the import of a BlockExpr?
Or maybe the branch if (auto *BD = From.dyn_cast<BlockDecl *>()) should be left out from the ASTImporter code, and this way BlockExpr and BlockDecl would be implemented later in another patch.

[ahatanak]

Address review comments:

• Destruct compound literals of ObjC array types too.
• Use pushLifetimeExtendedDestroy to extend the lifetime of compound literals to the end of the surrounding block. This simplifies the changes made in IRGen, but I was wondering whether we should do what we do to extend lifetime of block captures instead.

[ahatanak]

test/Import/objc-arc/Inputs/cleanup-objects.m

6 ↗

(On Diff #212942)

Yes, I removed it from ASTImporter::Import and left a FIXME comment there.

[ahatanak]

lib/CodeGen/CGExpr.cpp

4100

There is a check E->isFileScope() above this. Is that sufficient to check for block-scoped compound literals?

[martong]

clang/lib/AST/ASTImporter.cpp

7918 ↗

(On Diff #247919)

I think

return make_error<ImportError(ImportError::UnsupportedConstruct);

would be more appropriate here as this expresses that we do not support every aspect yet.

On the other hand a nullptr could be dereferenced later and thus could cause an unwanted crash.

[ahatanak]

Return make_error<ImportError> instead of a nullptr.

[rjmccall]

lib/CodeGen/CGExpr.cpp

4100

That plus the C/C++ difference; compound literals in C++ are just temporaries.

[rjmccall]

This patch looks good except for that C/C++ semantic difference. A compound literal temporary can be lifetime-extended in C++, but only in the standard way of binding a reference to it.

[ahatanak]

lib/CodeGen/CGExpr.cpp

4100

I haven't been able to come up with a piece of C++ code that executes EmitCompoundLiteralLValue. The following code gets rejected because you can't take the address of a temporary object in C++:

StrongSmall *p = &(StrongSmall){ 1, 0 };

If a bind a reference to it, AggExprEmitter::VisitCompoundLiteralExpr is called.

[rjmccall]

lib/CodeGen/CGExpr.cpp

4100

That makes sense; they're not gl-values in C++. It would be reasonable to assert that. But the C++ point does apply elsewhere.

[ahatanak]

Don't try to push a cleanup in C++.

[ahatanak]

lib/CodeGen/CGExpr.cpp

4100

It turns out this function is called in C++ when the compound literal is a vector type, so I've just added a check for C++ instead of an assert.

[rjmccall]

clang/lib/CodeGen/CGBlocks.cpp

869 ↗

(On Diff #248797)

I wonder if we could just switch blocks to the same thing.

clang/lib/Sema/SemaExpr.cpp

6271 ↗

(On Diff #248797)

This should all be conditional on C++ (but you should leave a comment explaining why).

clang/lib/Serialization/ASTWriterStmt.cpp

1729 ↗

(On Diff #248797)

Will this just serialize a second copy of the compound literal expression?

lib/CodeGen/CGExpr.cpp

4100

Really? Is the expression actually an l-value in this case somehow?

[martong]

Looks good from the ASTImporter's point of view. I don't have the competence to review the rest, so I am resigning.

[ahatanak]

Address review comments

[ahatanak]

clang/lib/CodeGen/CGBlocks.cpp

869 ↗

(On Diff #248797)

I think we can, but I haven't tried. We can fix it in a separate patch.

clang/lib/CodeGen/CGExprAgg.cpp

664 ↗

(On Diff #249173)

I fixed the way IsExternallyDestructed is used based on the comment in the other review.

clang/lib/Serialization/ASTWriterStmt.cpp

1729 ↗

(On Diff #248797)

This should emit a serialization::STMT_REF_PTR record, which is a reference to the previously serialized compound literal (see ASTWriter::WriteSubStmt).

lib/CodeGen/CGExpr.cpp

4100

I see this function being called when ScalarExprEmitter::VisitCompoundLiteralExpr calls EmitLoadOfLValue.

[rjmccall]

Minor comments; otherwise LGTM.

clang/lib/Sema/SemaExpr.cpp

6256 ↗

(On Diff #249173)

Probably add "in C; in C++, they're just temporaries".

lib/CodeGen/CGExpr.cpp

4100

Okay. As a general rule, nothing should be calling EmitLValue on an expression that isn't actually an l-value. It's a little more reasonable here because it's more like a helper routine. If this is the cleanest way to handle this in C++, it's okay, but please leave a comment explaining that here.

[ahatanak]

lib/CodeGen/CGExpr.cpp

4100

I added a comment in ScalarExprEmitter::VisitCompoundLiteralExpr.

lib/CodeGen/CGExpr.cpp

LValue CodeGenFunction::EmitCompoundLiteralLValue(const CompoundLiteralExpr *E){
 
   Address DeclPtr = CreateMemTemp(E->getType(), ".compoundliteral");
   const Expr *InitExpr = E->getInitializer();
   LValue Result = MakeAddrLValue(DeclPtr, E->getType(), AlignmentSource::Decl);
 
   EmitAnyExprToMem(InitExpr, DeclPtr, E->getType().getQualifiers(),
                    /*Init*/ true);
 
+  // Push a destructor cleanup if this is a non-trivial C struct. A compound
+  // literal object that doesn't have a file scope has automatic
+  // storage duration.
+  if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+    pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType());
+
   return Result;
 }
 
 LValue CodeGenFunction::EmitInitListLValue(const InitListExpr *E) {
   if (!E->isGLValue())
     // Initializing an aggregate temporary in C++11: T{...}.
     return EmitAggExprToLValue(E);
 
case TEK_Aggregate:
     return EmitAggExprToLValue(E);
   }
   llvm_unreachable("bad evaluation kind");
 }
 
 LValue CodeGenFunction::EmitCallExprLValue(const CallExpr *E) {
   RValue RV = EmitCallExpr(E);
 
+  if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+    pushDestroy(QualType::DK_nontrivial_c_struct, RV.getAggregateAddress(),
+                E->getType());
+
   if (!RV.isScalar())
     return MakeAddrLValue(RV.getAggregateAddress(), E->getType(),
                           AlignmentSource::Decl);
 
   assert(E->getCallReturnType(getContext())->isReferenceType() &&
          "Can't have a scalar return unless the return type is a "
          "reference type!");
 

lib/CodeGen/CGExprAgg.cpp

bool AggExprEmitter::TypeRequiresGCollection(QualType T) {
   // Check whether the type has an object member.
   return Record->hasObjectMember();
 }
 
 void AggExprEmitter::withReturnValueSlot(
     const Expr *E, llvm::function_ref<RValue(ReturnValueSlot)> EmitCall) {
   QualType RetTy = E->getType();
   bool RequiresDestruction =
-      Dest.isIgnored() &&
+      !Dest.isExternallyDestructed() &&
       RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct;
 
   // If it makes no observable difference, save a memcpy + temporary.
   //
   // We need to always provide our own temporary if destruction is required.
   // Otherwise, EmitCall will emit its own, notice that it's "unused", and end
   // its lifetime before we have the chance to emit a proper destructor call.
   bool UseTemp = Dest.isPotentiallyAliased() || Dest.requiresGCollection() ||
if (Dest.isPotentiallyAliased() &&
       E->getType().isPODType(CGF.getContext())) {
     // For a POD type, just emit a load of the lvalue + a copy, because our
     // compound literal might alias the destination.
     EmitAggLoadOfLValue(E);
     return;
   }
 
   AggValueSlot Slot = EnsureSlot(E->getType());
+
+  // Push a destructor cleanup if this is a non-trivial C struct. A compound
+  // literal object that doesn't have a file scope has automatic
+  // storage duration.
+  if (!Dest.isExternallyDestructed() &&
+      E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) {
+    CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Slot.getAddress(),
+                    E->getType());
+    Dest.setExternallyDestructed();
+  }
   CGF.EmitAggExpr(E->getInitializer(), Slot);
 }
 
 /// Attempt to look through various unimportant expressions to find a
 /// cast of the given kind.
 static Expr *findPeephole(Expr *op, CastKind kind) {
   while (true) {
     op = op->IgnoreParens();
void AggExprEmitter::VisitCastExpr(CastExpr *E) {
   case CK_AddressSpaceConversion:
      return Visit(E->getSubExpr());
 
   case CK_LValueToRValue:
     // If we're loading from a volatile type, force the destination
     // into existence.
     if (E->getSubExpr()->getType().isVolatileQualified()) {
       EnsureDest(E->getType());
+      if (!Dest.isExternallyDestructed() &&
+          E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) {
+        CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Dest.getAddress(),
+                        E->getType());
+        Dest.setExternallyDestructed();
+      }
       return Visit(E->getSubExpr());
     }
 
     LLVM_FALLTHROUGH;
 
 
   case CK_NoOp:
   case CK_UserDefinedConversion:

test/CodeGenObjC/strong-in-c-struct.m

 // CHECK: ret void
 
 void test_copy_assignment_StrongBlock(StrongBlock *d, StrongBlock *s) {
   *d = *s;
 }
 
 // CHECK: define void @test_copy_constructor_StrongVolatile0(
 // CHECK: call void @__copy_constructor_8_8_t0w4_sv8(
+// CHECK-NOT: call
 // CHECK: call void @__destructor_8_sv8(
+// CHECK-NOT: call
 
 // CHECK: define linkonce_odr hidden void @__copy_constructor_8_8_t0w4_sv8(
 // CHECK: %[[V8:.*]] = load volatile i8*, i8** %{{.*}}, align 8
 // CHECK: %[[V9:.*]] = call i8* @llvm.objc.retain(i8* %[[V8]])
 // CHECK: store volatile i8* %[[V9]], i8** %{{.*}}, align 8
 
 void test_copy_constructor_StrongVolatile0(StrongVolatile *s) {
   StrongVolatile t = *s;

 // CHECK: %[[V13:.*]] = bitcast i8** %[[ADDR_CUR1]] to i32*
 // CHECK: %[[V14:.*]] = load volatile i32, i32* %[[V13]], align 4
 // CHECK: store volatile i32 %[[V14]], i32* %[[V12]], align 4
 
 void test_copy_constructor_VolatileArray(VolatileArray *a) {
   VolatileArray t = *a;
 }
 
+// CHECK: define void @test_member_access(
+// CHECK: %[[TMP:.*]] = alloca %[[STRUCT_STRONGSMALL]],
+// CHECK: %[[V3:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V3]])
+
+void test_member_access(void) {
+  id t = getStrongSmall().f1;
+}
+
+// CHECK: define void @test_compound_literal(
+// CHECK: %[[_COMPOUNDLITERAL:.*]] = alloca %[[STRUCT_STRONGSMALL]],
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[_COMPOUNDLITERAL]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V0]])
+
+void test_compound_literal(void) {
+  StrongSmall *p = &(StrongSmall){ 1, 0 };
+}
+
+// CHECK: define void @test_compound_literal2(
+// CHECK: %[[AGG_TMP_ENSURED:.*]] = alloca %[[STRUCT_STRONGSMALL]],
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[AGG_TMP_ENSURED]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V0]])
+
+void test_compound_literal2(void) {
+  (void)(StrongSmall){ 1, 0 };
+}
+
+// CHECK: define void @test_volatile_variable_reference(
+// CHECK: %[[AGG_TMP_ENSURED:.*]] = alloca %[[STRUCT_STRONGSMALL]], align 8
+// CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[AGG_TMP_ENSURED]] to i8**
+// CHECK: call void @__copy_constructor_8_8_tv0w32_sv8(i8** %[[V1]], i8** %{{.*}})
+// CHECK: %[[V3:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[AGG_TMP_ENSURED]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V3]])
+
+void test_volatile_variable_reference(volatile StrongSmall *a) {
+  (void)*a;
+}
+
 #endif /* USESTRUCT */