Skip to content
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
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsClangImporter.def
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,9 @@ ERROR(foreign_reference_types_release_non_void_return_type, none,
ERROR(foreign_reference_types_retain_release_not_a_function_decl, none,
"specified %select{retain|release}0 function '%1' is not a function",
(bool, StringRef))
ERROR(foreign_reference_types_retain_release_not_an_instance_function, none,
"specified %select{retain|release}0 function '%1' is a static function; expected an instance function",
(bool, StringRef))
ERROR(conforms_to_missing_dot, none,
"expected module name and protocol name separated by '.' in protocol "
"conformance; '%0' is invalid",
Expand Down
25 changes: 21 additions & 4 deletions lib/ClangImporter/ClangImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7915,10 +7915,27 @@ getRefParentDecls(const clang::RecordDecl *decl, ASTContext &ctx,
}

llvm::SmallVector<ValueDecl *, 1>
importer::getValueDeclsForName(
const clang::Decl *decl, ASTContext &ctx, StringRef name) {
importer::getValueDeclsForName(NominalTypeDecl *decl, StringRef name) {
// If the name is empty, don't try to find any decls.
if (name.empty())
return {};

auto &ctx = decl->getASTContext();
auto clangDecl = decl->getClangDecl();
llvm::SmallVector<ValueDecl *, 1> results;
auto *clangMod = decl->getOwningModule();

if (name.starts_with(".")) {
// Look for a member of decl instead of a global.
StringRef memberName = name.drop_front(1);
if (memberName.empty())
return {};
auto declName = DeclName(ctx.getIdentifier(memberName));
auto allResults = evaluateOrDefault(
ctx.evaluator, ClangRecordMemberLookup({decl, declName}), {});
return SmallVector<ValueDecl *, 1>(allResults.begin(), allResults.end());
}

auto *clangMod = clangDecl->getOwningModule();
if (clangMod && clangMod->isSubModule())
clangMod = clangMod->getTopLevelModule();
if (clangMod) {
Expand Down Expand Up @@ -8523,7 +8540,7 @@ CustomRefCountingOperationResult CustomRefCountingOperation::evaluate(
return {CustomRefCountingOperationResult::immortal, nullptr, name};

llvm::SmallVector<ValueDecl *, 1> results =
getValueDeclsForName(swiftDecl->getClangDecl(), ctx, name);
getValueDeclsForName(const_cast<ClassDecl*>(swiftDecl), name);
if (results.size() == 1)
return {CustomRefCountingOperationResult::foundOperation, results.front(),
name};
Expand Down
46 changes: 37 additions & 9 deletions lib/ClangImporter/ImportDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2788,6 +2788,7 @@ namespace {

enum class RetainReleaseOperationKind {
notAfunction,
notAnInstanceFunction,
invalidReturnType,
invalidParameters,
valid
Expand All @@ -2801,17 +2802,32 @@ namespace {
if (!operationFn)
return RetainReleaseOperationKind::notAfunction;

if (operationFn->getParameters()->size() != 1)
return RetainReleaseOperationKind::invalidParameters;
if (operationFn->isStatic())
return RetainReleaseOperationKind::notAnInstanceFunction;

Type paramType =
operationFn->getParameters()->get(0)->getInterfaceType();
// Unwrap if paramType is an OptionalType
if (Type optionalType = paramType->getOptionalObjectType()) {
paramType = optionalType;
}
if (operationFn->isInstanceMember()) {
if (operationFn->getParameters()->size() != 0)
return RetainReleaseOperationKind::invalidParameters;
} else {
if (operationFn->getParameters()->size() != 1)
return RetainReleaseOperationKind::invalidParameters;
}

Type paramType;
NominalTypeDecl *paramDecl = nullptr;
if (!operationFn->isInstanceMember()) {
paramType =
operationFn->getParameters()->get(0)->getInterfaceType();
// Unwrap if paramType is an OptionalType
if (Type optionalType = paramType->getOptionalObjectType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you might be able to get rid of this branch via lookThroughSingleOptionalType.

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 didn't actually touch this code in this PR, I'm not sure why GitHub shows it in the diff 😃

paramType = optionalType;
}

swift::NominalTypeDecl *paramDecl = paramType->getAnyNominal();
paramDecl = paramType->getAnyNominal();
} else {
paramDecl = cast<NominalTypeDecl>(operationFn->getParent());
paramType = paramDecl->getDeclaredInterfaceType();
}

// The return type should be void (for release functions), or void
// or the parameter type (for retain functions).
Expand Down Expand Up @@ -2896,6 +2912,12 @@ namespace {
diag::foreign_reference_types_retain_release_not_a_function_decl,
false, retainOperation.name);
break;
case RetainReleaseOperationKind::notAnInstanceFunction:
Impl.diagnose(
loc,
diag::foreign_reference_types_retain_release_not_an_instance_function,
false, retainOperation.name);
break;
case RetainReleaseOperationKind::invalidReturnType:
Impl.diagnose(
loc,
Expand Down Expand Up @@ -2961,6 +2983,12 @@ namespace {
diag::foreign_reference_types_retain_release_not_a_function_decl,
true, releaseOperation.name);
break;
case RetainReleaseOperationKind::notAnInstanceFunction:
Impl.diagnose(
loc,
diag::foreign_reference_types_retain_release_not_an_instance_function,
true, releaseOperation.name);
break;
case RetainReleaseOperationKind::invalidReturnType:
Impl.diagnose(
loc,
Expand Down
2 changes: 1 addition & 1 deletion lib/ClangImporter/ImporterImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -2200,7 +2200,7 @@ ImportedType findOptionSetEnum(clang::QualType type,
///
/// The name we're looking for is the Swift name.
llvm::SmallVector<ValueDecl *, 1>
getValueDeclsForName(const clang::Decl *decl, ASTContext &ctx, StringRef name);
getValueDeclsForName(NominalTypeDecl* decl, StringRef name);

} // end namespace importer
} // end namespace swift
Expand Down
3 changes: 1 addition & 2 deletions lib/ClangImporter/SwiftDeclSynthesizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2882,8 +2882,7 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
if (!destroyFuncName.consume_front("destroy:"))
continue;

auto decls = getValueDeclsForName(
clangType, nominal->getASTContext(), destroyFuncName);
auto decls = getValueDeclsForName(nominal, destroyFuncName);
for (auto decl : decls) {
auto func = dyn_cast<FuncDecl>(decl);
if (!func)
Expand Down
5 changes: 5 additions & 0 deletions lib/IRGen/GenObjC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1724,6 +1724,11 @@ void IRGenFunction::emitBlockRelease(llvm::Value *value) {

void IRGenFunction::emitForeignReferenceTypeLifetimeOperation(
ValueDecl *fn, llvm::Value *value, bool needsNullCheck) {
if (auto originalDecl = fn->getASTContext()
.getClangModuleLoader()
->getOriginalForClonedMember(fn))
fn = originalDecl;

assert(fn->getClangDecl() && isa<clang::FunctionDecl>(fn->getClangDecl()));

auto clangFn = cast<clang::FunctionDecl>(fn->getClangDecl());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
#include <swift/bridging>

struct RefCountedBox {
int value;
int refCount = 1;

RefCountedBox(int value) : value(value) {}

void doRetain() { refCount++; }
void doRelease() { refCount--; }
} SWIFT_SHARED_REFERENCE(.doRetain, .doRelease);

struct DerivedRefCountedBox : RefCountedBox {
int secondValue = 1;
DerivedRefCountedBox(int value, int secondValue)
: RefCountedBox(value), secondValue(secondValue) {}
};

Choose a reason for hiding this comment

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

Do you think you might be kind enough to also add some templated ref counted base types along the lines of cntrump/MixingLanguagesInAnXcodeProject@main...adetaylor:MixingLanguagesInAnXcodeProject:demonstrate-refcount-probs? Just to make sure this works (and continues to work) for cases like WebKit's? I suspect that the pattern is fairly common since it's a good way to do reference counting without the overhead of virtual functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, let's test that scenario explicitly.


// MARK: Retain in a base type, release in derived

struct BaseHasRetain {
mutable int refCount = 1;
void doRetainInBase() const { refCount++; }
};

struct DerivedHasRelease : BaseHasRetain {
int value;
DerivedHasRelease(int value) : value(value) {}

void doRelease() const { refCount--; }
} SWIFT_SHARED_REFERENCE(.doRetainInBase, .doRelease);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when you have a base class FRT with virtual retain and release methods, and the derived class overrides its retain and release functions? And what about for pure virtual methods?

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 a great point! In case this does not work as intended and not easy to fix, I believe we could just reject virtual refcounting functions for now and fix these in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works! We should have a test for it indeed, I just added one.


// MARK: Retain in a base type, release in templated derived

template <typename T>
struct TemplatedDerivedHasRelease : BaseHasRetain {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to have a test following the curiously recurring template pattern where the base itself is templated. Similar to RefCountedBase in LLVM. We usually need the CRTP for the release operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for that scenario.

T value;
TemplatedDerivedHasRelease(T value) : value(value) {}

void doReleaseTemplated() const { refCount--; }
} SWIFT_SHARED_REFERENCE(.doRetainInBase, .doReleaseTemplated);

using TemplatedDerivedHasReleaseFloat = TemplatedDerivedHasRelease<float>;
using TemplatedDerivedHasReleaseInt = TemplatedDerivedHasRelease<int>;

// MARK: Retain/release in CRTP base type

template <typename Derived>
struct CRTPBase {
mutable int refCount = 1;
void crtpRetain() const { refCount++; }
void crtpRelease() const { refCount--; }
} SWIFT_SHARED_REFERENCE(.crtpRetain, .crtpRelease);

struct CRTPDerived : CRTPBase<CRTPDerived> {
int value;
CRTPDerived(int value) : value(value) {}
};

// MARK: Virtual retain and release

struct VirtualRetainRelease {
int value;
mutable int refCount = 1;
VirtualRetainRelease(int value) : value(value) {}

virtual void doRetainVirtual() const { refCount++; }
virtual void doReleaseVirtual() const { refCount--; }
virtual ~VirtualRetainRelease() = default;
} SWIFT_SHARED_REFERENCE(.doRetainVirtual, .doReleaseVirtual);

struct DerivedVirtualRetainRelease : VirtualRetainRelease {
DerivedVirtualRetainRelease(int value) : VirtualRetainRelease(value) {}

mutable bool calledDerived = false;
void doRetainVirtual() const override { refCount++; calledDerived = true; }
void doReleaseVirtual() const override { refCount--; }
};

// MARK: Pure virtual retain and release

struct PureVirtualRetainRelease {
int value;
mutable int refCount = 1;
PureVirtualRetainRelease(int value) : value(value) {}

virtual void doRetainPure() const = 0;
virtual void doReleasePure() const = 0;
virtual ~PureVirtualRetainRelease() = default;
} SWIFT_SHARED_REFERENCE(.doRetainPure, .doReleasePure);

struct DerivedPureVirtualRetainRelease : PureVirtualRetainRelease {
mutable int refCount = 1;

DerivedPureVirtualRetainRelease(int value) : PureVirtualRetainRelease(value) {}
void doRetainPure() const override { refCount++; }
void doReleasePure() const override { refCount--; }
};

// MARK: Static retain/release
#ifdef INCORRECT
struct StaticRetainRelease {
// expected-error@-1 {{specified retain function '.staticRetain' is a static function; expected an instance function}}
// expected-error@-2 {{specified release function '.staticRelease' is a static function; expected an instance function}}
int value;
int refCount = 1;

StaticRetainRelease(int value) : value(value) {}

static void staticRetain(StaticRetainRelease* o) { o->refCount++; }
static void staticRelease(StaticRetainRelease* o) { o->refCount--; }
} SWIFT_SHARED_REFERENCE(.staticRetain, .staticRelease);

struct DerivedStaticRetainRelease : StaticRetainRelease {
// expected-error@-1 {{cannot find retain function '.staticRetain' for reference type 'DerivedStaticRetainRelease'}}
// expected-error@-2 {{cannot find release function '.staticRelease' for reference type 'DerivedStaticRetainRelease'}}
int secondValue = 1;
DerivedStaticRetainRelease(int value, int secondValue)
: StaticRetainRelease(value), secondValue(secondValue) {}
};
#endif
5 changes: 5 additions & 0 deletions test/Interop/Cxx/foreign-reference/Inputs/module.modulemap
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ module ReferenceCountedObjCProperty {
export *
}

module LifetimeOperationMethods {
header "lifetime-operation-methods.h"
requires cplusplus
}

module MemberLayout {
header "member-layout.h"
requires cplusplus
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// RUN: %target-swift-ide-test -print-module -cxx-interoperability-mode=upcoming-swift -I %swift_src_root/lib/ClangImporter/SwiftBridging -module-to-print=LifetimeOperationMethods -I %S/Inputs -source-filename=x | %FileCheck %s

// CHECK: class RefCountedBox {
// CHECK: func doRetain()
// CHECK: func doRelease()
// CHECK: }
// CHECK: class DerivedRefCountedBox {
// CHECK: func doRetain()
// CHECK: func doRelease()
// CHECK: }

// CHECK: class DerivedHasRelease {
// CHECK: func doRelease()
// CHECK: func doRetainInBase()
// CHECK: }

// CHECK: class TemplatedDerivedHasRelease<CFloat> {
// CHECK: var value: Float
// CHECK: func doReleaseTemplated()
// CHECK: func doRetainInBase()
// CHECK: }
// CHECK: class TemplatedDerivedHasRelease<CInt> {
// CHECK: var value: Int32
// CHECK: func doReleaseTemplated()
// CHECK: func doRetainInBase()
// CHECK: }

// CHECK: class CRTPDerived {
// CHECK: var value: Int32
// CHECK: }

// CHECK: class VirtualRetainRelease {
// CHECK: func doRetainVirtual()
// CHECK: func doReleaseVirtual()
// CHECK: }
// CHECK: class DerivedVirtualRetainRelease {
// CHECK: func doRetainVirtual()
// CHECK: func doReleaseVirtual()
// CHECK: }

// CHECK: class PureVirtualRetainRelease {
// CHECK: func doRetainPure()
// CHECK: func doReleasePure()
// CHECK: }
// CHECK: class DerivedPureVirtualRetainRelease {
// CHECK: func doRetainPure()
// CHECK: func doReleasePure()
// CHECK: var refCount: Int32
// CHECK: }
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// RUN: %target-typecheck-verify-swift -Xcc -DINCORRECT -I %S%{fs-sep}Inputs -I %swift_src_root/lib/ClangImporter/SwiftBridging -verify-additional-file %S%{fs-sep}Inputs%{fs-sep}lifetime-operation-methods.h -cxx-interoperability-mode=upcoming-swift -disable-availability-checking

import LifetimeOperationMethods

let _ = StaticRetainRelease(123)
let _ = DerivedStaticRetainRelease(123, 456)
Loading