[IRGen] Fix a bug where an argument wasn't annotated with sret (#71459)

hjyamauchi · hjyamauchi · commit fd3ad8d2d9fb · 2024-11-11T17:15:20.000-08:00
Fix a bug in expandExternalSignatureTypes where it wasn't annotating a function call parameter type with sret when the result was being returned indirectly. The bug was causing calls to ObjC methods that return their results indirectly to crash. Additionally, fix the return type for C++ constructors computed in expandExternalSignatureTypes. Previously, the return type was always void even on targets that require constructors to return this (e.g., Apple arm64), which was causing C++ constructor thunks to be emitted needlessly. Resolves rdar://121618707 Cherrypick commit b3f302b Cherrypick PR #71459
diff --git a/lib/IRGen/GenCall.cpp b/lib/IRGen/GenCall.cpp
@@ -24,6 +24,7 @@
 #include "swift/SIL/SILModule.h"
 #include "swift/SIL/SILType.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/GlobalDecl.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
@@ -420,6 +421,10 @@ namespace {
     IRGenModule &IGM;
     CanSILFunctionType FnType;
     bool forStaticCall = false; // Used for objc_method (direct call or not).
+
+    // Indicates this is a c++ constructor call.
+    const clang::CXXConstructorDecl *cxxCtorDecl = nullptr;
+
   public:
     SmallVector<llvm::Type*, 8> ParamIRTypes;
     llvm::Type *ResultIRType = nullptr;
@@ -434,8 +439,10 @@ namespace {
     FunctionPointerKind FnKind;
 
     SignatureExpansion(IRGenModule &IGM, CanSILFunctionType fnType,
-                       FunctionPointerKind fnKind, bool forStaticCall = false)
-        : IGM(IGM), FnType(fnType), forStaticCall(forStaticCall), FnKind(fnKind) {}
+                       FunctionPointerKind fnKind, bool forStaticCall = false,
+                       const clang::CXXConstructorDecl *cxxCtorDecl = nullptr)
+        : IGM(IGM), FnType(fnType), forStaticCall(forStaticCall),
+          cxxCtorDecl(cxxCtorDecl), FnKind(fnKind) {}
 
     /// Expand the components of the primary entrypoint of the function type.
     void expandFunctionType(
@@ -460,7 +467,7 @@ namespace {
 
   private:
     const TypeInfo &expand(SILParameterInfo param);
-    llvm::Type *addIndirectResult();
+    llvm::Type *addIndirectResult(SILType resultType);
 
     SILFunctionConventions getSILFuncConventions() const {
       return SILFunctionConventions(FnType, IGM.getSILModule());
@@ -514,9 +521,7 @@ namespace {
 } // end namespace irgen
 } // end namespace swift
 
-llvm::Type *SignatureExpansion::addIndirectResult() {
-  auto resultType = getSILFuncConventions().getSILResultType(
-      IGM.getMaximalTypeExpansionContext());
+llvm::Type *SignatureExpansion::addIndirectResult(SILType resultType) {
   const TypeInfo &resultTI = IGM.getTypeInfo(resultType);
   auto storageTy = resultTI.getStorageType();
   addIndirectResultAttributes(IGM, Attrs, ParamIRTypes.size(), claimSRet(),
@@ -913,7 +918,7 @@ SignatureExpansion::expandDirectResult() {
     auto &ti = IGM.getTypeInfo(resultType);
     auto &native = ti.nativeReturnValueSchema(IGM);
     if (native.requiresIndirect())
-      return std::make_pair(addIndirectResult(), nullptr);
+      return std::make_pair(addIndirectResult(resultType), nullptr);
 
     // Disable the use of sret if we have a non-trivial direct result.
     if (!native.empty()) CanUseSRet = false;
@@ -1345,26 +1350,28 @@ static bool doesClangExpansionMatchSchema(IRGenModule &IGM,
 void SignatureExpansion::expandExternalSignatureTypes() {
   assert(FnType->getLanguage() == SILFunctionLanguage::C);
 
-  // Convert the SIL result type to a Clang type.
-  auto clangResultTy =
-    IGM.getClangType(FnType->getFormalCSemanticResult(IGM.getSILModule()));
+  auto SILResultTy = [&]() {
+    if (FnType->getNumResults() == 0)
+      return SILType::getPrimitiveObjectType(IGM.Context.TheEmptyTupleType);
+
+    return SILType::getPrimitiveObjectType(
+        FnType->getSingleResult().getReturnValueType(
+            IGM.getSILModule(), FnType, TypeExpansionContext::minimal()));
+  }();
+
+  // Convert the SIL result type to a Clang type. If this is for a c++
+  // constructor, use 'void' as the return type to arrange the function type.
+  auto clangResultTy = IGM.getClangType(
+      cxxCtorDecl
+          ? SILType::getPrimitiveObjectType(IGM.Context.TheEmptyTupleType)
+          : SILResultTy);
 
   // Now convert the parameters to Clang types.
   auto params = FnType->getParameters();
 
   SmallVector<clang::CanQualType,4> paramTys;
   auto const &clangCtx = IGM.getClangASTContext();
 
-  bool formalIndirectResult = FnType->getNumResults() > 0 &&
-                              FnType->getSingleResult().isFormalIndirect();
-  if (formalIndirectResult) {
-    auto resultType = getSILFuncConventions().getSingleSILResultType(
-        IGM.getMaximalTypeExpansionContext());
-    auto clangTy =
-        IGM.getClangASTContext().getPointerType(IGM.getClangType(resultType));
-    paramTys.push_back(clangTy);
-  }
-
   switch (FnType->getRepresentation()) {
   case SILFunctionTypeRepresentation::ObjCMethod: {
     // ObjC methods take their 'self' argument first, followed by an
@@ -1393,7 +1400,11 @@ void SignatureExpansion::expandExternalSignatureTypes() {
   }
 
   case SILFunctionTypeRepresentation::CFunctionPointer:
-    // No implicit arguments.
+    if (cxxCtorDecl) {
+      auto clangTy = IGM.getClangASTContext().getPointerType(
+          IGM.getClangType(SILResultTy));
+      paramTys.push_back(clangTy);
+    }
     break;
 
   case SILFunctionTypeRepresentation::Thin:
@@ -1417,6 +1428,7 @@ void SignatureExpansion::expandExternalSignatureTypes() {
 
   // Generate function info for this signature.
   auto extInfo = clang::FunctionType::ExtInfo();
+
   auto &FI = clang::CodeGen::arrangeFreeFunctionCall(IGM.ClangCodeGen->CGM(),
                                              clangResultTy, paramTys, extInfo,
                                              clang::CodeGen::RequiredArgs::All);
@@ -1427,6 +1439,14 @@ void SignatureExpansion::expandExternalSignatureTypes() {
 
   auto &returnInfo = FI.getReturnInfo();
 
+#ifndef NDEBUG
+  bool formalIndirectResult = FnType->getNumResults() > 0 &&
+                              FnType->getSingleResult().isFormalIndirect();
+  assert(
+      (cxxCtorDecl || !formalIndirectResult || returnInfo.isIndirect()) &&
+      "swift and clang disagree on whether the result is returned indirectly");
+#endif
+
   // Does the result need an extension attribute?
   if (returnInfo.isExtend()) {
     bool signExt = clangResultTy->hasSignedIntegerRepresentation();
@@ -1531,16 +1551,18 @@ void SignatureExpansion::expandExternalSignatureTypes() {
 
   // If we return indirectly, that is the first parameter type.
   if (returnInfo.isIndirect()) {
+    auto resultType = getSILFuncConventions().getSingleSILResultType(
+        IGM.getMaximalTypeExpansionContext());
     if (IGM.Triple.isWindowsMSVCEnvironment() &&
         FnType->getRepresentation() ==
             SILFunctionTypeRepresentation::CXXMethod) {
       // Windows ABI places `this` before the
       // returned indirect values.
       emitArg(0);
       firstParamToLowerNormally = 1;
-      addIndirectResult();
+      addIndirectResult(resultType);
     } else
-      addIndirectResult();
+      addIndirectResult(resultType);
   }
 
   // Use a special IR type for passing block pointers.
@@ -1554,7 +1576,12 @@ void SignatureExpansion::expandExternalSignatureTypes() {
   for (auto i : indices(paramTys).slice(firstParamToLowerNormally))
     emitArg(i);
 
-  if (returnInfo.isIndirect() || returnInfo.isIgnore()) {
+  if (cxxCtorDecl) {
+    ResultIRType = cast<llvm::Function>(IGM.getAddrOfClangGlobalDecl(
+                                            {cxxCtorDecl, clang::Ctor_Complete},
+                                            (ForDefinition_t) false))
+                       ->getReturnType();
+  } else if (returnInfo.isIndirect() || returnInfo.isIgnore()) {
     ResultIRType = IGM.VoidTy;
   } else {
     ResultIRType = returnInfo.getCoerceToType();
@@ -1874,7 +1901,7 @@ void SignatureExpansion::expandAsyncEntryType() {
   auto &ti = IGM.getTypeInfo(resultType);
   auto &native = ti.nativeReturnValueSchema(IGM);
   if (native.requiresIndirect())
-    addIndirectResult();
+    addIndirectResult(resultType);
 
   // Add the indirect result types.
   expandIndirectResults();
@@ -2035,10 +2062,11 @@ Signature SignatureExpansion::getSignature() {
 
 Signature Signature::getUncached(IRGenModule &IGM,
                                  CanSILFunctionType formalType,
-                                 FunctionPointerKind fpKind,
-                                 bool forStaticCall) {
+                                 FunctionPointerKind fpKind, bool forStaticCall,
+                                 const clang::CXXConstructorDecl *cxxCtorDecl) {
   GenericContextScope scope(IGM, formalType->getInvocationGenericSignature());
-  SignatureExpansion expansion(IGM, formalType, fpKind, forStaticCall);
+  SignatureExpansion expansion(IGM, formalType, fpKind, forStaticCall,
+                               cxxCtorDecl);
   expansion.expandFunctionType();
   return expansion.getSignature();
 }
@@ -3083,7 +3111,13 @@ llvm::CallBase *IRBuilder::CreateCallOrInvoke(
     for (unsigned argIndex = 0; argIndex < func->arg_size(); ++argIndex) {
       if (func->hasParamAttribute(argIndex, llvm::Attribute::StructRet)) {
         llvm::AttrBuilder builder(func->getContext());
-        builder.addStructRetAttr(func->getParamStructRetType(argIndex));
+        // See if there is a sret parameter in the signature. There are cases
+        // where the called function has a sret parameter, but the signature
+        // doesn't (e.g., noreturn functions).
+        llvm::Type *ty = attrs.getParamStructRetType(argIndex);
+        if (!ty)
+          ty = func->getParamStructRetType(argIndex);
+        builder.addStructRetAttr(ty);
         attrs = attrs.addParamAttributes(func->getContext(), argIndex, builder);
       }
       if (func->hasParamAttribute(argIndex, llvm::Attribute::ByVal)) {
@@ -3797,11 +3831,13 @@ static void externalizeArguments(IRGenFunction &IGF, const Callee &callee,
     params = params.drop_back();
   }
 
-  if (fnType->getNumResults() > 0 &&
-             fnType->getSingleResult().isFormalIndirect()) {
-    // Ignore the indirect result parameter.
+  bool formalIndirectResult = fnType->getNumResults() > 0 &&
+                              fnType->getSingleResult().isFormalIndirect();
+
+  // If clang returns directly and swift returns indirectly, this must be a c++
+  // constructor call. In that case, skip the "self" param.
+  if (!FI.getReturnInfo().isIndirect() && formalIndirectResult)
     firstParam += 1;
-  }
 
   for (unsigned i = firstParam; i != paramEnd; ++i) {
     auto clangParamTy = FI.arg_begin()[i].type;
diff --git a/lib/IRGen/GenDecl.cpp b/lib/IRGen/GenDecl.cpp
@@ -3376,7 +3376,10 @@ llvm::Constant *swift::irgen::emitCXXConstructorThunkIfNeeded(
       emitCXXConstructorCall(subIGF, ctor, ctorFnType, ctorAddress, Args);
   if (isa<llvm::InvokeInst>(call))
     IGM.emittedForeignFunctionThunksWithExceptionTraps.insert(thunk);
-  subIGF.Builder.CreateRetVoid();
+  if (ctorFnType->getReturnType()->isVoidTy())
+    subIGF.Builder.CreateRetVoid();
+  else
+    subIGF.Builder.CreateRet(call);
 
   return thunk;
 }
@@ -3448,7 +3451,7 @@ llvm::Function *IRGenModule::getAddrOfSILFunction(
     }
 
     if (cxxCtor) {
-      Signature signature = getSignature(f->getLoweredFunctionType());
+      Signature signature = getSignature(f->getLoweredFunctionType(), cxxCtor);
 
       // The thunk has private linkage, so it doesn't need to have a predictable
       // mangled name -- we just need to make sure the name is unique.
diff --git a/lib/IRGen/GenFunc.cpp b/lib/IRGen/GenFunc.cpp
@@ -125,11 +125,15 @@ namespace {
     const CanSILFunctionType FormalType;
 
     mutable Signature TheSignature;
+    mutable Signature TheCXXConstructorSignature;
 
   public:
     FuncSignatureInfo(CanSILFunctionType formalType)
       : FormalType(formalType) {}
 
+    Signature
+    getCXXConstructorSignature(const clang::CXXConstructorDecl *cxxCtorDecl,
+                               IRGenModule &IGM) const;
     Signature getSignature(IRGenModule &IGM) const;
   };
 
@@ -677,6 +681,20 @@ Signature FuncSignatureInfo::getSignature(IRGenModule &IGM) const {
   return TheSignature;
 }
 
+Signature FuncSignatureInfo::getCXXConstructorSignature(
+    const clang::CXXConstructorDecl *cxxCtorDecl, IRGenModule &IGM) const {
+  // If it's already been filled in, we're done.
+  if (TheCXXConstructorSignature.isValid())
+    return TheCXXConstructorSignature;
+
+  // Update the cache and return.
+  TheCXXConstructorSignature =
+      Signature::getUncached(IGM, FormalType, FunctionPointerKind(FormalType),
+                             /*forStaticCall*/ false, cxxCtorDecl);
+  assert(TheCXXConstructorSignature.isValid());
+  return TheCXXConstructorSignature;
+}
+
 Signature ObjCFuncSignatureInfo::getDirectSignature(IRGenModule &IGM) const {
   // If it's already been filled in, we're done.
   if (TheDirectSignature.isValid())
@@ -711,13 +729,17 @@ getFuncSignatureInfoForLowered(IRGenModule &IGM, CanSILFunctionType type) {
   llvm_unreachable("bad function type representation");
 }
 
-Signature IRGenModule::getSignature(CanSILFunctionType type) {
-  return getSignature(type, FunctionPointerKind(type));
+Signature
+IRGenModule::getSignature(CanSILFunctionType type,
+                          const clang::CXXConstructorDecl *cxxCtorDecl) {
+  return getSignature(type, FunctionPointerKind(type), /*forStaticCall*/ false,
+                      cxxCtorDecl);
 }
 
-Signature IRGenModule::getSignature(CanSILFunctionType type,
-                                    FunctionPointerKind kind,
-                                    bool forStaticCall) {
+Signature
+IRGenModule::getSignature(CanSILFunctionType type, FunctionPointerKind kind,
+                          bool forStaticCall,
+                          const clang::CXXConstructorDecl *cxxCtorDecl) {
   // Don't bother caching if we're working with a special kind.
   if (kind.isSpecial())
     return Signature::getUncached(*this, type, kind);
@@ -729,6 +751,10 @@ Signature IRGenModule::getSignature(CanSILFunctionType type,
     auto &objcSigInfo = static_cast<const ObjCFuncSignatureInfo &>(sigInfo);
     return objcSigInfo.getDirectSignature(*this);
   }
+
+  if (cxxCtorDecl)
+    return sigInfo.getCXXConstructorSignature(cxxCtorDecl, *this);
+
   return sigInfo.getSignature(*this);
 }
 
diff --git a/lib/IRGen/GenStruct.cpp b/lib/IRGen/GenStruct.cpp
@@ -628,7 +628,7 @@ namespace {
       auto clangFnAddr =
           IGF.IGM.getAddrOfClangGlobalDecl(globalDecl, NotForDefinition);
       auto callee = cast<llvm::Function>(clangFnAddr->stripPointerCasts());
-      Signature signature = IGF.IGM.getSignature(fnType);
+      Signature signature = IGF.IGM.getSignature(fnType, copyConstructor);
       std::string name = "__swift_cxx_copy_ctor" + callee->getName().str();
       auto *origClangFnAddr = clangFnAddr;
       clangFnAddr = emitCXXConstructorThunkIfNeeded(
diff --git a/lib/IRGen/IRGenModule.h b/lib/IRGen/IRGenModule.h
@@ -1513,10 +1513,13 @@ private:                                                                       \
   void finalizeClangCodeGen();
   void finishEmitAfterTopLevel();
 
-  Signature getSignature(CanSILFunctionType fnType);
-  Signature getSignature(CanSILFunctionType fnType,
-                         FunctionPointerKind kind,
-                         bool forStaticCall = false);
+  Signature
+  getSignature(CanSILFunctionType fnType,
+               const clang::CXXConstructorDecl *cxxCtorDecl = nullptr);
+  Signature
+  getSignature(CanSILFunctionType fnType, FunctionPointerKind kind,
+               bool forStaticCall = false,
+               const clang::CXXConstructorDecl *cxxCtorDecl = nullptr);
   llvm::FunctionType *getFunctionType(CanSILFunctionType type,
                                       llvm::AttributeList &attrs,
                                       ForeignFunctionInfo *foreignInfo=nullptr);
diff --git a/lib/IRGen/IRGenSIL.cpp b/lib/IRGen/IRGenSIL.cpp
@@ -2892,8 +2892,13 @@ void IRGenSILFunction::visitFunctionRefBaseInst(FunctionRefBaseInst *i) {
   auto fnType = fn->getLoweredFunctionType();
 
   auto fpKind = irgen::classifyFunctionPointerKind(fn);
+  const clang::CXXConstructorDecl *cxxCtorDecl = nullptr;
 
-  auto sig = IGM.getSignature(fnType, fpKind, true /*forStaticCall*/);
+  if (auto *clangFnDecl = fn->getClangDecl())
+    cxxCtorDecl = dyn_cast<clang::CXXConstructorDecl>(clangFnDecl);
+
+  auto sig =
+      IGM.getSignature(fnType, fpKind, true /*forStaticCall*/, cxxCtorDecl);
 
   // Note that the pointer value returned by getAddrOfSILFunction doesn't
   // necessarily have element type sig.getType(), e.g. if it's imported.
diff --git a/lib/IRGen/Signature.h b/lib/IRGen/Signature.h
@@ -31,6 +31,7 @@ namespace llvm {
 }
 
 namespace clang {
+  class CXXConstructorDecl;
   namespace CodeGen {
     class CGFunctionInfo;    
   }
@@ -202,9 +203,10 @@ class Signature {
   /// This is a private detail of the implementation of
   /// IRGenModule::getSignature(CanSILFunctionType), which is what
   /// clients should generally be using.
-  static Signature getUncached(IRGenModule &IGM, CanSILFunctionType formalType,
-                               FunctionPointerKind kind,
-                               bool forStaticCall = false);
+  static Signature
+  getUncached(IRGenModule &IGM, CanSILFunctionType formalType,
+              FunctionPointerKind kind, bool forStaticCall = false,
+              const clang::CXXConstructorDecl *cxxCtorDecl = nullptr);
 
   static SignatureExpansionABIDetails
   getUncachedABIDetails(IRGenModule &IGM, CanSILFunctionType formalType,
diff --git a/test/Interop/Cxx/class/method/methods-this-and-indirect-return-irgen-itanium.swift b/test/Interop/Cxx/class/method/methods-this-and-indirect-return-irgen-itanium.swift
@@ -12,6 +12,6 @@ public func use() -> CInt {
 
 // CHECK: %[[instance:.*]] = alloca %TSo10HasMethodsV
 // CHECK: %[[result:.*]] = alloca %TSo19NonTrivialInWrapperV
-// CHECK: call void @_ZN10HasMethods28nonConstPassThroughAsWrapperEi(ptr sret(%struct.NonTrivialInWrapper) %[[result]], ptr %[[instance]], i32 42)
+// CHECK: call void @_ZN10HasMethods28nonConstPassThroughAsWrapperEi(ptr noalias sret(%TSo19NonTrivialInWrapperV) %[[result]], ptr %[[instance]], i32 42)
 
 // CHECK: define {{.*}} void @_ZN10HasMethods28nonConstPassThroughAsWrapperEi(ptr noalias sret(%struct.NonTrivialInWrapper) {{.*}} %{{.*}}, ptr {{.*}} %{{.*}}, i32 noundef %{{.*}})
diff --git a/test/Interop/Cxx/class/method/methods-this-and-indirect-return-irgen-msvc.swift b/test/Interop/Cxx/class/method/methods-this-and-indirect-return-irgen-msvc.swift
diff --git a/test/Interop/Cxx/objc-correctness/sret.swift b/test/Interop/Cxx/objc-correctness/sret.swift
