Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

[hans]

Summary

Thunks that return member pointers via sret are broken due to using temporary storage for the return value on the stack, and then passing that pointer to a tail call, violating the rule that a tail call can't access allocas in the caller (see bug).

Since r90526 (Eli), we put aggregate return values directly in the sret slot, but this doesn't apply to member pointers which are considered scalar.

Maybe I'm missing something subtle, but why not always use the sret slot directly for indirect return values? This patch does that, fixing the thunks.
Please take a look.

[hans]

-llvm-commits
+cfe-commits

[rjmccall]

That seems reasonable.

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 &&
-      !hasScalarEvaluationKind(CurFnInfo->getReturnType()))
+      CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect)
     Slot = ReturnValueSlot(ReturnValue, ResultType.isVolatileQualified());
 
   // Now emit our call.
   llvm::Instruction *CallOrInvoke;
   CGCallee Callee = CGCallee::forDirect(CalleePtr, CurGD);
   RValue RV = EmitCall(*CurFnInfo, Callee, Slot, CallArgs, &CallOrInvoke);
 
   // Consider return adjustment if we have ThunkInfo.

lib/CodeGen/CodeGenFunction.cpp

#undef SANITIZER
 
   if (RetTy->isVoidType()) {
     // Void type; nothing to return.
     ReturnValue = Address::invalid();
 
     // Count the implicit return.
     if (!endsWithReturn(D))
       ++NumReturnExprs;
-  } else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect &&
-             !hasScalarEvaluationKind(CurFnInfo->getReturnType())) {
-    // Indirect aggregate return; emit returned value directly into sret slot.
+  } else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect) {
+    // Indirect return; emit returned value directly into sret slot.
     // This reduces code size, and affects correctness in C++.
     auto AI = CurFn->arg_begin();
     if (CurFnInfo->getReturnInfo().isSRetAfterThis())
       ++AI;
     ReturnValue = Address(&*AI, CurFnInfo->getReturnInfo().getIndirectAlign());
   } else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::InAlloca &&
              !hasScalarEvaluationKind(CurFnInfo->getReturnType())) {
     // Load the sret pointer from the argument struct and return into that.

test/CodeGenCXX/thunk-returning-memptr.cpp

+// RUN: %clang_cc1 -triple=i686 -emit-llvm -o - %s | FileCheck %s
+
+
+struct X;
+typedef void (X::*memptr)();
+
+struct A {
+  virtual memptr f();
+};
+
+struct B {
+  virtual memptr f();
+};
+
+struct C : A, B {
+  C();
+  memptr f() override __attribute__((noinline)) { return nullptr; };
+};
+
+C::C() {}
+
+// Make sure the member pointer is returned from the thunk via the return slot.
+// Because of the tail call, the return value cannot be copied into a local
+// alloca. (PR39901)
+
+// CHECK-LABEL: define linkonce_odr void @_ZThn4_N1C1fEv({ i32, i32 }* noalias sret %agg.result, %struct.C* %this)
+// CHECK: tail call void @_ZN1C1fEv({ i32, i32 }* sret %agg.result