Skip to content

[cxx-interop] Add support for custom C++ destructors. #32291

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

Merged
merged 1 commit into from
Jan 28, 2021
Merged
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
80 changes: 76 additions & 4 deletions lib/IRGen/GenStruct.cpp
Original file line number Diff line number Diff line change
@@ -16,20 +16,25 @@

#include "GenStruct.h"

#include "swift/AST/Types.h"
#include "swift/AST/ClangModuleLoader.h"
#include "swift/AST/Decl.h"
#include "swift/AST/IRGenOptions.h"
#include "swift/AST/Pattern.h"
#include "swift/AST/SubstitutionMap.h"
#include "swift/AST/Types.h"
#include "swift/IRGen/Linking.h"
#include "swift/SIL/SILModule.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
#include "clang/AST/Decl.h"
#include "clang/AST/GlobalDecl.h"
#include "clang/AST/Mangle.h"
#include "clang/AST/RecordLayout.h"
#include "clang/CodeGen/CodeGenABITypes.h"
#include "clang/CodeGen/SwiftCallingConv.h"
#include "clang/Sema/Sema.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"

#include "GenMeta.h"
#include "GenRecord.h"
@@ -62,6 +67,24 @@ static StructTypeInfoKind getStructTypeInfoKind(const TypeInfo &type) {
return (StructTypeInfoKind) type.getSubclassKind();
}

/// If this type has a CXXDestructorDecl, find it and return it. Otherwise,
/// return nullptr.
static clang::CXXDestructorDecl *getCXXDestructor(SILType type) {
auto *structDecl = type.getStructOrBoundGenericStruct();
if (!structDecl || !structDecl->getClangDecl())
return nullptr;
const clang::CXXRecordDecl *cxxRecordDecl =
dyn_cast<clang::CXXRecordDecl>(structDecl->getClangDecl());
if (!cxxRecordDecl)
return nullptr;
for (auto member : cxxRecordDecl->methods()) {
if (auto dest = dyn_cast<clang::CXXDestructorDecl>(member)) {
return dest;
}
}
return nullptr;
}

