[CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

[ahatanak]

Summary

This fixes PR45997 (https://bugs.llvm.org/show_bug.cgi?id=45997), which is caused by a bug that has existed since we started passing and returning C++ structs with ObjC strong pointer members (see https://reviews.llvm.org/D44908) or structs annotated with trivial_abi directly.

rdar://problem/63740936

[rjmccall]

This seems fine. I do wonder if the "real" bug is that this ought to be handled properly in EmitReturnFromThunk, but regardless, the fix seems acceptable.

Is there a similar bug with trivial_abi? Should we have a test case for that?

[ahatanak]

Add test case for trivial_abi.

[ahatanak]

In D82513#2142596, @rjmccall wrote:

This seems fine. I do wonder if the "real" bug is that this ought to be handled properly in EmitReturnFromThunk, but regardless, the fix seems acceptable.

Since EmitReturnFromThunk is used only by EmitCallAndReturnForThunk, I think it's better to just avoid the copy in the first place. Perhaps we should teach EmitReturnOfRValue to handle aggregates with non-trivial copy or move assignments correctly, but it seems that it won't have any effect once we commit the changes in this patch.

[rjmccall]

I agree that avoiding the copy is best. However, at the very least, if that function isn't going to handle the aggregate case correctly, it should assert that it isn't in it.

Otherwise this LGTM.

[ahatanak]

Assert in EmitReturnFromThunk that the result type isn't an aggregate.

[rjmccall]

Thanks! LGTM.

clang/lib/CodeGen/CGCXXABI.cpp

 void CGCXXABI::setCXXABIThisValue(CodeGenFunction &CGF, llvm::Value *ThisPtr) {
   /// Initialize the 'this' slot.
   assert(getThisDecl(CGF) && "no 'this' variable for function");
   CGF.CXXABIThisValue = ThisPtr;
 }
 
 void CGCXXABI::EmitReturnFromThunk(CodeGenFunction &CGF,
                                    RValue RV, QualType ResultType) {
+  assert(!hasAggregateEvaluationKind(ResultType) && "cannot handle aggregates");
   CGF.EmitReturnOfRValue(RV, ResultType);
 }
 
 CharUnits CGCXXABI::GetArrayCookieSize(const CXXNewExpr *expr) {
   if (!requiresArrayCookie(expr))
     return CharUnits::Zero();
   return getArrayCookieSizeImpl(expr->getAllocatedType());
 }

clang/lib/CodeGen/CGVTables.cpp

#endif
   // Determine whether we have a return value slot to use.
   QualType ResultType = CGM.getCXXABI().HasThisReturn(CurGD)
                             ? ThisType
                             : CGM.getCXXABI().hasMostDerivedReturn(CurGD)
                                   ? CGM.getContext().VoidPtrTy
                                   : FPT->getReturnType();
   ReturnValueSlot Slot;
   if (!ResultType->isVoidType() &&
-      CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect)
+      (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect ||
+       hasAggregateEvaluationKind(ResultType)))
     Slot = ReturnValueSlot(ReturnValue, ResultType.isVolatileQualified(),
                            /*IsUnused=*/false, /*IsExternallyDestructed=*/true);
 
   // Now emit our call.
   llvm::CallBase *CallOrInvoke;
   RValue RV = EmitCall(*CurFnInfo, CGCallee::forDirect(Callee, CurGD), Slot,
                        CallArgs, &CallOrInvoke);
 

clang/test/CodeGenCXX/trivial_abi.cpp

struct HasTrivial {
   Trivial m;
 };
 
 struct HasNonTrivial {
   Small s;
   NonTrivial m;
 };
 
+struct B0 {
+  virtual Small m0();
+};
+
+struct B1 {
+  virtual Small m0();
+};
+
+struct D0 : B0, B1 {
+  Small m0() override;
+};
+
+// CHECK-LABEL: define i64 @_ZThn8_N2D02m0Ev(
+// CHECK: %[[RETVAL:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[CALL:.*]] = tail call i64 @_ZN2D02m0Ev(
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[RETVAL]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[CALL]] to i32*
+// CHECK: store i32* %[[COERCE_VAL_IP]], i32** %[[COERCE_DIVE]], align 8
+// CHECK: %[[COERCE_DIVE2:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[RETVAL]], i32 0, i32 0
+// CHECK: %[[V3:.*]] = load i32*, i32** %[[COERCE_DIVE2]], align 8
+// CHECK: %[[COERCE_VAL_PI:.*]] = ptrtoint i32* %[[V3]] to i64
+// CHECK: ret i64 %[[COERCE_VAL_PI]]
+
+Small D0::m0() { return {}; }
+
 // CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
 // CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
 // CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[A]], i32 0, i32 0
 // CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to i32*
 // CHECK: store i32* %[[COERCE_VAL_IP]], i32** %[[COERCE_DIVE]], align 8
 // CHECK: %[[CALL:.*]] = call %[[STRUCT_SMALL]]* @_ZN5SmallD1Ev(%[[STRUCT_SMALL]]* %[[A]])
 // CHECK: ret void
 // CHECK: }

clang/test/CodeGenObjCXX/objc-struct-cxx-abi.mm

 
 // CHECK: define void @_Z26testCallContainsNonTrivialP18ContainsNonTrivial(
 // CHECK: call void @_Z27testParamContainsNonTrivial18ContainsNonTrivial(%[[STRUCT_CONTAINSNONTRIVIAL]]* %{{.*}})
 // CHECK: call %struct.ContainsNonTrivial* @_ZN18ContainsNonTrivialD1Ev(%[[STRUCT_CONTAINSNONTRIVIAL]]* %{{.*}})
 
 void testCallContainsNonTrivial(ContainsNonTrivial *a) {
   testParamContainsNonTrivial(*a);
 }
+
+namespace testThunk {
+
+// CHECK-LABEL: define i64 @_ZThn8_N9testThunk2D02m0Ev(
+// CHECK: %[[RETVAL:.*]] = alloca %[[STRUCT_STRONG]], align 8
+// CHECK: %[[CALL:.*]] = tail call i64 @_ZN9testThunk2D02m0Ev(
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_STRONG]], %[[STRUCT_STRONG]]* %[[RETVAL]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[CALL]] to i8*
+// CHECK: store i8* %[[COERCE_VAL_IP]], i8** %[[COERCE_DIVE]], align 8
+// CHECK: %[[COERCE_DIVE2:.*]] = getelementptr inbounds %[[STRUCT_STRONG]], %[[STRUCT_STRONG]]* %[[RETVAL]], i32 0, i32 0
+// CHECK: %[[V3:.*]] = load i8*, i8** %[[COERCE_DIVE2]], align 8
+// CHECK: %[[COERCE_VAL_PI:.*]] = ptrtoint i8* %[[V3]] to i64
+// CHECK: ret i64 %[[COERCE_VAL_PI]]
+
+struct B0 {
+  virtual Strong m0();
+};
+
+struct B1 {
+  virtual Strong m0();
+};
+
+struct D0 : B0, B1 {
+  Strong m0() override;
+};
+
+Strong D0::m0() { return {}; }
+
+}