Add a new parameter convention `@in_cxx` for non-trivial C++ classes that are passed indirectly and destructed by the caller #73019

ahatanaka · 2024-04-12T23:13:38Z

Fix a bug where @in, which indicates the callee is responsible for destroying the passed object, was being used to pass such classes.

rdar://122707697

ahatanaka · 2024-04-12T23:14:12Z

@swift-ci please test

ahatanaka · 2024-04-23T21:06:28Z

@swift-ci please test

ahatanaka · 2024-04-24T00:08:20Z

@swift-ci please test

rjmccall · 2024-04-24T01:53:11Z

SwiftCompilerSources/Sources/SIL/Argument.swift

@@ -347,6 +347,11 @@ public enum ArgumentConvention : CustomStringConvertible {
  /// convention used by mutable captures in @noescape closures.
  case indirectInoutAliasable

+  /// This argument is passed indirectly, i.e. by directly passing the address
+  /// of an object in memory. The callee may modify, but does not destroy the
+  /// object.


Suggested change

/// object.

/// object. This corresponds to the parameter-passing convention of the

/// Itanium C++ ABI, which is used ubiquitously on non-Windows targets.

The description sounds like either inout or inout_aliasable to me. What are the SIL-level semantics of the new convention?

rjmccall · 2024-04-24T01:55:09Z

SwiftCompilerSources/Sources/SIL/Argument.swift

@@ -416,7 +422,8 @@ public enum ArgumentConvention : CustomStringConvertible {
         .packOwned, .packGuaranteed:
      return true
    case .directOwned, .directUnowned, .directGuaranteed,
-         .indirectInout, .indirectInoutAliasable, .indirectOut,
+         .indirectInout, .indirectInoutAliasable, .indirectInCXX,
+         .indirectOut,


I'm not sure what this property is used for, but if it includes both @in and @in_guaranteed, it probably ought to include @in_cxx.

rjmccall · 2024-04-24T01:58:20Z

SwiftCompilerSources/Sources/SIL/FunctionConvention.swift

@@ -164,7 +164,7 @@ public struct ParameterInfo : CustomStringConvertible {
    switch convention {
    case .indirectIn, .indirectInGuaranteed:
      return hasLoweredAddresses || type.isOpenedExistentialWithError()
-    case .indirectInout, .indirectInoutAliasable:
+    case .indirectInout, .indirectInoutAliasable, .indirectInCXX:


I'd like @gottesmm to weigh in here. Naively, I'd say it'd be nice to model these as direct arguments in non-lowered-addresses mode, but the fact that we have to destroy the value in the caller (and observe any mutations made by the callee) might make that impractical.

It's potentially useful to have a SIL convention that says: the caller and callee must observe the same address of an addressable value. But that isn't generally implementable for all T given reabstraction of function types and tuples.

I'm still baffled by what this convention means. The comment says it's about observing mutation. If that's true, then we should pass a SIL address in non-lowered-addresses mode. But why, then aren't you using inout?

If it is not the case that the caller and callee need to observe the same address, then we should pass SIL values instead.

I saw an assertion failure when I tried using inout by making getIndirectCParameterConvention return Indirect_Inout instead of Indirect_In_CXX.

The failed assertion is in ArgEmitter::emit:

assert(arg.hasLValueType() == param.isIndirectInOut());

This is the Itanium C++ ABI convention in which non-trivial C++ values are passed indirectly but the caller is responsible for destroying them. The caller and callee do need to observe the same address. It is essentially the same as inout at the SIL level except that it's not inout in the source language, and I didn't want us to have to weaken all the SIL lowering assertions that e.g. an inout parameter always corresponds to an l-value argument expression, because those are quite useful in catching type-checking bugs. That is why I suggested to Akira that he add a new convention that was mostly treated the same way as inout throughout the SIL pipeline.

We should probably pass SIL arguments to in-cxx by-address at all stages, and make it very clear that the caller can observe mutation.

Can the callee observe aliases?

The parameter object can't be directly named except through the parameter; it's never the same object as e.g. a local variable in the caller. However, it can be aliased in the same way that almost any C++ object technically can, e.g. by escaping this during construction. We've generally been assuming that that's not something we need to worry about, at least from the perspective of "well, we're obviously not going to be enforce exclusivity on types that don't follow basic local-reasoning principles".

It occurs to me that we probably need special logic when consuming parameters like this. They probably should be consumable as long as the type is movable; we should compile consumption as a C++ move and then enforce use restrictions on the moved-from value just like we would for a Swift value, even though technically the object is still there.

I think I'm starting to understand. At the source level, this is pass-by-value into the callee, and the caller has no visibility of that value. But, since the convention requires the caller to destroy the value, the caller's implementation must also observe any mutation.

Just like @in, In ownership SIL (OSSA) the argument should be consumed by the call (not by a separate destroy after the call).

Just like @in, non-address-lowered SIL should pass the value, not an address.

OwnershipModelEliminator could insert a destroy_addr after the call for these arguments. By then, they will be addresses. And IRGen doesn't need to do anything special.

rjmccall · 2024-04-24T02:00:02Z

include/swift/AST/Types.h

@@ -4059,6 +4059,11 @@ enum class ParameterConvention : uint8_t {
  /// convention used by mutable captures in @noescape closures.
  Indirect_InoutAliasable,

+  /// This argument is passed indirectly, i.e. by directly passing the address
+  /// of an object in memory. The callee may modify, but does not destroy the
+  /// object.


Suggested change

/// object.

/// object. This corresponds to the parameter-passing convention of the

/// Itanium C++ ABI, which is used ubiquitously on non-Windows targets.

rjmccall · 2024-04-24T02:04:51Z

include/swift/SIL/SILDeclRef.h

@@ -200,6 +200,8 @@ struct SILDeclRef {
  /// Set if this is for an async let closure.
  unsigned isAsyncLetClosure : 1;

+  unsigned CFunctionPointer : 1;


Can you explain why this is necessary?

I'm trying to get getAbstractionPatternForConstant to return an AbstractionPattern with a clang type.

Without this change, checkForABIDifferences in convertCFunctionSignature returns TypeConverter::ABIDifference::NeedsThunk because loweredResultTy and loweredDestTy don't match, which causes the compiler to reject the code. loweredDestTy's parameter type is @in_cxx and I think loweredResultTy's parameter type would be @in_guaranteed without the change.

I ran into this problem when I was testing testClosureToFuncPtr in closure-thunk-macosx.swift, which passes a swift closure to a C++ function taking a function pointer.

The Conventions object for a foreign parameter of one of these types should always return @in_cxx, whether it appears as part of a C function pointer, a block, an ObjC method, or whatever. You should be able to just do this in getIndirectCParameterConvention.

This was needed to fix a preexisting bug, but I think it should be fixed in a separate patch and it can be fixed without making any changes to SILDeclRef.

#73299

rjmccall · 2024-04-24T02:05:43Z

lib/Demangling/Demangler.cpp

@@ -2160,6 +2160,7 @@ NodePointer Demangler::demangleImplParamConvention(Node::Kind ConvKind) {
    case 'l': attr = "@inout"; break;
    case 'b': attr = "@inout_aliasable"; break;
    case 'n': attr = "@in_guaranteed"; break;
+    case 'C': attr = "@in_cxx"; break;


This should be documented in docs/ABI/Mangling.rst.

Also, please add a test in https://github.com/apple/swift/blob/main/test/Demangle/Inputs/manglings.txt

ahatanaka · 2024-05-02T21:24:49Z

@swift-ci please test

ahatanaka · 2024-05-02T21:35:02Z

The updated patch treats @in_cxx as consumed arguments just like @in (except that it can be mutated) and inserts destroy_addr right before IRGen.

ahatanaka · 2024-05-02T21:37:40Z

test/Interop/Cxx/class/function-call-irgen.swift

+// CHECK: call void @llvm.lifetime.start.p0(i64 8, ptr %[[V0]])
+// CHECK: call ptr @__swift_cxx_ctor_ZN10NonTrivialC1Ev(ptr %[[V0]])
+// CHECK: invoke void %[[V1]](ptr %[[V0]])
+// CHECK: to label %[[INVOKE_CONT:.*]] unwind label %{{.*}}


I think we can turn invoke into call and simplify the IR if we fix this bug: #73396

ahatanaka · 2024-05-02T21:44:07Z

lib/SILGen/SILGenBridging.cpp

@@ -1416,7 +1421,11 @@ emitObjCThunkArguments(SILGenFunction &SGF, SILLocation loc, SILDeclRef thunk,
      arg = emitObjCUnconsumedArgument(SGF, loc, arg);
    }

-    auto managedArg = SGF.emitManagedRValueWithCleanup(arg);
+    // Don't emit a cleanup if the argument is @in_cxx.


I tried to use @in_guaranteed instead of @in_cxx for thunk parameters so that we don't have to make this change, but ran into a few issues.

ahatanaka · 2024-05-03T06:14:06Z

@swift-ci please test

atrick

I'm not completely confident about the SILGen changes. Could you please make sure that any tests specific to this feature also work with -enable-sil-opaque-values. We're trying to enable that by default now and can't keep adding new problems to debug.

atrick · 2024-05-15T00:33:20Z

lib/SILGen/SILGenBridging.cpp

@@ -325,6 +325,10 @@ static ManagedValue emitManagedParameter(SILGenFunction &SGF, SILLocation loc,
      return ManagedValue::forBorrowedAddressRValue(value);
    }

+  case ParameterConvention::Indirect_In_CXX:
+    // Don't emit a cleanup if the parameter is @in_cxx.
+    return ManagedValue::forOwnedRValue(value, CleanupHandle::invalid());


@nate-chandler could you take a quick peek at just the SILGen changes to see if the make sense to you?

As far as SIL opaque values goes, an easy thing to check would be whether the tests [added in this PR] crash out of SILGen with -enable-sil-opaque-values. Treating such a parameter as owned while suppressing its destruction seems to make sense given the problem being solved.

I see crashes when I add -enable-sil-opaque-values.

It crashes in SILValueOwnershipChecker::checkFunctionArgWithoutLifetimeEndingUses with the error message Owned function parameter without life ending uses! when I compile closure-thunk-executable-macosx.swift.

This is the SIL that's causing the crash:

// thunk for @escaping @callee_guaranteed (@in_guaranteed NonTrivial) -> () sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sSo10NonTrivialVIegn_ABIeyBC_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> (), @in_cxx NonTrivial) -> () { // %0 // user: %2 // %1 // user: %5 bb0(%0 : $*@block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> (), %1 : @owned $NonTrivial): %2 = project_block_storage %0 : $*@block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> () // user: %3 %3 = load [copy] %2 : $*@callee_guaranteed (@in_guaranteed NonTrivial) -> () // users: %8, %4 %4 = begin_borrow %3 : $@callee_guaranteed (@in_guaranteed NonTrivial) -> () // users: %6, %5 %5 = apply %4(%1) : $@callee_guaranteed (@in_guaranteed NonTrivial) -> () end_borrow %4 : $@callee_guaranteed (@in_guaranteed NonTrivial) -> () // id: %6 %7 = tuple () // user: %9 destroy_value %3 : $@callee_guaranteed (@in_guaranteed NonTrivial) -> () // id: %8 return %7 : $() // id: %9 } // end sil function '$sSo10NonTrivialVIegn_ABIeyBC_TR'

This is a thunk function for the block.

It seems that the checker is complaining that the second argument of the thunk (@in_cxx NonTrivial) doesn't have lifetime-ending uses.

I wonder changing the thunk's parameter to @in_guaranteed NonTrivial would fix the crash. Alternatively, maybe emitObjCThunkArguments should emit the cleanup instead of suppressing it and instead have destroy_addr removed before IRGen.

ahatanaka · 2024-05-17T20:23:33Z

@swift-ci please test

ahatanaka · 2024-05-17T23:38:57Z

@swift-ci please test

ahatanaka · 2024-05-18T04:11:26Z

@swift-ci please test

that are passed indirectly and destructed by the caller Fix a bug where `@in`, which indicates the callee is responsible for destroying the passed object, was being used to pass such classes. rdar://122707697

@in

- @in_cxx is handled the same way as @in in callers and @in_guaranteed in callees (except for mutation). Fix checks in various places accordingly. - Emit copies of @in_cxx parameters and destroy them in thunks.

@convention

'C' is already taken by @convention(c).

ahatanaka · 2024-05-29T19:01:39Z

@swift-ci please test

…ion if it's used by a function taking a @in_cxx parameter

ahatanaka · 2024-05-30T04:07:47Z

@swift-ci please test

ahatanaka · 2024-05-30T15:14:59Z

@swift-ci please test

ahatanaka · 2024-05-30T20:56:56Z

@swift-ci please test

ahatanaka requested review from zoecarver, hyp, egorzhdan, kavon, ktoso, hborla, slavapestov, xedin and tshortli as code owners April 12, 2024 23:13

ahatanaka requested review from jckarter, gottesmm, rjmccall and atrick April 12, 2024 23:16

egorzhdan added the c++ interop Feature: Interoperability with C++ label Apr 14, 2024

ahatanaka requested review from ravikandhadai, beccadax and fahadnayyar April 15, 2024 17:30

ahatanaka mentioned this pull request Apr 15, 2024

[SILGen] Fix a bug where the wrong convention was being used for lowering a closure to a C++ function pointer #73039

Merged

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from b1a8d3b to c6c9427 Compare April 23, 2024 21:03

ahatanaka requested a review from eeckstein as a code owner April 23, 2024 21:03

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from c6c9427 to 4c26cbb Compare April 23, 2024 23:56

rjmccall reviewed Apr 24, 2024

View reviewed changes

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from 4c26cbb to 198dd06 Compare May 2, 2024 21:23

ahatanaka commented May 2, 2024

View reviewed changes

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from b9db498 to 4c4cc4f Compare May 3, 2024 06:13

atrick reviewed May 15, 2024

View reviewed changes

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from 4c4cc4f to 0b6e7b6 Compare May 17, 2024 20:23

ahatanaka requested review from AnthonyLatsis, ahoppen, bnbarham, CodaFi, hamishknight and rintaro as code owners May 17, 2024 20:23

ahatanaka added 7 commits May 29, 2024 11:51

Add a new parameter convention @in_cxx for non-trivial C++ classes

782e01b

that are passed indirectly and destructed by the caller Fix a bug where `@in`, which indicates the callee is responsible for destroying the passed object, was being used to pass such classes. rdar://122707697

Address feedback from @atrick and @nate-chandler

a16ba67

- @in_cxx is handled the same way as @in in callers and @in_guaranteed in callees (except for mutation). Fix checks in various places accordingly. - Emit copies of @in_cxx parameters and destroy them in thunks.

Add type attribute for @in_cxx

d35b0a2

Emit destroy_addr when inlining a function that has @in_cxx parameters

cd56ed2

Fix mangling of @in_cxx

65d4ddc

'C' is already taken by @convention(c).

Add missing case

dbe228e

Fix function names in tests after mangling change

dab4b05

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from 45a5dc2 to dab4b05 Compare May 29, 2024 19:01

ahatanaka added 2 commits May 29, 2024 21:00

Fix tests

788f69a

Don't eliminate a copy_addr instruction in Temporary RValue Optimizat…

65567b5

…ion if it's used by a function taking a @in_cxx parameter

Fix assertion failure

09d7e35

Fix check strings

64044f1

	/// object.
	/// object. This corresponds to the parameter-passing convention of the
	/// Itanium C++ ABI, which is used ubiquitously on non-Windows targets.

Add a new parameter convention @in_cxx for non-trivial C++ classes that are passed indirectly and destructed by the caller #73019

Are you sure you want to change the base?

Add a new parameter convention @in_cxx for non-trivial C++ classes that are passed indirectly and destructed by the caller #73019

Conversation

ahatanaka commented Apr 12, 2024

ahatanaka commented Apr 12, 2024

ahatanaka commented Apr 23, 2024

ahatanaka commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmccall Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmccall Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahatanaka commented May 2, 2024

ahatanaka commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahatanaka commented May 3, 2024

atrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nate-chandler May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahatanaka commented May 17, 2024

ahatanaka commented May 17, 2024

ahatanaka commented May 18, 2024

ahatanaka commented May 29, 2024

ahatanaka commented May 30, 2024

ahatanaka commented May 30, 2024

ahatanaka commented May 30, 2024

Add a new parameter convention `@in_cxx` for non-trivial C++ classes that are passed indirectly and destructed by the caller #73019

Add a new parameter convention `@in_cxx` for non-trivial C++ classes that are passed indirectly and destructed by the caller #73019

rjmccall Apr 25, 2024 •

edited

rjmccall Apr 25, 2024 •

edited

nate-chandler May 15, 2024 •

edited