[CodeGen][ObjC] Call synthesized copy constructor/assignment operator functions in getter/setter functions of non-trivial C struct properties

[ahatanak]

Summary

This fixes a bug where the getter/setter functions were doing a trivial copy instead of calling the synthesized functions that copy non-trivial C struct types.

This fixes https://github.com/llvm/llvm-project/issues/56680.

[ahatanak]

There is another bug where an object isn't destructed when a non-trivial C struct property is set. I plan to send a patch that fixes the bug after this.

[rjmccall]

clang/lib/CodeGen/CGObjC.cpp

1449

When the method takes the structure by value, we're passed an owned copy with the responsibility to destroy it in the method, right? Is there a way for us to do a destructive move-assignment here and suppress the normal destruction of the parameter?

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

40

Yeah, this is the pattern which it would be great to avoid if we can.

[ahatanak]

clang/lib/CodeGen/CGObjC.cpp

1449

We could check whether a function parameter is a setter parameter and avoid pushing the cleanup for it if it is.

If you are thinking about a more general way of merging copy+destruction into a move, I think we need to take a different approach.

[ahatanak]

Call the move assignment operator in the setter instead of calling the copy assignment operator and the destructor.

[rjmccall]

Can we check for the right conditions when emitting the setter body and just deactivate the cleanup?

[ahatanak]

Deactivate the cleanup that was pushed in EmitParmDecl.

[rjmccall]

LGTM

clang/lib/CodeGen/CGObjC.cpp

 #include "ConstantEmitter.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "clang/CodeGen/CodeGenABITypes.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Analysis/ObjCARCUtil.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InlineAsm.h"
 using namespace clang;
 using namespace CodeGen;
