[CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation

[ahatanak]

Summary

Make sure the destructor is called if needed.

[rjmccall]

clang/test/CodeGenObjC/nontrivial-c-struct-property.m

89

It looks like we're copying a into a temporary and then copying out of that temporary into the argument slot. I don't see any reason the extra copy is required.

Also, aren't non-trivial structs consumed as arguments? If so, this would be an over-release.

[ahatanak]

clang/test/CodeGenObjC/nontrivial-c-struct-property.m

89

It isn't an over-release because, if the receiver isn't nil, temporary AGG_TMP is destructed in the callee and, if the receiver isn't nil, the temporary is destructed in the caller (by the call to @__destructor_8_s0(ptr %[[AGG_TMP]])).

As you pointed out, the copy isn't needed if the result of the assignment isn't used.

[rjmccall]

clang/test/CodeGenObjC/nontrivial-c-struct-property.m

89

Oh, is this second destructor call in conditionally-executed code? Can we test for that, at least?

[ahatanak]

Check that the destructor is called conditionally.

[ahatanak]

clang/test/CodeGenObjC/nontrivial-c-struct-property.m

89

The first copy constructor call is emitted when emitPseudoObjectExpr evaluates the OpaqueValueExpr that is the result expression. The second copy constructor call is emitted to copy the argument *a to the argument temporary.

Since the result expression isn't used in this case, there isn't any reason to emit the first copy constructor call. If Sema marks the OpaqueValueExpr for the result expression as unique when the result is unused, CodeGen avoids emitting the first copy constructor.

[rjmccall]

Oh, I see. That's a really unfortunate way to end up emitting this code pattern, since ignoring the result is so common. To fix that, we'd have to either figure out the result was unused in Sema or do a relatively complex analysis in IRGen, though.

Anyway, not something we have to do in this patch. LGTM.

We should reconsider the rules we use for temporary destruction one of these days, though. The current pattern is very error-prone, especially in the presence of exceptions.

[ahatanak]

In D136639#4006953, @rjmccall wrote:

Oh, I see. That's a really unfortunate way to end up emitting this code pattern, since ignoring the result is so common. To fix that, we'd have to either figure out the result was unused in Sema or do a relatively complex analysis in IRGen, though.

DiagnoseUnusedExprResult diagnoses unused expressions in Sema, so we can modify the PseudoObjectExprs when it's called.

clang/lib/CodeGen/CGExprAgg.cpp

public:
   void VisitOpaqueValueExpr(OpaqueValueExpr *E);
 
   void VisitPseudoObjectExpr(PseudoObjectExpr *E) {
     if (E->isGLValue()) {
       LValue LV = CGF.EmitPseudoObjectLValue(E);
       return EmitFinalDestCopy(E->getType(), LV);
     }
 
-    CGF.EmitPseudoObjectRValue(E, EnsureSlot(E->getType()));
+    AggValueSlot Slot = EnsureSlot(E->getType());
+    bool NeedsDestruction =
+        !Slot.isExternallyDestructed() &&
+        E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct;
+    if (NeedsDestruction)
+      Slot.setExternallyDestructed();
+    CGF.EmitPseudoObjectRValue(E, Slot);
+    if (NeedsDestruction)
+      CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Slot.getAddress(),
+                      E->getType());
   }
 
   void VisitVAArgExpr(VAArgExpr *E);
 
   void EmitInitializationToLValue(Expr *E, LValue Address);
   void EmitNullInitializationToLValue(LValue Address);
   //  case Expr::ChooseExprClass:
   void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }

clang/test/CodeGenObjC/nontrivial-c-struct-property.m

 // CHECK: call void @objc_copyCppObjectAtomic({{.*}}, {{.*}}, ptr noundef @__copy_constructor_8_8_s0)
 // CHECK-NOT: call
 // CHECK: ret i64
 
 // CHECK-LABEL: define internal void @"\01-[C setAtomic0:]"(
 // CHECK: call void @objc_copyCppObjectAtomic({{.*}}, {{.*}}, ptr noundef @__move_assignment_8_8_s0)
 // CHECK-NOT: call
 // CHECK: ret void
+
+// CHECK: define void @test0(ptr noundef %[[C:.*]], ptr noundef %[[A:.*]])
+// CHECK: %[[C_ADDR:.*]] = alloca ptr, align 8
+// CHECK: %[[A_ADDR:.*]] = alloca ptr, align 8
+// CHECK: %[[AGG_TMP_ENSURED:.*]] = alloca %[[STRUCT_S0]], align 8
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_S0]], align 8
+// CHECK: store ptr null, ptr %[[C_ADDR]], align 8
+// CHECK: call void @llvm.objc.storeStrong(ptr %[[C_ADDR]], ptr %[[C]])
+// CHECK: store ptr %[[A]], ptr %[[A_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load ptr, ptr %[[A_ADDR]], align 8
+// CHECK: call void @__copy_constructor_8_8_s0(ptr %[[AGG_TMP_ENSURED]], ptr %[[V0]])
+// CHECK: %[[V1:.*]] = load ptr, ptr %[[C_ADDR]], align 8
+// CHECK: call void @__copy_constructor_8_8_s0(ptr %[[AGG_TMP]], ptr %[[AGG_TMP_ENSURED]])
+// CHECK: %[[V2:.*]] = icmp eq ptr %[[V1]], null
+// CHECK: br i1 %[[V2]], label %[[MSGSEND_NULL:.*]], label %[[MSGSEND_CALL:.*]]
+
+// CHECK: [[MSGSEND_CALL]]:
+// CHECK: %[[V3:.*]] = load ptr, ptr @OBJC_SELECTOR_REFERENCES_, align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_S0]], ptr %[[AGG_TMP]], i32 0, i32 0
+// CHECK: %[[V4:.*]] = load ptr, ptr %[[COERCE_DIVE]], align 8
+// CHECK: %[[COERCE_VAL_PI:.*]] = ptrtoint ptr %[[V4]] to i64
+// CHECK: call void @objc_msgSend(ptr noundef %[[V1]], ptr noundef %[[V3]], i64 %[[COERCE_VAL_PI]])
+// CHECK: br label %[[MSGSEND_CONT:.*]]
+
+// CHECK: [[MSGSEND_NULL]]:
+// CHECK: call void @__destructor_8_s0(ptr %[[AGG_TMP]])
+// CHECK: br label %[[MSGSEND_CONT]]
+
+// CHECK: [[MSGSEND_CONT]]:
+// CHECK: call void @__destructor_8_s0(ptr %[[AGG_TMP_ENSURED]]
+// CHECK: call void @llvm.objc.storeStrong(ptr %[[C_ADDR]], ptr null)
+// CHECK: ret void
+
+void test0(C *c, S0 *a) {
+  c.atomic0 = *a;
+}