Skip to content

[cxx-interop] import const reference parameters as __shared T #40350

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

Closed
wants to merge 1 commit into from
Closed

[cxx-interop] import const reference parameters as __shared T #40350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions lib/ClangImporter/ImportType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1906,7 +1906,7 @@ ParameterList *ClangImporter::Implementation::importFunctionParameterList(
// Import the parameter type into Swift.
Type swiftParamTy;
bool isParamTypeImplicitlyUnwrapped = false;
bool isInOut = false;
ParamSpecifier paramSpecifier = ParamSpecifier::Default;
if ((isa<clang::ReferenceType>(paramTy) || isa<clang::PointerType>(paramTy)) &&
isa<clang::TemplateTypeParmType>(paramTy->getPointeeType())) {
auto pointeeType = paramTy->getPointeeType();
Expand All @@ -1926,7 +1926,8 @@ ParameterList *ClangImporter::Implementation::importFunctionParameterList(
} else {
if (auto refType = dyn_cast<clang::ReferenceType>(paramTy)) {
paramTy = refType->getPointeeType();
isInOut = true;
paramSpecifier = paramTy.isConstQualified() ? ParamSpecifier::Shared
Copy link
Contributor

Choose a reason for hiding this comment

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

"__shared" doesn't mean "shared value" is really means "non owning" (as Andrew helpfully reminded me). No matter what, It's still possible that copies will be made, since we don't support proper borrowing yet. But I'm told that's soon to come. (I think that's the same case for inout if the underlying object is constructed in Swift, so it's a bit of a mute point, though.)

Anyway, we shouldn't use "__shared" here. Let's just use the default parameter specifier and update it whenever we have something better.

CC @atrick

: ParamSpecifier::InOut;
}
auto importedType = importType(paramTy, importKind, allowNSUIntegerAsInt,
Bridgeability::Full, OptionalityOfParam);
Expand Down Expand Up @@ -1957,8 +1958,7 @@ ParameterList *ClangImporter::Implementation::importFunctionParameterList(
param, AccessLevel::Private, SourceLoc(), SourceLoc(), name,
importSourceLoc(param->getLocation()), bodyName,
ImportedHeaderUnit);
paramInfo->setSpecifier(isInOut ? ParamSpecifier::InOut
: ParamSpecifier::Default);
paramInfo->setSpecifier(paramSpecifier);
paramInfo->setInterfaceType(swiftParamTy);
recordImplicitUnwrapForDecl(paramInfo, isParamTypeImplicitlyUnwrapped);
parameters.push_back(paramInfo);
Expand Down
11 changes: 11 additions & 0 deletions lib/SIL/IR/SILFunctionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,6 +1289,17 @@ static bool isClangTypeMoreIndirectThanSubstType(TypeConverter &TC,

return true;
}

// Pass C++ const reference types indirectly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this comment to explain why this logic is necessary? I.e., how we don't have a better way to express that something's an immutable borrow/reference in Swift right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess another way to say it: the Swift AST has no way to represent const-ref until this point. So, we can't say const-ref in the importer, that's why this logic is here and not in the importer.

if (clangTy->isReferenceType() &&
clangTy->getPointeeType().isConstQualified()) {
// FIXME: Template substitution references are still mapped as
Copy link
Contributor

Choose a reason for hiding this comment

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

What case is this covering? T&?

// UnsafePointer.
if (isa<clang::SubstTemplateTypeParmType>(
clangTy->getPointeeType().getTypePtr()))
return false;
return true;
}
return false;
}

Expand Down
16 changes: 16 additions & 0 deletions test/Interop/Cxx/reference/Inputs/reference.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,19 @@ void setConstStaticIntRvalueRef(const int &&i) { staticInt = i; }

auto getFuncRef() -> int (&)() { return getStaticInt; }
auto getFuncRvalueRef() -> int (&&)() { return getStaticInt; }

void takeConstPODStructRef(const PODStruct &value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these can probably all go in the header (makes it easier to find in the future). The source file is just for the statics.

staticInt = value.x;
}

void takeConstPODStructRvalueRef(const PODStruct &&value) {
staticInt = value.x;
}

void takeConstPODStructPointerConstRef(PODStruct * _Nonnull const &value) {
staticInt = value->x;
}

void takeConstPODStructPointerConstRvalueRef(PODStruct * _Nonnull const &&value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be importing these.

But also, this is a real anti-pattern. Clang should probably complain. And... should we ever import these?

staticInt = value->x;
}
10 changes: 10 additions & 0 deletions test/Interop/Cxx/reference/Inputs/reference.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,14 @@ auto getFuncRvalueRef() -> int (&&)();
// crashing when we have an "_Atomic" type or a reference to one.
void dontImportAtomicRef(_Atomic(int)&) { }

struct PODStruct {
int x;
};

void takeConstPODStructRef(const PODStruct &);
void takeConstPODStructRvalueRef(const PODStruct &&);

void takeConstPODStructPointerConstRef(PODStruct * _Nonnull const &);
void takeConstPODStructPointerConstRvalueRef(PODStruct * _Nonnull const &&);

#endif // TEST_INTEROP_CXX_REFERENCE_INPUTS_REFERENCE_H
8 changes: 4 additions & 4 deletions test/Interop/Cxx/reference/reference-irgen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ public func setCxxRef() {
// CHECK: call void @{{_Z15setStaticIntRefRi|"\?setStaticIntRef@@YAXAEAH@Z"}}(i32* %{{.*}})

public func setCxxConstRef() {
var val: CInt = 21
setConstStaticIntRef(&val)
let val: CInt = 21
setConstStaticIntRef(val)
}

// CHECK: define {{(protected |dllexport )?}}swiftcc void @"$s4main14setCxxConstRefyyF"()
Expand All @@ -55,8 +55,8 @@ public func setCxxRvalueRef() {
// CHECK: call void @{{_Z21setStaticIntRvalueRefOi|"\?setStaticIntRvalueRef@@YAX\$\$QEAH@Z"}}(i32* %{{.*}})

public func setCxxConstRvalueRef() {
var val: CInt = 21
setConstStaticIntRvalueRef(&val)
let val: CInt = 21
setConstStaticIntRvalueRef(val)
}

// CHECK: define {{(protected |dllexport )?}}swiftcc void @"$s4main20setCxxConstRvalueRefyyF"()
Expand Down
4 changes: 2 additions & 2 deletions test/Interop/Cxx/reference/reference-module-interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
// CHECK: func setStaticInt(_: Int32)
// CHECK: func setStaticIntRef(_: inout Int32)
// CHECK: func setStaticIntRvalueRef(_: inout Int32)
// CHECK: func setConstStaticIntRef(_: inout Int32)
// CHECK: func setConstStaticIntRvalueRef(_: inout Int32)
// CHECK: func setConstStaticIntRef(_: __shared Int32)
// CHECK: func setConstStaticIntRvalueRef(_: __shared Int32)
// CHECK: func getFuncRef() -> @convention(c) () -> Int32
// CHECK: func getFuncRvalueRef() -> @convention(c) () -> Int32

Expand Down
16 changes: 8 additions & 8 deletions test/Interop/Cxx/reference/reference-silgen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ func setCxxRef() {
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@inout Int32) -> ()

func setCxxConstRef() {
var val: CInt = 21
setConstStaticIntRef(&val)
let val: CInt = 21
setConstStaticIntRef(val)
}

// CHECK: sil hidden @$s4main14setCxxConstRefyyF : $@convention(thin) () -> ()
// CHECK: [[REF:%.*]] = function_ref @{{_Z20setConstStaticIntRefRKi|\?setConstStaticIntRef@@YAXAEBH@Z}} : $@convention(c) (@inout Int32) -> ()
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@inout Int32) -> ()
// CHECK: [[REF:%.*]] = function_ref @{{_Z20setConstStaticIntRefRKi|\?setConstStaticIntRef@@YAXAEBH@Z}} : $@convention(c) (@in_guaranteed Int32) -> ()
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@in_guaranteed Int32) -> ()

func setCxxRvalueRef() {
var val: CInt = 21
Expand All @@ -62,10 +62,10 @@ func setCxxRvalueRef() {
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@inout Int32) -> ()

func setCxxConstRvalueRef() {
var val: CInt = 21
setConstStaticIntRvalueRef(&val)
let val: CInt = 21
setConstStaticIntRvalueRef(val)
}

// CHECK: sil hidden @$s4main20setCxxConstRvalueRefyyF : $@convention(thin) () -> ()
// CHECK: [[REF:%.*]] = function_ref @{{_Z26setConstStaticIntRvalueRefOKi|\?setConstStaticIntRvalueRef@@YAX\$\$QEBH@Z}} : $@convention(c) (@inout Int32) -> ()
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@inout Int32) -> ()
// CHECK: [[REF:%.*]] = function_ref @{{_Z26setConstStaticIntRvalueRefOKi|\?setConstStaticIntRvalueRef@@YAX\$\$QEBH@Z}} : $@convention(c) (@in_guaranteed Int32) -> ()
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@in_guaranteed Int32) -> ()
44 changes: 40 additions & 4 deletions test/Interop/Cxx/reference/reference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ ReferenceTestSuite.test("pass-lvalue-reference") {

ReferenceTestSuite.test("pass-const-lvalue-reference") {
expectNotEqual(22, getStaticInt())
var val: CInt = 22
setConstStaticIntRef(&val)
let val: CInt = 22
setConstStaticIntRef(val)
expectEqual(22, getStaticInt())
}

Expand All @@ -60,8 +60,8 @@ ReferenceTestSuite.test("pass-rvalue-reference") {

ReferenceTestSuite.test("pass-const-rvalue-reference") {
expectNotEqual(53, getStaticInt())
var val: CInt = 53
setConstStaticIntRvalueRef(&val)
let val: CInt = 53
setConstStaticIntRvalueRef(val)
expectEqual(53, getStaticInt())
}

Expand All @@ -81,4 +81,40 @@ ReferenceTestSuite.test("func-rvalue-reference") {
expectEqual(61, cxxF())
}

ReferenceTestSuite.test("pod-struct-const-lvalue-reference") {
let pod = PODStruct(x: 78)

expectNotEqual(78, getStaticInt())
takeConstPODStructRef(pod)
expectEqual(78, getStaticInt())
}

ReferenceTestSuite.test("pod-struct-const-rvalue-reference") {
let pod = PODStruct(x: 79)

expectNotEqual(79, getStaticInt())
takeConstPODStructRvalueRef(pod)
expectEqual(79, getStaticInt())
}

ReferenceTestSuite.test("pod-struct-pointer-const-lvalue-reference") {
var pod = PODStruct(x: 84)

expectNotEqual(84, getStaticInt())
withUnsafeMutablePointer(to: &pod) { ump in
takeConstPODStructPointerConstRef(ump)
}
expectEqual(84, getStaticInt())
}

ReferenceTestSuite.test("pod-struct-pointer-const-rvalue-reference") {
var pod = PODStruct(x: 85)

expectNotEqual(85, getStaticInt())
withUnsafeMutablePointer(to: &pod) { ump in
takeConstPODStructPointerConstRvalueRef(ump)
}
expectEqual(85, getStaticInt())
}

runAllTests()