[CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators

[ahatanak]

Summary

rdar://problem/64989559

[ahatanak]

ping

[rjmccall]

LGTM.

[dblaikie]

Idle curiosity: The patch description says "non-trivial C struct temporaries" - can C have non-trivial structs? Or is this limited to Objective C (judging/guessing by the tests)?

[ahatanak]

C structs with ObjC pointer fields are non-trivial when ARC is enabled since construction, destruction, and copy are non-trivial (see https://reviews.llvm.org/D41228). As far as I know, this is limited to ObjC.

[dblaikie]

In D83448#2357574, @ahatanak wrote:

C structs with ObjC pointer fields are non-trivial when ARC is enabled since construction, destruction, and copy are non-trivial (see https://reviews.llvm.org/D41228). As far as I know, this is limited to ObjC.

OK, good to know - thanks for explaining!

[rjmccall]

Pointer authentication can also make structs non-trivial to copy.

clang/lib/CodeGen/CGExprAgg.cpp

void AggExprEmitter::VisitBinAssign(const BinaryOperator *E) {
   if (!LHSSlot.isVolatile() &&
       CGF.hasVolatileMember(E->getLHS()->getType()))
     LHSSlot.setVolatile(true);
 
   CGF.EmitAggExpr(E->getRHS(), LHSSlot);
 
   // Copy into the destination if the assignment isn't ignored.
   EmitFinalDestCopy(E->getType(), LHS);
+
+  if (!Dest.isIgnored() && !Dest.isExternallyDestructed() &&
+      E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+    CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Dest.getAddress(),
+                    E->getType());
 }
 
 void AggExprEmitter::
 VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
   llvm::BasicBlock *LHSBlock = CGF.createBasicBlock("cond.true");
   llvm::BasicBlock *RHSBlock = CGF.createBasicBlock("cond.false");
   llvm::BasicBlock *ContBlock = CGF.createBasicBlock("cond.end");
 
   // Bind the common expression if necessary.
   CodeGenFunction::OpaqueValueMapping binding(CGF, E);
 
   CodeGenFunction::ConditionalEvaluation eval(CGF);
   CGF.EmitBranchOnBoolExpr(E->getCond(), LHSBlock, RHSBlock,
                            CGF.getProfileCount(E));
 
   // Save whether the destination's lifetime is externally managed.
   bool isExternallyDestructed = Dest.isExternallyDestructed();
+  bool destructNonTrivialCStruct =
+      !isExternallyDestructed &&
+      E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct;
+  isExternallyDestructed |= destructNonTrivialCStruct;
+  Dest.setExternallyDestructed(isExternallyDestructed);
 
   eval.begin(CGF);
   CGF.EmitBlock(LHSBlock);
   CGF.incrementProfileCounter(E);
   Visit(E->getTrueExpr());
   eval.end(CGF);
 
   assert(CGF.HaveInsertPoint() && "expression evaluation ended with no IP!");
   CGF.Builder.CreateBr(ContBlock);
 
   // If the result of an agg expression is unused, then the emission
   // of the LHS might need to create a destination slot.  That's fine
   // with us, and we can safely emit the RHS into the same slot, but
   // we shouldn't claim that it's already being destructed.
   Dest.setExternallyDestructed(isExternallyDestructed);
 
   eval.begin(CGF);
   CGF.EmitBlock(RHSBlock);
   Visit(E->getFalseExpr());
   eval.end(CGF);
 
+  if (destructNonTrivialCStruct)
+    CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Dest.getAddress(),
+                    E->getType());
+
   CGF.EmitBlock(ContBlock);
 }
 
 void AggExprEmitter::VisitChooseExpr(const ChooseExpr *CE) {
   Visit(CE->getChosenSubExpr());
 }
 
 void AggExprEmitter::VisitVAArgExpr(VAArgExpr *VE) {

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

 void func(Strong *);
 
 @interface C
 - (StrongSmall)getStrongSmall;
 + (StrongSmall)getStrongSmallClass;
 @end
 
 id g0;
+StrongSmall g1, g2;
 
+// CHECK: %[[STRUCT_STRONGSMALL:.*]] = type { i32, i8* }
 // CHECK: %[[STRUCT_STRONGOUTER:.*]] = type { %[[STRUCT_STRONG:.*]], i8*, double }
 // CHECK: %[[STRUCT_STRONG]] = type { %[[STRUCT_TRIVIAL:.*]], i8* }
 // CHECK: %[[STRUCT_TRIVIAL]] = type { [4 x i32] }
 // CHECK: %[[STRUCT_BLOCK_BYREF_T:.*]] = type { i8*, %[[STRUCT_BLOCK_BYREF_T]]*, i32, i32, i8*, i8*, i8*, %[[STRUCT_STRONGOUTER]] }
-// CHECK: %[[STRUCT_STRONGSMALL:.*]] = type { i32, i8* }
 // CHECK: %[[STRUCT_STRONGBLOCK:.*]] = type { void ()* }
 // CHECK: %[[STRUCT_BITFIELD1:.*]] = type { i8, i8, i8*, i32, i8*, [3 x i32], i8*, double, i8, i8 }
 
 // CHECK: define void @test_constructor_destructor_StrongOuter()
 // CHECK: %[[T:.*]] = alloca %[[STRUCT_STRONGOUTER]], align 8
 // CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T]] to i8**
 // CHECK: call void @__default_constructor_8_S_s16_s24(i8** %[[V0]])
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGOUTER]]* %[[T]] to i8**

 
 // CHECK: define linkonce_odr hidden void @__default_constructor_8_sv0
 // CHECK: define linkonce_odr hidden void @__copy_assignment_8_8_sv0
 void test_zero_bitfield() {
   struct ZeroBitfield volatile a, b;
   a = b;
 }
 