static llvm::Value *emitCmdValueForGetterSetterBody(CodeGenFunction &CGF,
   return CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(MD->getCmdDecl()), "cmd");
 }
 
 void
 CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl *classImpl,
                                         const ObjCPropertyImplDecl *propImpl,
                                         const ObjCMethodDecl *GetterMethodDecl,
                                         llvm::Constant *AtomicHelperFn) {
+
+  ObjCIvarDecl *ivar = propImpl->getPropertyIvarDecl();
+
+  if (ivar->getType().isNonTrivialToPrimitiveCopy() == QualType::PCK_Struct) {
+    if (!AtomicHelperFn) {
+      LValue Src =
+          EmitLValueForIvar(TypeOfSelfObject(), LoadObjCSelf(), ivar, 0);
+      LValue Dst = MakeAddrLValue(ReturnValue, ivar->getType());
+      callCStructCopyConstructor(Dst, Src);
+    } else {
+      ObjCIvarDecl *ivar = propImpl->getPropertyIvarDecl();
+      emitCPPObjectAtomicGetterCall(*this, ReturnValue.getPointer(), ivar,
+                                    AtomicHelperFn);
+    }
+    return;
+  }
+
   // If there's a non-trivial 'get' expression, we just have to emit that.
   if (!hasTrivialGetExpr(propImpl)) {
     if (!AtomicHelperFn) {
       auto *ret = ReturnStmt::Create(getContext(), SourceLocation(),
                                      propImpl->getGetterCXXConstructor(),
                                      /* NRVOCandidate=*/nullptr);
       EmitReturnStmt(*ret);
     }
     else {
       ObjCIvarDecl *ivar = propImpl->getPropertyIvarDecl();
       emitCPPObjectAtomicGetterCall(*this, ReturnValue.getPointer(),
                                     ivar, AtomicHelperFn);
     }
     return;
   }
 
   const ObjCPropertyDecl *prop = propImpl->getPropertyDecl();
   QualType propType = prop->getType();
   ObjCMethodDecl *getterMethod = propImpl->getGetterMethodDecl();
 
-  ObjCIvarDecl *ivar = propImpl->getPropertyIvarDecl();
-
   // Pick an implementation strategy.
   PropertyImplStrategy strategy(CGM, propImpl);
   switch (strategy.getKind()) {
   case PropertyImplStrategy::Native: {
     // We don't need to do anything for a zero-size struct.
     if (strategy.getIvarSize().isZero())
       return;
 

 
 void
 CodeGenFunction::generateObjCSetterBody(const ObjCImplementationDecl *classImpl,
                                         const ObjCPropertyImplDecl *propImpl,
                                         llvm::Constant *AtomicHelperFn) {
   ObjCIvarDecl *ivar = propImpl->getPropertyIvarDecl();
   ObjCMethodDecl *setterMethod = propImpl->getSetterMethodDecl();
 
+  if (ivar->getType().isNonTrivialToPrimitiveCopy() == QualType::PCK_Struct) {
+    ParmVarDecl *PVD = *setterMethod->param_begin();
+    if (!AtomicHelperFn) {
+      // Call the move assignment operator instead of calling the copy
+      // assignment operator and destructor.
+      LValue Dst = EmitLValueForIvar(TypeOfSelfObject(), LoadObjCSelf(), ivar,
+                                     /*quals*/ 0);
+      LValue Src = MakeAddrLValue(GetAddrOfLocalVar(PVD), ivar->getType());
+      callCStructMoveAssignmentOperator(Dst, Src);
+    } else {
+      // If atomic, assignment is called via a locking api.
+      emitCPPObjectAtomicSetterCall(*this, setterMethod, ivar, AtomicHelperFn);
+    }
+    // Decativate the destructor for the setter parameter.
+    DeactivateCleanupBlock(CalleeDestructedParamCleanups[PVD], AllocaInsertPt);
+    return;
+  }
+
   // Just use the setter expression if Sema gave us one and it's
   // non-trivial.
   if (!hasTrivialSetExpr(propImpl)) {
     if (!AtomicHelperFn)
       // If non-atomic, assignment is called directly.
       EmitStmt(propImpl->getSetterCXXAssignment());
     else
       // If atomic, assignment is called via a locking api.

 
 /// GenerateObjCAtomicSetterCopyHelperFunction - Given a c++ object type with
 /// non-trivial copy assignment function, produce following helper function.
 /// static void copyHelper(Ty *dest, const Ty *source) { *dest = *source; }
 ///
 llvm::Constant *
 CodeGenFunction::GenerateObjCAtomicSetterCopyHelperFunction(
                                         const ObjCPropertyImplDecl *PID) {
+  const ObjCPropertyDecl *PD = PID->getPropertyDecl();
+  if ((!(PD->getPropertyAttributes() & ObjCPropertyAttribute::kind_atomic)))
+    return nullptr;
+
+  QualType Ty = PID->getPropertyIvarDecl()->getType();
+  ASTContext &C = getContext();
+
+  if (Ty.isNonTrivialToPrimitiveCopy() == QualType::PCK_Struct) {
+    // Call the move assignment operator instead of calling the copy assignment
+    // operator and destructor.
+    CharUnits Alignment = C.getTypeAlignInChars(Ty);
+    llvm::Constant *Fn = getNonTrivialCStructMoveAssignmentOperator(
+        CGM, Alignment, Alignment, Ty.isVolatileQualified(), Ty);
+    return llvm::ConstantExpr::getBitCast(Fn, VoidPtrTy);
+  }
+
   if (!getLangOpts().CPlusPlus ||
       !getLangOpts().ObjCRuntime.hasAtomicCopyHelper())
     return nullptr;
-  QualType Ty = PID->getPropertyIvarDecl()->getType();
   if (!Ty->isRecordType())
     return nullptr;
-  const ObjCPropertyDecl *PD = PID->getPropertyDecl();
-  if ((!(PD->getPropertyAttributes() & ObjCPropertyAttribute::kind_atomic)))
-    return nullptr;
   llvm::Constant *HelperFn = nullptr;
   if (hasTrivialSetExpr(PID))
     return nullptr;
   assert(PID->getSetterCXXAssignment() && "SetterCXXAssignment - null");
   if ((HelperFn = CGM.getAtomicSetterHelperFnMap(Ty)))
     return HelperFn;
 
-  ASTContext &C = getContext();
   IdentifierInfo *II
     = &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
 
   QualType ReturnTy = C.VoidTy;
   QualType DestTy = C.getPointerType(Ty);
   QualType SrcTy = Ty;
   SrcTy.addConst();
   SrcTy = C.getPointerType(SrcTy);
CodeGenFunction::GenerateObjCAtomicSetterCopyHelperFunction(
   EmitStmt(TheCall);
 
   FinishFunction();
   HelperFn = llvm::ConstantExpr::getBitCast(Fn, VoidPtrTy);
   CGM.setAtomicSetterHelperFnMap(Ty, HelperFn);
   return HelperFn;
 }
 
-llvm::Constant *
-CodeGenFunction::GenerateObjCAtomicGetterCopyHelperFunction(
-                                            const ObjCPropertyImplDecl *PID) {
+llvm::Constant *CodeGenFunction::GenerateObjCAtomicGetterCopyHelperFunction(
+  const ObjCPropertyDecl *PD = PID->getPropertyDecl();
+  if ((!(PD->getPropertyAttributes() & ObjCPropertyAttribute::kind_atomic)))
+    return nullptr;
+
+  QualType Ty = PD->getType();
+  ASTContext &C = getContext();
+
+  if (Ty.isNonTrivialToPrimitiveCopy() == QualType::PCK_Struct) {
+    CharUnits Alignment = C.getTypeAlignInChars(Ty);
+    llvm::Constant *Fn = getNonTrivialCStructCopyConstructor(
+        CGM, Alignment, Alignment, Ty.isVolatileQualified(), Ty);
+    return llvm::ConstantExpr::getBitCast(Fn, VoidPtrTy);
+  }
+
   if (!getLangOpts().CPlusPlus ||
       !getLangOpts().ObjCRuntime.hasAtomicCopyHelper())
     return nullptr;
-  const ObjCPropertyDecl *PD = PID->getPropertyDecl();
-  QualType Ty = PD->getType();
   if (!Ty->isRecordType())
     return nullptr;
-  if ((!(PD->getPropertyAttributes() & ObjCPropertyAttribute::kind_atomic)))
-    return nullptr;
   llvm::Constant *HelperFn = nullptr;
   if (hasTrivialGetExpr(PID))
     return nullptr;
   assert(PID->getGetterCXXConstructor() && "getGetterCXXConstructor - null");
   if ((HelperFn = CGM.getAtomicGetterHelperFnMap(Ty)))
     return HelperFn;
 
-  ASTContext &C = getContext();
   IdentifierInfo *II =
       &CGM.getContext().Idents.get("__copy_helper_atomic_property_");
 
   QualType ReturnTy = C.VoidTy;
   QualType DestTy = C.getPointerType(Ty);
   QualType SrcTy = Ty;
   SrcTy.addConst();
   SrcTy = C.getPointerType(SrcTy);

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

+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+
+typedef struct {
+  id x;
+} S0;
+
+@interface C {
+  S0 _p1;
+}
+@property(nonatomic) S0 nonatomic;
+@property S0 atomic0;
+@property S0 p1;
+-(S0)p1;
+-(void)setP1:(S0)s0;
+@end
+
+@implementation C
+-(S0)p1 {
+  return _p1;
+}
+-(void)setP1:(S0)s0 {
+  _p1 = s0;
+}
+@end
+
+// CHECK: %[[STRUCT_S0:.*]] = type { ptr }
+
+// Check that parameters of user-defined setters are destructed.
+
+// CHECK-LABEL: define internal void @"\01-[C setP1:]"(
+// CHECK: %[[S0:.*]] = alloca %[[STRUCT_S0]], align 8
+// CHECK: call void @__copy_assignment_8_8_s0(ptr %{{.*}}, ptr %[[S0]])
+// CHECK: call void @__destructor_8_s0(ptr %[[S0]])
+
+// CHECK: define internal i64 @"\01-[C nonatomic]"(ptr noundef %[[SELF:.*]], {{.*}})
+// CHECK: %[[RETVAL:.*]] = alloca %[[STRUCT_S0]], align 8
+// CHECK: %[[SELF_ADDR:.*]] = alloca ptr, align 8
+// CHECK: store ptr %[[SELF]], ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load ptr, ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[IVAR:.*]] = load i32, ptr @"OBJC_IVAR_$_C._nonatomic", align 8
+// CHECK: %[[IVAR_CONV:.*]] = sext i32 %[[IVAR]] to i64
+// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[V0]], i64 %[[IVAR_CONV]]
+// CHECK: call void @__copy_constructor_8_8_s0(ptr %[[RETVAL]], ptr %[[ADD_PTR]])
+// CHECK-NOT: call
+// CHECK: ret i64
+
+// CHECK: define internal void @"\01-[C setNonatomic:]"(ptr noundef %[[SELF:.*]], {{.*}}, i64 %[[NONATOMIC_COERCE:.*]])
+// CHECK: %[[NONATOMIC:.*]] = alloca %[[STRUCT_S0]], align 8
+// CHECK: %[[SELF_ADDR:.*]] = alloca ptr, align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_S0]], ptr %[[NONATOMIC]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[NONATOMIC_COERCE]] to ptr
+// CHECK: store ptr %[[COERCE_VAL_IP]], ptr %[[COERCE_DIVE]], align 8
+// CHECK: store ptr %[[SELF]], ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load ptr, ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[IVAR:.*]] = load i32, ptr @"OBJC_IVAR_$_C._nonatomic", align 8
+// CHECK: %[[IVAR_CONV:.*]] = sext i32 %[[IVAR]] to i64
+// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[V0]], i64 %[[IVAR_CONV]]
+// CHECK: call void @__move_assignment_8_8_s0(ptr %[[ADD_PTR]], ptr %[[NONATOMIC]])
+// CHECK-NOT: call
+// CHECK: ret void
+
+// CHECK-LABEL: define internal i64 @"\01-[C atomic0]"(
+// 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