namespace {
class StructFieldInfo : public RecordField<StructFieldInfo> {
public:
@@ -362,11 +385,60 @@ namespace {
// with user-defined special member functions.
SpareBitVector(llvm::Optional<APInt>{
llvm::APInt(size.getValueInBits(), 0)}),
align, IsPOD, IsNotBitwiseTakable, IsFixedSize),
align, IsNotPOD, IsNotBitwiseTakable, IsFixedSize),
ClangDecl(clangDecl) {
(void)ClangDecl;
}

void destroy(IRGenFunction &IGF, Address address, SILType T,
bool isOutlined) const override {
auto *destructor = getCXXDestructor(T);
// If the destructor is trivial, clang will assert when we call
// `emitCXXDestructorCall` so, just let Swift handle this destructor.
if (!destructor || destructor->isTrivial()) {
// If we didn't find a destructor to call, bail out to the parent
// implementation.
StructTypeInfoBase<AddressOnlyClangRecordTypeInfo, FixedTypeInfo,
ClangFieldInfo>::destroy(IGF, address, T,
isOutlined);
return;
}

if (!destructor->isUserProvided() &&
!destructor->doesThisDeclarationHaveABody()) {
assert(!destructor->isDeleted() &&
"Swift cannot handle a type with no known destructor.");
// Make sure we define the destructor so we have something to call.
auto &sema = IGF.IGM.Context.getClangModuleLoader()->getClangSema();
sema.DefineImplicitDestructor(clang::SourceLocation(), destructor);
}

clang::GlobalDecl destructorGlobalDecl(destructor, clang::Dtor_Complete);
auto *destructorFnAddr =
cast<llvm::Function>(IGF.IGM.getAddrOfClangGlobalDecl(
destructorGlobalDecl, NotForDefinition));

SmallVector<llvm::Value *, 2> args;
auto *thisArg = IGF.coerceValue(address.getAddress(),
destructorFnAddr->getArg(0)->getType(),
IGF.IGM.DataLayout);
args.push_back(thisArg);
llvm::Value *implicitParam =
clang::CodeGen::getCXXDestructorImplicitParam(
IGF.IGM.getClangCGM(), IGF.Builder.GetInsertBlock(),
IGF.Builder.GetInsertPoint(), destructor, clang::Dtor_Complete,
false, false);
if (implicitParam) {
implicitParam = IGF.coerceValue(implicitParam,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is any of the tests covering this case of having an implicit param?

Copy link
Contributor Author

@zoecarver zoecarver Nov 11, 2020

Choose a reason for hiding this comment

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

Hmm, you're right it might not be needed. The implicit param would be a vtable pointer for a base class which I think could only happen if we inherited from a virtual class in Swift.

@martinboehme was the one who originally suggested that there was a need for this. I couldn't come up with an example where we'd need one today (maybe in the future there'd be a need) but maybe they have one in mind.

It did prompt me to add some tests with virtual destructors/base classes, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @gribozavr remembers Martin's motivation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Martin wrote me in an email:

Yes, none of the ABIs I'm aware of will actually add an implicit param in the cases that we can trigger from Swift today. Still, that's just relying on the way we happen to be using the ABIs that happen to be supported today. It would seem cleaner to keep this logic to add the implicit param if needed, though it's somewhat unsatisfactory that there is no way to test this logic today.

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 don't think it's too big of a deal; I could go either way. It's about 10 lines of code, so either I can keep it or put a comment, so future me knows what to do.

destructorFnAddr->getArg(1)->getType(),
IGF.IGM.DataLayout);
args.push_back(implicitParam);
}

IGF.Builder.CreateCall(destructorFnAddr->getFunctionType(),
destructorFnAddr, args);
}

TypeLayoutEntry *buildTypeLayoutEntry(IRGenModule &IGM,
SILType T) const override {
return IGM.typeLayoutCache.getOrCreateScalarEntry(*this, T);
2 changes: 1 addition & 1 deletion lib/IRGen/IRGenModule.h
Original file line number Diff line number Diff line change
@@ -1062,10 +1062,10 @@ class IRGenModule {
SILType objectType, const TypeInfo &objectTI,
const OutliningMetadataCollector &collector);

private:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC there was a review comment here saying I should use a higher level of abstraction. (Just leaving this comment so I remember.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think this is the correct method to use. I'm not sure what the problem with making it public is. If there is a more appropriate API, let me know and I'll use that. I suppose I could just use IGM.ClangCodeGen->GetAddrOfGlobal directly, but that seems worse to me.

llvm::Constant *getAddrOfClangGlobalDecl(clang::GlobalDecl global,
ForDefinition_t forDefinition);

private:
using CopyAddrHelperGenerator =
llvm::function_ref<void(IRGenFunction &IGF, Address dest, Address src,
SILType objectType, const TypeInfo &objectTI)>;
20 changes: 20 additions & 0 deletions test/Interop/Cxx/class/Inputs/destructors.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_DESTRUCTORS_H
#define TEST_INTEROP_CXX_CLASS_INPUTS_DESTRUCTORS_H

struct DummyStruct {};

struct HasUserProvidedDestructorAndDummy {
DummyStruct dummy;
~HasUserProvidedDestructorAndDummy() {}
};

struct HasUserProvidedDestructor {
int *value;
~HasUserProvidedDestructor() { *value = 42; }
};

struct HasNonTrivialImplicitDestructor {
HasUserProvidedDestructor member;
};

#endif // TEST_INTEROP_CXX_CLASS_INPUTS_DESTRUCTORS_H
5 changes: 5 additions & 0 deletions test/Interop/Cxx/class/Inputs/module.modulemap
Original file line number Diff line number Diff line change
@@ -18,6 +18,11 @@ module ConstructorsObjC {
requires cplusplus
}

module Destructors {
header "destructors.h"
requires cplusplus
}

module LoadableTypes {
header "loadable-types.h"
requires cplusplus
13 changes: 13 additions & 0 deletions test/Interop/Cxx/class/destructors-correct-abi-irgen.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// RUN: %swift -I %S/Inputs -enable-cxx-interop -emit-ir %s | %FileCheck %s

import Destructors

// CHECK-LABEL: define {{.*}}void @"$s4main4testyyF"
// CHECK: [[H:%.*]] = alloca %TSo33HasUserProvidedDestructorAndDummyV
// CHECK: [[CXX_THIS:%.*]] = bitcast %TSo33HasUserProvidedDestructorAndDummyV* [[H]] to %struct.HasUserProvidedDestructorAndDummy*
// CHECK: call {{.*}}@{{_ZN33HasUserProvidedDestructorAndDummyD(1|2)Ev|"\?\?1HasUserProvidedDestructorAndDummy@@QEAA@XZ"}}(%struct.HasUserProvidedDestructorAndDummy* [[CXX_THIS]])
// CHECK: ret void
public func test() {
let d = DummyStruct()
let h = HasUserProvidedDestructorAndDummy(dummy: d)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// RUN: %target-swift-frontend -enable-cxx-interop -I %S/Inputs %s -emit-ir | %FileCheck %s

import Destructors

// CHECK-LABEL: define {{.*}}void @"$s4main35testHasNonTrivialImplicitDestructoryyF"
// CHECK: call {{.*}}@{{_ZN31HasNonTrivialImplicitDestructorD(1|2)Ev|"\?\?1HasNonTrivialImplicitDestructor@@QEAA@XZ"}}(%struct.HasNonTrivialImplicitDestructor*
// CHECK: ret void

// TODO: Somehow check that _ZN31HasNonTrivialImplicitDestructorD1Ev (if present) calls _ZN25HasUserProvidedDestructorD2Ev.

public func testHasNonTrivialImplicitDestructor() {
_ = HasNonTrivialImplicitDestructor()
}

// Check that we call the base destructor.
// CHECK-LABEL: define {{.*}}@{{_ZN31HasNonTrivialImplicitDestructorD2Ev|"\?\?1HasNonTrivialImplicitDestructor@@QEAA@XZ"}}(%struct.HasNonTrivialImplicitDestructor*
// CHECK: call {{.*}}@{{_ZN25HasUserProvidedDestructorD(1|2)Ev|"\?\?1HasUserProvidedDestructor@@QEAA@XZ"}}
// CHECK: ret
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#ifndef TEST_INTEROP_CXX_VALUE_WITNESS_TABLE_INPUTS_CUSTOM_DESTRUCTORS_H
#define TEST_INTEROP_CXX_VALUE_WITNESS_TABLE_INPUTS_CUSTOM_DESTRUCTORS_H

struct HasUserProvidedDestructor {
int *value;
~HasUserProvidedDestructor() { *value = 42; }
};

struct HasEmptyDestructorAndMemberWithUserDefinedConstructor {
HasUserProvidedDestructor member;
~HasEmptyDestructorAndMemberWithUserDefinedConstructor() { /* empty */
}
};

struct HasNonTrivialImplicitDestructor {
HasUserProvidedDestructor member;
};

struct HasNonTrivialDefaultedDestructor {
HasUserProvidedDestructor member;
~HasNonTrivialDefaultedDestructor() = default;
};

struct HasDefaultedDestructor {
~HasDefaultedDestructor() = default;
};

// For the following objects with virtual bases / destructors, make sure that
// any exectuable user of these objects disable rtti and exceptions. Otherwise,
// the linker will error because of undefined vtables.
// FIXME: Once we can link with libc++ we can enable RTTI.

struct HasVirtualBaseAndDestructor : virtual HasDefaultedDestructor {
int *value;
HasVirtualBaseAndDestructor(int *value) : value(value) {}
~HasVirtualBaseAndDestructor() { *value = 42; }
};

struct HasVirtualDestructor {
// An object with a virtual destructor requires a delete operator in case
// we try to delete the base object. Until we can link against libc++, use
// this dummy implementation.
static void operator delete(void *p) { __builtin_unreachable(); }
virtual ~HasVirtualDestructor(){};
};

struct HasVirtualDefaultedDestructor {
static void operator delete(void *p) { __builtin_unreachable(); }
virtual ~HasVirtualDefaultedDestructor() = default;
};

struct HasBaseWithVirtualDestructor : HasVirtualDestructor {
int *value;
HasBaseWithVirtualDestructor(int *value) : value(value) {}
~HasBaseWithVirtualDestructor() { *value = 42; }
};

struct HasVirtualBaseWithVirtualDestructor : virtual HasVirtualDestructor {
int *value;
HasVirtualBaseWithVirtualDestructor(int *value) : value(value) {}
~HasVirtualBaseWithVirtualDestructor() { *value = 42; }
};

struct DummyStruct {};

struct HasUserProvidedDestructorAndDummy {
DummyStruct dummy;
~HasUserProvidedDestructorAndDummy() {}
};

// Make sure that we don't crash on struct templates with destructors.
template <typename T> struct S {
~S() {}
};

#endif // TEST_INTEROP_CXX_VALUE_WITNESS_TABLE_INPUTS_CUSTOM_DESTRUCTORS_H
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module CustomDestructor {
header "custom-destructors.h"
requires cplusplus
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// RUN: %target-swift-frontend -enable-cxx-interop -I %S/Inputs %s -emit-ir | %FileCheck %s

import CustomDestructor

protocol InitWithDummy {
init(dummy: DummyStruct)
}

extension HasUserProvidedDestructorAndDummy : InitWithDummy { }

// Make sure the destructor is added as a witness.
// CHECK: @"$sSo33HasUserProvidedDestructorAndDummyVWV" = linkonce_odr hidden constant %swift.vwtable
// CHECK-SAME: i8* bitcast (void (%swift.opaque*, %swift.type*)* @"$sSo33HasUserProvidedDestructorAndDummyVwxx" to i8*)

// CHECK-LABEL: define {{.*}}void @"$s4main37testHasUserProvidedDestructorAndDummyyyF"
// CHECK: [[OBJ:%.*]] = alloca %TSo33HasUserProvidedDestructorAndDummyV
// CHECK: [[CXX_OBJ:%.*]] = bitcast %TSo33HasUserProvidedDestructorAndDummyV* [[OBJ]] to %struct.HasUserProvidedDestructorAndDummy*
// CHECK: call {{.*}}@{{_ZN33HasUserProvidedDestructorAndDummyD(1|2)Ev|"\?\?1HasUserProvidedDestructorAndDummy@@QEAA@XZ"}}(%struct.HasUserProvidedDestructorAndDummy* [[CXX_OBJ]])
// CHECK: ret void

// Make sure we not only declare but define the destructor.
// CHECK-LABEL: define {{.*}}@{{_ZN33HasUserProvidedDestructorAndDummyD(1|2)Ev|"\?\?1HasUserProvidedDestructorAndDummy@@QEAA@XZ"}}
// CHECK: ret
public func testHasUserProvidedDestructorAndDummy() {
_ = HasUserProvidedDestructorAndDummy(dummy: DummyStruct())
}

// CHECK-LABEL: define {{.*}}void @"$s4main26testHasDefaultedDestructoryyF"
// CHECK: call {{.*}}@{{_ZN22HasDefaultedDestructorC(1|2)Ev|"\?\?0HasDefaultedDestructor@@QEAA@XZ"}}(%struct.HasDefaultedDestructor*
// CHECK: ret void

// CHECK-LABEL: define {{.*}}@{{_ZN22HasDefaultedDestructorC(1|2)Ev|"\?\?0HasDefaultedDestructor@@QEAA@XZ"}}(%struct.HasDefaultedDestructor*
// CHECK: ret
public func testHasDefaultedDestructor() {
_ = HasDefaultedDestructor()
}

// Make sure the destroy value witness calls the destructor.
// CHECK-LABEL: define {{.*}}void @"$sSo33HasUserProvidedDestructorAndDummyVwxx"
// CHECK: call {{.*}}@{{_ZN33HasUserProvidedDestructorAndDummyD(1|2)Ev|"\?\?1HasUserProvidedDestructorAndDummy@@QEAA@XZ"}}(%struct.HasUserProvidedDestructorAndDummy*
// CHECK: ret
Loading