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-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

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

@swift-ci please test

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 · 2024-06-18T18:09:46Z

@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).

nate-chandler

LGTM

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 requested review from AnthonyLatsis, ahoppen, bnbarham, CodaFi, hamishknight and rintaro as code owners May 17, 2024 20:23

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

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from 64044f1 to e890ce6 Compare June 18, 2024 18:08

ahatanaka added 10 commits June 26, 2024 11:29

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

e86a0a0

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

8b63e98

- @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

4dfb1e0

Emit destroy_addr when inlining a function that has @in_cxx parameters

191dc92

Fix mangling of @in_cxx

187a95b

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

Add missing case

3d102ed

Fix function names in tests after mangling change

cf33eef

Fix tests

be51347

Fix check strings

242ee1b

Fix isConsumedParameter, etc.

4d6a47f

ahatanaka force-pushed the non-trivial-cxx-class-convention branch from e890ce6 to 4d6a47f Compare June 26, 2024 18:30

nate-chandler approved these changes Jun 27, 2024

View reviewed changes

ahatanaka merged commit 42bc49d into main Jun 27, 2024
5 checks passed

ahatanaka deleted the non-trivial-cxx-class-convention branch June 27, 2024 16:44

	/// 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

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 Loading

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 Loading

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 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

ahatanaka commented Jun 18, 2024

nate-chandler left a comment

Choose a reason for hiding this comment

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

Loading

rjmccall Apr 25, 2024 •

edited

Loading