Skip to content

[cxx-interop] Fix calling convention for rvalue reference params #79576

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

Merged
merged 1 commit into from
Mar 4, 2025
Merged

[cxx-interop] Fix calling convention for rvalue reference params #79576

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
5 changes: 5 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 @@ -1256,6 +1256,9 @@ class Conventions {
case ValueOwnership::Shared:
return ParameterConvention::Indirect_In_Guaranteed;
case ValueOwnership::Owned:
if (kind == ConventionsKind::CFunction ||
kind == ConventionsKind::CFunctionType)
return getIndirectParameter(index, type, substTL);
return ParameterConvention::Indirect_In;
}
llvm_unreachable("unhandled ownership");
Expand Down Expand Up @@ -3392,6 +3395,8 @@ static ParameterConvention getIndirectCParameterConvention(clang::QualType type)
// A trivial const * parameter in C should be considered @in.
if (importer::isCxxConstReferenceType(type.getTypePtr()))
return ParameterConvention::Indirect_In_Guaranteed;
if (type->isRValueReferenceType())
return ParameterConvention::Indirect_In_CXX;
if (auto *decl = type->getAsRecordDecl()) {
if (!decl->isParamDestroyedInCallee())
return ParameterConvention::Indirect_In_CXX;
Expand Down
3 changes: 2 additions & 1 deletion lib/SILGen/SILGenApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4655,7 +4655,8 @@ static void emitConsumedLValueRecursive(SILGenFunction &SGF, SILLocation loc,

// Load if necessary.
if (value.getType().isAddress()) {
if (!param.isIndirectIn() || !SGF.silConv.useLoweredAddresses()) {
if ((!param.isIndirectIn() && !param.isIndirectInCXX()) ||
!SGF.silConv.useLoweredAddresses()) {
value = SGF.B.createFormalAccessLoadTake(loc, value);

// If our value is a moveonlywrapped type, unwrap it using owned so that
Expand Down
2 changes: 2 additions & 0 deletions lib/SILOptimizer/Transforms/CopyForwarding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ class AnalyzeForwardUse
case SILArgumentConvention::Indirect_In:
return true;
case SILArgumentConvention::Indirect_In_Guaranteed:
case SILArgumentConvention::Indirect_In_CXX:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to help me better understand this patch, because I don't see this case being defined in this patch, and the default case is llvm_unreachable.

Does this mean that the Indirect_In_Cxx case was not possible prior to this patch? (I'm guessing it's the additions to lib/SIL/IR/SILFunctionType.cpp that make this case possible?)

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 hit the unreachable assertion with the test case I added. My suspicion is that it was possible to trigger this even before my patch we just did not run into it with an assertions enabled compiler.

case SILArgumentConvention::Indirect_Inout:
case SILArgumentConvention::Indirect_InoutAliasable:
return false;
Expand Down Expand Up @@ -445,6 +446,7 @@ class AnalyzeBackwardUse
case SILArgumentConvention::Indirect_InoutAliasable:
case SILArgumentConvention::Indirect_In_Guaranteed:
return false;
case SILArgumentConvention::Indirect_In_CXX:
case SILArgumentConvention::Indirect_In:
llvm_unreachable("copy_addr src destroyed without reinitialization");
default:
Expand Down
5 changes: 5 additions & 0 deletions test/Interop/Cxx/reference/Inputs/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ module Reference {
requires cplusplus
}

module SpecialMembers {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why is this module case called SpecialMembers/print-special-members.h? The theme seems to be about logging copy- and move-constructor calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In C++ we call move/copy constructors special member functions: https://en.wikipedia.org/wiki/Special_member_functions

header "print-special-members.h"
requires cplusplus
}

module Closures {
header "closures.h"
requires cplusplus
Expand Down
21 changes: 21 additions & 0 deletions test/Interop/Cxx/reference/Inputs/print-special-members.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#pragma once
#include <cstdio>

struct MoveOnly {
int id;
MoveOnly() : id(0) { printf("MoveOnly %d created\n", id); }
MoveOnly(const MoveOnly&) = delete;
MoveOnly(MoveOnly&& other) : id(other.id + 1) { printf("MoveOnly %d move-created\n", id); }
~MoveOnly() { printf("MoveOnly %d destroyed\n", id); }
};

struct Copyable {
int id;
Copyable() : id(0) { printf("Copyable %d created\n", id); }
Copyable(const Copyable& other) : id(other.id + 1) { printf("Copyable %d copy-created\n", id); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is/can this copy constructor invoked when we do something like:

let c0 = Copyable()
let c1 = Copyable(c0) // or should it be Copyable(other: c0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we do not import the copy constructor as an initializer into Swift. Users cannot invoke them directly.

Copyable(Copyable&& other) : id(other.id + 1) { printf("Copyable %d move-created\n", id); }
~Copyable() { printf("Copyable %d destroyed\n", id); }
};

inline void byRValueRef(MoveOnly&& x) {}
inline void byRValueRef(Copyable&& x) {}
70 changes: 70 additions & 0 deletions test/Interop/Cxx/reference/rvalue-reference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-experimental-cxx-interop -Onone) | %FileCheck -check-prefix=CHECK-DASH-ONONE %s
// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-experimental-cxx-interop -O) | %FileCheck -check-prefix=CHECK-DASH-O %s
//
// REQUIRES: executable_test

import SpecialMembers

func consume(_ x: consuming MoveOnly) {}
func consume(_ x: consuming Copyable) {}

func moveOnly1() {
let x = MoveOnly()
// CHECK-DASH-ONONE: MoveOnly 0 created
// CHECK-DASH-O: MoveOnly 0 created
byRValueRef(consuming: x)
// CHECK-DASH-ONONE-NEXT: MoveOnly 1 move-created
// CHECK-DASH-O-NEXT: MoveOnly 1 move-created
// CHECK-DASH-ONONE-NEXT: MoveOnly 0 destroyed
// CHECK-DASH-O-NEXT: MoveOnly 0 destroyed
// CHECK-DASH-ONONE-NEXT: MoveOnly 1 destroyed
// CHECK-DASH-O-NEXT: MoveOnly 1 destroyed
}

func moveOnly2() {
let x = MoveOnly()
// CHECK-DASH-ONONE-NEXT: MoveOnly 0 created
// CHECK-DASH-O-NEXT: MoveOnly 0 created
consume(x)
// CHECK-DASH-ONONE-NEXT: MoveOnly 1 move-created
// CHECK-DASH-ONONE-NEXT: MoveOnly 0 destroyed
// CHECK-DASH-O-NEXT: MoveOnly 0 destroyed
// CHECK-DASH-ONONE-NEXT: MoveOnly 2 move-created
// CHECK-DASH-ONONE-NEXT: MoveOnly 1 destroyed
// CHECK-DASH-ONONE-NEXT: MoveOnly 2 destroyed
}

func copyable1() {
let x = Copyable()
// CHECK-DASH-ONONE-NEXT: Copyable 0 created
// CHECK-DASH-O-NEXT: Copyable 0 created
byRValueRef(consuming: x)
// CHECK-DASH-ONONE-NEXT: Copyable 1 copy-created
// CHECK-DASH-O-NEXT: Copyable 1 copy-created
// CHECK-DASH-ONONE-NEXT: Copyable 1 destroyed
// CHECK-DASH-O-NEXT: Copyable 1 destroyed
// CHECK-DASH-ONONE-NEXT: Copyable 0 destroyed
// CHECK-DASH-O-NEXT: Copyable 0 destroyed
}

func copyable2() {
let x4 = Copyable()
// CHECK-DASH-ONONE-NEXT: Copyable 0 created
// CHECK-DASH-O-NEXT: Copyable 0 created
consume(x4)
// CHECK-DASH-ONONE-NEXT: Copyable 1 copy-created
// CHECK-DASH-ONONE-NEXT: Copyable 2 move-created
// CHECK-DASH-ONONE-NEXT: Copyable 1 destroyed
// CHECK-DASH-ONONE-NEXT: Copyable 2 destroyed
// CHECK-DASH-ONONE-NEXT: Copyable 0 destroyed
// CHECK-DASH-O-NEXT: Copyable 0 destroyed
}

func main() {
moveOnly1()
moveOnly2()
copyable1()
copyable2()
}

main()