Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 22, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
12bbc11
[IRGen] Fix a bug where an argument wasn't annotated with sret
ahatanaka Feb 1, 2024
e2081b9
Pass target triple
ahatanaka Feb 8, 2024
b65aaf8
Make sure the sret type matches the type of the pointee type.
ahatanaka Feb 8, 2024
6a83363
Fix tests
ahatanaka Feb 9, 2024
9ddee0f
Call arrangeCXXStructorDeclaration to arrange constructor signatures
ahatanaka Feb 9, 2024
d444a75
Compute the return type of c++ constructors without calling arrangeCX…
ahatanaka Feb 14, 2024
a87806b
Fix sret type in test case
ahatanaka Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 69 additions & 33 deletions lib/IRGen/GenCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,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"
Expand Down Expand Up @@ -428,6 +429,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;
Expand All @@ -442,8 +447,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(
Expand All @@ -468,7 +475,7 @@ namespace {

private:
const TypeInfo &expand(SILParameterInfo param);
llvm::Type *addIndirectResult();
llvm::Type *addIndirectResult(SILType resultType);

SILFunctionConventions getSILFuncConventions() const {
return SILFunctionConventions(FnType, IGM.getSILModule());
Expand Down Expand Up @@ -526,9 +533,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(),
Expand Down Expand Up @@ -925,7 +930,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;
Expand Down Expand Up @@ -1361,26 +1366,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the ABI for C++ constructors on Apple ARM64; they return this. We can ignore that result, and technically the call will probably succeed as a void call, but we should really arrange the function type correctly. Can we just use Clang as a library to arrange the constructor signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's CodeGenTypes::arrangeCXXStructorDeclaration, but that function isn't available in a header file swift has access to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding public function arrangeCXXStructorDeclaration to CodeGenABITypes.h so that it can be called to arrange the constructor signature. But that's causing an assertion to fail in externalizeArguments:

https://github.com/apple/swift/blob/main/lib/IRGen/GenCall.cpp#L4019

The cast fails because AddressOnlyCXXClangRecordTypeInfo is returned instead of a LoadableTypeInfo. The following function returns AddressOnlyCXXClangRecordTypeInfo:

https://github.com/apple/swift/blob/6dccf3d6679f442cafa9021de50cf677ac727b53/lib/IRGen/GenStruct.cpp#L1334

The type in question is StructWithCopyConstructorAndValue in test/Interop/Cxx/class/type-classification-non-trivial.swift and the assertion fails when the argument is passed to the constructor of StructWithCopyConstructorAndSubobjectCopyConstructorAndValue:

https://github.com/apple/swift/blob/6dccf3d6679f442cafa9021de50cf677ac727b53/test/Interop/Cxx/class/type-classification-non-trivial.swift#L29

I don't see the assertion fail if I revert the changes and stop using arrangeCXXStructorDeclaration. The difference is that arrangeCXXStructorDeclaration returns ABIArgInfo::Indirect for the argument whereas the previous approach (which passes a pointer type to arrangeFreeFunctionCall as the parameter type) returned ABIArgInfo::Direct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks likeAddressOnlyCXXClangRecordTypeInfo has methods like initializeWithCopy, which can possibly be used to initialize the temporary.

Copy link
Contributor

@rjmccall rjmccall Feb 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would introduce an extra copy of a non-trivial type, which we generally don't want to do in IRGen as part of making a call.

In this case, SIL type lowering already recognized that the type has to be passed indirectly as a parameter or return value and encoded that using an indirect SIL convention. You should be able to assert that indirect SIL parameters and results correspond to ABIArgInfo::Indirect in the Clang FunctionInfo. (The reverse isn't always true.)

: 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
Expand Down Expand Up @@ -1409,7 +1416,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:
Expand Down Expand Up @@ -1437,6 +1448,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);
Expand All @@ -1447,6 +1459,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();
Expand Down Expand Up @@ -1551,16 +1571,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.
Expand All @@ -1574,7 +1596,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();
Expand Down Expand Up @@ -1986,7 +2013,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();
Expand Down Expand Up @@ -2153,10 +2180,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();
}
Expand Down Expand Up @@ -3235,7 +3263,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)) {
Expand Down Expand Up @@ -3950,11 +3984,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;
Expand Down
7 changes: 5 additions & 2 deletions lib/IRGen/GenDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3511,7 +3511,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;
}
Expand Down Expand Up @@ -3583,7 +3586,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.
Expand Down
36 changes: 31 additions & 5 deletions lib/IRGen/GenFunc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down Expand Up @@ -674,6 +678,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())
Expand Down Expand Up @@ -712,13 +730,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);
Expand All @@ -730,6 +752,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);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/IRGen/GenStruct.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,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(
Expand Down
11 changes: 7 additions & 4 deletions lib/IRGen/IRGenModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1574,10 +1574,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);
Expand Down
7 changes: 6 additions & 1 deletion lib/IRGen/IRGenSIL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2969,8 +2969,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.
Expand Down
8 changes: 5 additions & 3 deletions lib/IRGen/Signature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ namespace llvm {
}

namespace clang {
class CXXConstructorDecl;
namespace CodeGen {
class CGFunctionInfo;
}
Expand Down Expand Up @@ -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,
Expand Down
Loading