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

[ahatanaka]

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

[ahatanaka]

@swift-ci please smoke test

[ahatanaka]

@swift-ci please smoke test

[ahatanaka]

I think ptr noalias nocapture sret(%[[TSO1SV]]) should be emitted here. Should getSingleSILResultType be called in SignatureExpansion::addIndirectResult instead of getSILResultType?

[aschwaighofer]

the sret argument type should match the pointed to type of the argument. yes, SWIFT_OPAQUE is wrong.

[ahatanaka]

I suppose the type in CHECK: invoke void @_Z4getSi(ptr noalias nocapture sret(%[[CLASS_S]]) %[[V5]], i32 1) should be TSO1SV too instead of CLASS_S? But if I make that change, the called function's type is going to differ from the declared function clang generates, which is declare void @_Z4getSi(ptr sret([[CLASS_S]]), because the sret type is different. I don't think we can change the type of the alloca?

[aschwaighofer]

Is [[CLASS_S]] the C type and TSO1SV the Swift imported version of the C type? I would think if the layout is equivalent we should be okay i.e it need not be literally the same type.

[ahatanaka]

Yes, that's correct.

[rjmccall]

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?

[ahatanaka]

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

[ahatanaka]

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.

[ahatanaka]

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

[rjmccall]

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.)

[ahatanaka]

@swift-ci please smoke test

[ahatanaka]

@swift-ci please smoke test

[ahatanaka]

I think there's a preexisting bug here where a call to a C++ constructor thunk won't be emitted after the first time the C++ constructor is called: https://github.com/apple/swift/pull/71459/files#diff-32a58f88e8e7d691eb190fea3d41e8eade0536a5e8e931cc68f916236e69cefeL3555

If a C++ constructor that requires a thunk (for example, on windows) is called the first time, the thunk is created and returned, and that's fine. But if it's called the second time, this function will return the C++ constructor instead of its thunk because the constructor is already in the module.

[ahatanaka]

#71790 fixes the preexisting bug. The fix is needed before I can make the changes necessary to make expandExternalSignatureType return the correct return type for C++ constructors.

[ahatanaka]

@swift-ci please smoke test

[ahatanaka]

The updated patch calls getAddrOfClangGlobalDecl instead of calling a new clang API (arrangeCXXStructorDeclaration) to get the return types of c++ constructors.

Fixing the return type prevents thunks for c++ constructors from being emitted when they aren't needed. Previously, thunks were emitted on Apple arm64 because there was a mismatch between the signature computed in SignatureExpansion::expandExternalSignatureTypes and the actual signature of the constructor in llvm IR.

[ahatanaka]

@swift-ci please smoke test

[rjmccall]

I think the long-term fix here is probably to add a cxx_method or cxx_constructor representation to SIL function types, but this is a reasonable short-term fix.