+// CHECK-LABEL: define i8* @test_conditional0(
+// CHECK: %[[TMP:.*]] = alloca %[[STRUCT_STRONGSMALL]], align 8
+
+// CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__copy_constructor_8_8_t0w4_s8(i8** %[[V1]], i8** bitcast (%[[STRUCT_STRONGSMALL]]* @g2 to i8**))
+
+// CHECK: %[[V2:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__copy_constructor_8_8_t0w4_s8(i8** %[[V2]], i8** bitcast (%[[STRUCT_STRONGSMALL]]* @g1 to i8**))
+
+// CHECK: %[[V5:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V5]])
+// CHECK: @llvm.objc.autoreleaseReturnValue
+
+id test_conditional0(int c) {
+  return (c ? g2 : g1).f1;
+}
+
+// CHECK-LABEL: define i8* @test_conditional1(
+// CHECK-NOT: call void @__destructor
+
+id test_conditional1(int c) {
+  calleeStrongSmall(c ? g2 : g1);
+}
+
+// CHECK-LABEL: define i8* @test_assignment0(
+// CHECK: %[[TMP:.*]] = alloca %[[STRUCT_STRONGSMALL]], align 8
+// CHECK: call void @__copy_assignment_8_8_t0w4_s8(i8** bitcast (%[[STRUCT_STRONGSMALL]]* @g2 to i8**), i8** bitcast (%[[STRUCT_STRONGSMALL]]* @g1 to i8**))
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__copy_constructor_8_8_t0w4_s8(i8** %[[V0]], i8** bitcast (%[[STRUCT_STRONGSMALL]]* @g2 to i8**))
+// CHECK: %[[V3:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V3]])
+
+id test_assignment0(void) {
+  return (g2 = g1).f1;
+}
+
+// CHECK-LABEL: define i8* @test_assignment1(
+// CHECK-NOT: call void @__destructor
+
+id test_assignment1(void) {
+  calleeStrongSmall(g2 = g1);
+}
+
 #endif /* USESTRUCT */