Skip to content
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.

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

[cxx-interop] Use user defined copy constructor to copy C++ objects. #32378

Merged
merged 1 commit into from
Feb 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 53 additions & 16 deletions lib/ClangImporter/ImportDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "swift/AST/Pattern.h"
#include "swift/AST/PrettyStackTrace.h"
#include "swift/AST/ProtocolConformance.h"
#include "swift/AST/SemanticAttrs.h"
#include "swift/AST/Stmt.h"
#include "swift/AST/TypeCheckRequests.h"
#include "swift/AST/Types.h"
Expand All @@ -47,11 +48,10 @@
#include "swift/Parse/Lexer.h"
#include "swift/Parse/Parser.h"
#include "swift/Strings.h"

#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
#include "clang/AST/DeclObjCCommon.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjCCommon.h"
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/Specifiers.h"
#include "clang/Basic/TargetInfo.h"
Expand Down Expand Up @@ -3481,6 +3481,11 @@ namespace {
ctors.push_back(createDefaultConstructor(Impl, result));
}

// We can assume that it is possible to correctly construct the object by
// simply initializing its member variables to arbitrary supplied values
// only when the same is possible in C++. While we could check for that
// exactly, checking whether the C++ class is an aggregate
// (C++ [dcl.init.aggr]) has the same effect.
bool isAggregate = !cxxRecordDecl || cxxRecordDecl->isAggregate();
if (hasReferenceableFields && hasMemberwiseInitializer && isAggregate) {
// The default zero initializer suppresses the implicit value
Expand Down Expand Up @@ -3513,8 +3518,14 @@ namespace {
result->setIsCxxNonTrivial(!cxxRecordDecl->isTriviallyCopyable());

for (auto ctor : cxxRecordDecl->ctors()) {
if (ctor->isCopyConstructor() &&
(ctor->isDeleted() || ctor->getAccess() != clang::AS_public)) {
if (ctor->isCopyConstructor()) {
// If we have no way of copying the type we can't import the class
// at all because we cannot express the correct semantics as a swift
// struct.
if (ctor->isDeleted() || ctor->getAccess() != clang::AS_public)
return nullptr;
}
if (ctor->getAccess() != clang::AS_public) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to admit I don't understand the logic that existed here before this PR, and which was introduced by #31707.

AFAICT, the definition of "trivially copyable" says nothing about the access level of the copy constructor. It does say something about the copy constructor being deleted, namely that this is compatible with the class being trivially copyable.

Beyond that, I'm not sure why simply taking the result of cxxRecordDecl->isTriviallyCopyable() isn't sufficient here.

@MForster I think you introduced this logic -- can you comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[class.prop]/p1 defines a trivially copyable class as a class, "that has at least one eligible copy constructor..." I think the important word there is eligible. A private copy constructor is not eligible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's how C++ defines "eligible":

https://eel.is/c++draft/class#special-6

Copy link
Contributor

Choose a reason for hiding this comment

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

True. However, I think we have to still check for public access level, since it would be otherwise quite weird that Swift can use a private constructor -- even if it is trivial.

result->setIsCxxNonTrivial(true);
break;
}
Expand All @@ -3540,19 +3551,45 @@ namespace {
return VisitRecordDecl(decl);

auto &clangSema = Impl.getClangSema();
// Make Clang define the implicit default constructor if the class needs
// it. Make sure we only do this if the class has been fully defined and
// we're not in a dependent context (this is equivalent to the logic in
// CanDeclareSpecialMemberFunction in Clang's SemaLookup.cpp).
// Make Clang define any implicit constructors it may need (copy,
// default). Make sure we only do this if the class has been fully defined
// and we're not in a dependent context (this is equivalent to the logic
// in CanDeclareSpecialMemberFunction in Clang's SemaLookup.cpp).
if (decl->getDefinition() && !decl->isBeingDefined() &&
!decl->isDependentContext() &&
decl->needsImplicitDefaultConstructor()) {
clang::CXXConstructorDecl *ctor =
clangSema.DeclareImplicitDefaultConstructor(
const_cast<clang::CXXRecordDecl *>(decl->getDefinition()));
if (!ctor->isDeleted())
clangSema.DefineImplicitDefaultConstructor(clang::SourceLocation(),
ctor);
!decl->isDependentContext()) {
if (decl->needsImplicitDefaultConstructor()) {
clang::CXXConstructorDecl *ctor =
clangSema.DeclareImplicitDefaultConstructor(
const_cast<clang::CXXRecordDecl *>(decl->getDefinition()));
if (!ctor->isDeleted())
clangSema.DefineImplicitDefaultConstructor(clang::SourceLocation(),
ctor);
}
clang::CXXConstructorDecl *copyCtor = nullptr;
if (decl->needsImplicitCopyConstructor()) {
zoecarver marked this conversation as resolved.
Show resolved Hide resolved
copyCtor = clangSema.DeclareImplicitCopyConstructor(
const_cast<clang::CXXRecordDecl *>(decl));
} else {
// We may have a defaulted copy constructor that needs to be defined.
// Try to find it.
for (auto methods : decl->methods()) {
if (auto declCtor = dyn_cast<clang::CXXConstructorDecl>(methods)) {
if (declCtor->isCopyConstructor() && declCtor->isDefaulted() &&
declCtor->getAccess() == clang::AS_public &&
!declCtor->isDeleted() &&
// Note: we use "doesThisDeclarationHaveABody" here because
// that's what "DefineImplicitCopyConstructor" checks.
!declCtor->doesThisDeclarationHaveABody()) {
copyCtor = declCtor;
break;
}
}
}
}
if (copyCtor) {
clangSema.DefineImplicitCopyConstructor(clang::SourceLocation(),
copyCtor);
}
}

return VisitRecordDecl(decl);
Expand Down
42 changes: 17 additions & 25 deletions lib/IRGen/GenDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2788,20 +2788,10 @@ void IRGenModule::emitDynamicReplacementOriginalFunctionThunk(SILFunction *f) {
IGF.Builder.CreateRet(Res);
}

/// If the calling convention for `ctor` doesn't match the calling convention
/// that we assumed for it when we imported it as `initializer`, emit and
/// return a thunk that conforms to the assumed calling convention. The thunk
/// is marked `alwaysinline`, so it doesn't generate any runtime overhead.
/// If the assumed calling convention was correct, just return `ctor`.
///
/// See also comments in CXXMethodConventions in SIL/IR/SILFunctionType.cpp.
static llvm::Constant *
emitCXXConstructorThunkIfNeeded(IRGenModule &IGM, SILFunction *initializer,
const clang::CXXConstructorDecl *ctor,
const LinkEntity &entity,
llvm::Constant *ctorAddress) {
Signature signature = IGM.getSignature(initializer->getLoweredFunctionType());

llvm::Constant *swift::irgen::emitCXXConstructorThunkIfNeeded(
IRGenModule &IGM, Signature signature,
const clang::CXXConstructorDecl *ctor, StringRef name,
llvm::Constant *ctorAddress) {
llvm::FunctionType *assumedFnType = signature.getType();
llvm::FunctionType *ctorFnType =
cast<llvm::FunctionType>(ctorAddress->getType()->getPointerElementType());
Expand All @@ -2810,13 +2800,6 @@ emitCXXConstructorThunkIfNeeded(IRGenModule &IGM, SILFunction *initializer,
return ctorAddress;
}

// 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.
llvm::SmallString<32> name;
llvm::raw_svector_ostream stream(name);
stream << "__swift_cxx_ctor";
entity.mangle(stream);

llvm::Function *thunk = llvm::Function::Create(
assumedFnType, llvm::Function::PrivateLinkage, name, &IGM.Module);

Expand Down Expand Up @@ -2892,11 +2875,20 @@ llvm::Function *IRGenModule::getAddrOfSILFunction(
} else {
auto globalDecl = getClangGlobalDeclForFunction(clangDecl);
clangAddr = getAddrOfClangGlobalDecl(globalDecl, forDefinition);
}

if (auto ctor = dyn_cast<clang::CXXConstructorDecl>(clangDecl)) {
clangAddr =
emitCXXConstructorThunkIfNeeded(*this, f, ctor, entity, clangAddr);
}
if (auto ctor = dyn_cast<clang::CXXConstructorDecl>(clangDecl)) {
Signature signature = getSignature(f->getLoweredFunctionType());

// 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.
llvm::SmallString<32> name;
llvm::raw_svector_ostream stream(name);
stream << "__swift_cxx_ctor";
entity.mangle(stream);

clangAddr = emitCXXConstructorThunkIfNeeded(*this, signature, ctor, name,
clangAddr);
}
}

Expand Down
17 changes: 15 additions & 2 deletions lib/IRGen/GenDecl.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@
#ifndef SWIFT_IRGEN_GENDECL_H
#define SWIFT_IRGEN_GENDECL_H

#include "DebugTypeInfo.h"
#include "IRGen.h"
#include "swift/Basic/OptimizationMode.h"
#include "swift/SIL/SILLocation.h"
#include "clang/AST/DeclCXX.h"
#include "llvm/IR/CallingConv.h"
#include "llvm/Support/CommandLine.h"
#include "DebugTypeInfo.h"
#include "IRGen.h"

namespace llvm {
class AttributeList;
Expand Down Expand Up @@ -57,6 +58,18 @@ namespace irgen {
createLinkerDirectiveVariable(IRGenModule &IGM, StringRef Name);

void disableAddressSanitizer(IRGenModule &IGM, llvm::GlobalVariable *var);

/// If the calling convention for `ctor` doesn't match the calling convention
/// that we assumed for it when we imported it as `initializer`, emit and
/// return a thunk that conforms to the assumed calling convention. The thunk
/// is marked `alwaysinline`, so it doesn't generate any runtime overhead.
/// If the assumed calling convention was correct, just return `ctor`.
///
/// See also comments in CXXMethodConventions in SIL/IR/SILFunctionType.cpp.
llvm::Constant *
emitCXXConstructorThunkIfNeeded(IRGenModule &IGM, Signature signature,
const clang::CXXConstructorDecl *ctor,
StringRef name, llvm::Constant *ctorAddress);
}
}

Expand Down
89 changes: 88 additions & 1 deletion lib/IRGen/GenStruct.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
#include "swift/AST/Decl.h"
#include "swift/AST/IRGenOptions.h"
#include "swift/AST/Pattern.h"
#include "swift/AST/SemanticAttrs.h"
#include "swift/AST/SubstitutionMap.h"
#include "swift/AST/Types.h"
#include "swift/IRGen/Linking.h"
#include "swift/SIL/SILFunctionBuilder.h"
#include "swift/SIL/SILModule.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
Expand All @@ -36,17 +38,19 @@
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"

#include "GenDecl.h"
#include "GenMeta.h"
#include "GenRecord.h"
#include "GenType.h"
#include "IRGenFunction.h"
#include "IRGenModule.h"
#include "IndirectTypeInfo.h"
#include "MemberAccessStrategy.h"
#include "MetadataLayout.h"
#include "NonFixedTypeInfo.h"
#include "ResilientTypeInfo.h"
#include "Signature.h"
#include "StructMetadataVisitor.h"
#include "MetadataLayout.h"

#pragma clang diagnostic ignored "-Winconsistent-missing-override"

Expand Down Expand Up @@ -326,6 +330,7 @@ namespace {
public StructTypeInfoBase<LoadableClangRecordTypeInfo, LoadableTypeInfo,
ClangFieldInfo> {
const clang::RecordDecl *ClangDecl;

public:
LoadableClangRecordTypeInfo(ArrayRef<ClangFieldInfo> fields,
unsigned explosionSize,
Expand Down Expand Up @@ -374,6 +379,73 @@ namespace {
ClangFieldInfo> {
const clang::RecordDecl *ClangDecl;

const clang::CXXConstructorDecl *findCopyConstructor() const {
const clang::CXXRecordDecl *cxxRecordDecl =
dyn_cast<clang::CXXRecordDecl>(ClangDecl);
if (!cxxRecordDecl)
return nullptr;
for (auto method : cxxRecordDecl->methods()) {
if (auto ctor = dyn_cast<clang::CXXConstructorDecl>(method)) {
if (ctor->isCopyConstructor())
return ctor;
}
}
return nullptr;
}

CanSILFunctionType createCXXCopyConstructorFunctionType(IRGenFunction &IGF,
SILType T) const {
// Create the following function type:
// @convention(c) (UnsafePointer<T>) -> @out T
// This is how clang *would* import the copy constructor. So, later, when
// we pass it to "emitCXXConstructorThunkIfNeeded" we get a thunk with
// the following LLVM function type:
// void (%struct.T* %this, %struct.T* %0)
auto ptrTypeDecl =
IGF.getSILModule().getASTContext().getUnsafePointerDecl();
auto subst = SubstitutionMap::get(ptrTypeDecl->getGenericSignature(),
{T.getASTType()},
ArrayRef<ProtocolConformanceRef>{});
auto ptrType = ptrTypeDecl->getDeclaredInterfaceType().subst(subst);
SILParameterInfo ptrParam(ptrType->getCanonicalType(),
ParameterConvention::Direct_Unowned);
SILResultInfo result(T.getASTType(), ResultConvention::Indirect);

return SILFunctionType::get(
GenericSignature(),
SILFunctionType::ExtInfo().withRepresentation(
SILFunctionTypeRepresentation::CFunctionPointer),
SILCoroutineKind::None,
/*callee=*/ParameterConvention::Direct_Unowned,
/*params*/ {ptrParam},
/*yields*/ {}, /*results*/ {result},
/*error*/ None,
/*pattern subs*/ SubstitutionMap(),
/*invocation subs*/ SubstitutionMap(), IGF.IGM.Context);
}

void emitCopyWithCopyConstructor(
IRGenFunction &IGF, SILType T,
const clang::CXXConstructorDecl *copyConstructor, llvm::Value *src,
llvm::Value *dest) const {
auto fnType = createCXXCopyConstructorFunctionType(IGF, T);
auto globalDecl =
clang::GlobalDecl(copyConstructor, clang::Ctor_Complete);
auto clangFnAddr =
IGF.IGM.getAddrOfClangGlobalDecl(globalDecl, NotForDefinition);
auto callee = cast<llvm::Function>(clangFnAddr->stripPointerCasts());
Signature signature = IGF.IGM.getSignature(fnType);
std::string name = "__swift_cxx_copy_ctor" + callee->getName().str();
clangFnAddr = emitCXXConstructorThunkIfNeeded(
IGF.IGM, signature, copyConstructor, name, clangFnAddr);
callee = cast<llvm::Function>(clangFnAddr);
dest = IGF.coerceValue(dest, callee->getFunctionType()->getParamType(0),
IGF.IGM.DataLayout);
src = IGF.coerceValue(src, callee->getFunctionType()->getParamType(1),
IGF.IGM.DataLayout);
IGF.Builder.CreateCall(callee, {dest, src});
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't correct on all ABIs -- some ABIs add implicit arguments to constructor calls in some situations. Look at #30630 and, in particular, emitConstructorThunkIfNeeded().

You'll either have to call emitConstructorThunkIfNeeded() or (probably preferably) create a small helper function that calls getClangGlobalDeclForFunction(), getAddrOfClangGlobalDecl(), and emitConstructorThunkIfNeeded() (see changes to IRGenModule::getAddrOfSILFunction in #30630).

Also, add a test that makes sure the implicit arguments are added when appropriate.

Copy link
Contributor Author

@zoecarver zoecarver Jun 16, 2020

Choose a reason for hiding this comment

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

I created a helper function and generalized emitConstructorThunkIfNeeded a bit (to take a Signature instead of a SILFunction and a StringRef name instead of LinkEntity). Thanks for pointing this out :)

Copy link
Contributor Author

@zoecarver zoecarver Jun 16, 2020

Choose a reason for hiding this comment

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

@martinboehme Looking at the tests from #30630. I'm getting:

 error: unable to load standard library for target 'x86_64-unknown-windows-msvc'

What target builds the windows stdlib?

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a helper function and generalized emitConstructorThunkIfNeeded a bit (to take a Signature instead of a SILFunction and a StringRef name instead of LinkEntity). Thanks for pointing this out :)

Hm. I had forgotten that emitCXXConstructorThunkIfNeeded takes a SILFunction.

I think that the way you're calling the modified function doesn't work though. I think the way you're calling it, assumedFnType will always be equal to ctorFnType, so it will never create a thunk. What I think you need to do is to create your own assumed function type, then compare against that.

Maybe, at the end of the day, it's easier to do things at the SIL level after all though -- i.e. import the copy constructor as a hidden initializer, emit a call to that, and let the existing machinery take care of creating a thunk if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@martinboehme Looking at the tests from #30630. I'm getting:

 error: unable to load standard library for target 'x86_64-unknown-windows-msvc'

What target builds the windows stdlib?

Not sure what you mean -- are you getting this error when running existing tests on #30630 (I think they should all be passing on CI though?) or when trying to create new tests in the spirit of the tests in #30630?

It's unfortunately not possible currently to cross-compile stdlib, IIUC. In other words, you can't create a Windows stdlib on Linux, for example. It would be great if this were possible, but for the time being, if you want to do portable IRGen tests (i.e. run Windows IRGen tests on Linux, for example), you can't use stdlib in them. This is pretty restrictive, but you can usually still find a way to test what you want to test -- for an example, see constructors-irgen.swift in #30630.

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 think that the way you're calling the modified function doesn't work though. I think the way you're calling it, assumedFnType will always be equal to ctorFnType, so it will never create a thunk. What I think you need to do is to create your own assumed function type, then compare against that.

I think you're right.

Maybe, at the end of the day, it's easier to do things at the SIL level after all though -- i.e. import the copy constructor as a hidden initializer, emit a call to that, and let the existing machinery take care of creating a thunk if needed.

We could also import it as a __double_underscore method which should help with overload resolution. Then just use it's function signature and ignore it.

Alternatively, we could try to build the "correct" function type in IRGen and then use that in the signature argument.

}

public:
AddressOnlyClangRecordTypeInfo(ArrayRef<ClangFieldInfo> fields,
llvm::Type *storageType, Size size,
Expand Down Expand Up @@ -451,6 +523,21 @@ namespace {
"member functions.");
}

void initializeWithCopy(IRGenFunction &IGF, Address destAddr,
Address srcAddr, SILType T,
bool isOutlined) const override {
if (auto copyConstructor = findCopyConstructor()) {
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
srcAddr.getAddress(),
destAddr.getAddress());
return;
}
StructTypeInfoBase<AddressOnlyClangRecordTypeInfo, FixedTypeInfo,
ClangFieldInfo>::initializeWithCopy(IGF, destAddr,
srcAddr, T,
isOutlined);
}

llvm::NoneType getNonFixedOffsets(IRGenFunction &IGF) const { return None; }
llvm::NoneType getNonFixedOffsets(IRGenFunction &IGF, SILType T) const {
return None;
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SwiftShims/RefCount.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
// This definition is a placeholder for importing into Swift.
// It provides size and alignment but cannot be manipulated safely there.
typedef struct {
__swift_uintptr_t refCounts SWIFT_ATTRIBUTE_UNAVAILABLE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, in C-mode this attribute is meaningless which is why we didn't see the error before now. If we want to make sure that in runtime mode we don't use this type, I'll need to create a separate and more involved fix. Let me know if that needs to be done.

Another "fix" could be to only add SWIFT_ATTRIBUTE_UNAVAILABLE when ifndef __swift__ .

Copy link
Contributor

Choose a reason for hiding this comment

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

So, in C-mode this attribute is meaningless which is why we didn't see the error before now.

Which error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we have a reference type, for example an array, we try to add an implicit copy constructor in C++ mode. When we try to do this, we get an error that refCounts is unreachable:

swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/RefCount.h:20:9: error: 'refCounts' is unavailable
typedef struct {
        ^
swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/HeapObject.h:45:8: note: in implicit copy constructor for 'InlineRefCountsPlaceholder' first required here
struct HeapObject {
       ^
swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/GlobalObjects.h:38:8: note: in implicit copy constructor for 'HeapObject' first required here
struct _SwiftEmptyArrayStorage {
       ^
<unknown>:0: note: in implicit copy constructor for '_SwiftEmptyArrayStorage' first required here
swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/RefCount.h:21:21: note: 'refCounts' has been explicitly marked unavailable here
  __swift_uintptr_t refCounts SWIFT_ATTRIBUTE_UNAVAILABLE;

__swift_uintptr_t refCounts;
} InlineRefCountsPlaceholder;

#if defined(__swift__)
Expand Down
11 changes: 11 additions & 0 deletions test/Interop/Cxx/class/Inputs/constructors.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ struct TemplatedConstructorWithExtraArg {
TemplatedConstructorWithExtraArg(T value, U other) { }
};

struct HasUserProvidedCopyConstructor {
int numCopies;
HasUserProvidedCopyConstructor(int numCopies = 0) : numCopies(numCopies) {}
HasUserProvidedCopyConstructor(const HasUserProvidedCopyConstructor &other)
: numCopies(other.numCopies + 1) {}
};

struct DeletedCopyConstructor {
DeletedCopyConstructor(const DeletedCopyConstructor &) = delete;
};

// TODO: we should be able to import this constructor correctly. Until we can,
// make sure not to crash.
struct UsingBaseConstructor : ConstructorWithParam {
Expand Down
Loading