[cxx-interop] Use user defined copy constructor to copy C++ objects. #32378

zoecarver · 2020-06-14T20:24:40Z

Calls user-defined copy constructor when present. Based on #30630 (probably best to wait until that lands to review this).

zoecarver · 2020-06-14T20:25:55Z

lib/AST/ClangTypeConverter.cpp

@@ -410,7 +410,19 @@ clang::QualType ClangTypeConverter::visitProtocolType(ProtocolType *type) {
 // Metatypes can be converted to Class when they are metatypes for concrete


In case it wasn't clear from the summary, most of the changes here are from #30630.

martinboehme

Thanks! I was going to do this at some point, but hadn't gotten round to it yet.

lib/ClangImporter/ImportDecl.cpp

lib/IRGen/GenStruct.cpp

test/Interop/Cxx/constructor/copy-constructor-irgen.swift

test/Interop/Cxx/constructor/copy-basic.swift

test/Interop/Cxx/constructor/copy-constructor-irgen.swift

zoecarver · 2020-06-16T05:40:28Z

Thanks for the reviews! I fixed a few of the comments. I'll fix the rest tomorrow (and add some related witness table changes from #32291).

zoecarver · 2020-06-17T16:08:44Z

@gribozavr and @martinboehme thank you for your comments and patience. I think I've (finally) resolved all the comments (except for the thunk/Microsoft ABI issue). Sorry for the delay.

martinboehme

Responding to one of your code-level comments here, as github apparently won't allow me to respond to the comment thread any more:

We could also import it as a __double_underscore method which should help with overload resolution. Then just use it's function signature and ignore it.

Yes, something like that I think. I'm not super-familiar with the rules that Swift has here (to make sure we're not generating a name that could conflict with anything else) -- @gribozavr can probably say more about this.

Alternatively, we could try to build the "correct" function type in IRGen and then use that in the signature argument.

Yes, that's what we would need to do. But I think that would end up being quite a bit more code than the alternative we're discussing above. I've also pointed out an additional name-mangling issue in one of my new comments.

I think it's easier to just import the copy constructor as a SIL function, emit a call to that, and have the existing code take care of the rest.

lib/ClangImporter/ImportDecl.cpp

martinboehme · 2020-06-18T06:15:37Z

lib/ClangImporter/ImportDecl.cpp

+            if (ctor->isDeleted())
+              return nullptr;
+          }
+          if (ctor->getAccess() != clang::AS_public) {


I have to admit I don't understand the logic that existed here before this PR, and which was introduced by #31707.

AFAICT, the definition of "trivially copyable" says nothing about the access level of the copy constructor. It does say something about the copy constructor being deleted, namely that this is compatible with the class being trivially copyable.

Beyond that, I'm not sure why simply taking the result of cxxRecordDecl->isTriviallyCopyable() isn't sufficient here.

@MForster I think you introduced this logic -- can you comment?

[class.prop]/p1 defines a trivially copyable class as a class, "that has at least one eligible copy constructor..." I think the important word there is eligible. A private copy constructor is not eligible.

I don't think that's how C++ defines "eligible":

https://eel.is/c++draft/class#special-6

True. However, I think we have to still check for public access level, since it would be otherwise quite weird that Swift can use a private constructor -- even if it is trivial.

lib/ClangImporter/ImportDecl.cpp

martinboehme · 2020-06-18T06:39:22Z

test/Interop/Cxx/class/copy-constructor-thunk.swift

@@ -0,0 +1,21 @@
+// Target-specific tests for C++ copy constructor code generation.


@gribozavr Did you really intend that this test should be in the class (not value-witness-table) directory?

This forces us to duplicate the definitions of the C++ classes between the two directories, which seems suboptimal.

test/Interop/Cxx/value-witness-table/copy-basic.swift

martinboehme · 2020-06-18T06:54:35Z

test/Interop/Cxx/value-witness-table/copy-constructor-irgen.swift

+// RUN: %target-swift-frontend -enable-cxx-interop -I %S/Inputs %s -emit-ir | %FileCheck %s
+
+// REQUIRES: CPU=x86_64
+// REQUIRES: objc_interop


It's a shame that this test needs to be so platform-specific.

If you change the definitions of your C++ test classes a bit, I think you should be able to use a strategy similar to the one used here:

https://github.com/apple/swift/pull/30630/files#diff-496932cc8ce48ca2aa1c0a25fd7d1d8b

This would then also allow you to tests for platforms that require implicit parameters to be inserted.

I think the main obstacle to importing your current C++ classes without a stdlib present is that they use the C++ type int in the interface, and the corresponding CInt is defined in stdlib. So I'd suggest defining variants of these classes that don't use int in the interface, and using them for this test.

Even without an int member, they need the stdlib to represent the reference types (with an UnsafePointer). I tried updating this test for multiple targets but, I don't think this will work (without the stdlib). I even tried mocking up a basic UnsafePointer in the test file but, that didn't seem to work either (and it seems very fragile).

Another possibility is I could duplicate this test and have a REQUIRES comment for each target (i.e., have three tests for three targets). WDTY the best solution is?

zoecarver · 2020-06-24T02:31:50Z

Yes, that's what we would need to do. But I think that would end up being quite a bit more code than the alternative we're discussing above. I've also pointed out an additional name-mangling issue in one of my new comments.

I figured out all we really need to do is create the SIL function type and that didn't seem to be too much work. I also got it working by importing the constructor like any other, so if you'd rather we can do that (but I like this way better).

FWIW one issue I ran into when importing the constructor as a SIL function is that Swift will try to remove the function unless there is a use of it (or maybe it just lazily SILGens it-- either way it doesn't seem to exist when I dump the SIL if I don't explicitly call it somewhere).

shahmishal · 2020-10-01T06:55:08Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

zoecarver · 2020-11-01T18:59:16Z

@swift-ci please smoke test Linux platform.

zoecarver · 2020-11-28T08:01:17Z

@swift-ci please test

zoecarver · 2020-11-28T08:01:56Z

@swift-ci please test

zoecarver · 2020-11-28T08:02:13Z

@hlopko and @gribozavr friendly ping. This is ready for review when you're ready to review it :)

swift-ci · 2020-11-28T09:02:08Z

Build failed
Swift Test OS X Platform
Git Sha - 1d2ac7860d014eaf5776a20d1459f8d26e879710

swift-ci · 2020-11-28T10:23:53Z

Build failed
Swift Test Linux Platform
Git Sha - 1d2ac7860d014eaf5776a20d1459f8d26e879710

zoecarver · 2020-11-28T21:47:53Z

stdlib/public/SwiftShims/RefCount.h

@@ -18,7 +18,7 @@
 // This definition is a placeholder for importing into Swift.
 // It provides size and alignment but cannot be manipulated safely there.
 typedef struct {
-  __swift_uintptr_t refCounts SWIFT_ATTRIBUTE_UNAVAILABLE;


So, in C-mode this attribute is meaningless which is why we didn't see the error before now. If we want to make sure that in runtime mode we don't use this type, I'll need to create a separate and more involved fix. Let me know if that needs to be done.

Another "fix" could be to only add SWIFT_ATTRIBUTE_UNAVAILABLE when ifndef __swift__ .

So, in C-mode this attribute is meaningless which is why we didn't see the error before now.

Which error?

When we have a reference type, for example an array, we try to add an implicit copy constructor in C++ mode. When we try to do this, we get an error that refCounts is unreachable:

swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/RefCount.h:20:9: error: 'refCounts' is unavailable typedef struct { ^ swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/HeapObject.h:45:8: note: in implicit copy constructor for 'InlineRefCountsPlaceholder' first required here struct HeapObject { ^ swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/GlobalObjects.h:38:8: note: in implicit copy constructor for 'HeapObject' first required here struct _SwiftEmptyArrayStorage { ^ <unknown>:0: note: in implicit copy constructor for '_SwiftEmptyArrayStorage' first required here swift-source/build/Ninja-ReleaseAssert/swift-macosx-arm64/lib/swift/shims/RefCount.h:21:21: note: 'refCounts' has been explicitly marked unavailable here __swift_uintptr_t refCounts SWIFT_ATTRIBUTE_UNAVAILABLE;

zoecarver · 2020-11-28T21:48:36Z

@swift-ci please test

lib/ClangImporter/ImportDecl.cpp

gribozavr · 2021-01-18T13:46:39Z

lib/ClangImporter/ImportDecl.cpp

+            if (ctor->isDeleted())
+              return nullptr;
+          }
+          if (ctor->getAccess() != clang::AS_public) {


True. However, I think we have to still check for public access level, since it would be otherwise quite weird that Swift can use a private constructor -- even if it is trivial.

gribozavr · 2021-01-18T13:53:51Z

test/Interop/Cxx/class/type-classification-loadable-silgen.swift

@@ -54,17 +54,17 @@ func pass(s: StructWithSubobjectDefaultedCopyConstructor) {

 // CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithPrivateDefaultedCopyConstructor)
 func pass(s: StructWithPrivateDefaultedCopyConstructor) {
-  // CHECK: bb0(%0 : $*StructWithPrivateDefaultedCopyConstructor):


Agreed. However, you added this code:

if (ctor->isDeleted() || ctor->getAccess() != clang::AS_public) return nullptr;

shouldn't it take care of rejecting it based on the access control level?

gribozavr · 2021-01-18T14:06:19Z

test/Interop/Cxx/class/type-classification-loadable-silgen.swift

 }

 // CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithInheritedPrivateDefaultedCopyConstructor)
 func pass(s: StructWithInheritedPrivateDefaultedCopyConstructor) {
-  // CHECK: bb0(%0 : $*StructWithInheritedPrivateDefaultedCopyConstructor):
+  // CHECK: bb0(%0 : $StructWithInheritedPrivateDefaultedCopyConstructor):


There's code in the importer that should make a type address-only if there is any private constructor, do you know why that is not kicking in?

gribozavr · 2021-01-18T14:12:58Z

stdlib/public/SwiftShims/RefCount.h

@@ -18,7 +18,7 @@
 // This definition is a placeholder for importing into Swift.
 // It provides size and alignment but cannot be manipulated safely there.
 typedef struct {
-  __swift_uintptr_t refCounts SWIFT_ATTRIBUTE_UNAVAILABLE;


So, in C-mode this attribute is meaningless which is why we didn't see the error before now.

Which error?

test/Interop/Cxx/value-witness-table/Inputs/basic.h

test/Interop/Cxx/value-witness-table/copy-basic.swift

test/Interop/Cxx/value-witness-table/copy-constructor-irgen.swift

test/Interop/Cxx/class/copy-constructor-abi.swift

lib/IRGen/GenStruct.cpp

zoecarver · 2021-01-26T20:53:38Z

@swift-ci please smoke test.

zoecarver · 2021-01-26T20:54:17Z

@swift-ci please smoke test macOS.

zoecarver · 2021-01-26T20:54:24Z

@swift-ci please smoke test Windows.

zoecarver · 2021-01-27T04:11:58Z

@swift-ci please smoke test Windows.

zoecarver · 2021-01-27T06:05:46Z

@swift-ci please smoke test.

zoecarver · 2021-01-27T21:32:53Z

@swift-ci please smoke test Windows.

zoecarver · 2021-01-31T21:22:13Z

@swift-ci please smoke test.

zoecarver · 2021-02-01T02:37:08Z

@gribozavr tests are passing and suggestions have been applied. Any more comments?

test/Interop/Cxx/class/type-classification-module-interface.swift

If a user-defined copy constructor exists, use that to copy imported C++ types.

zoecarver · 2021-02-03T22:34:37Z

@swift-ci please test.

zoecarver · 2021-02-04T00:07:28Z

@swift-ci please test Windows platform.

zoecarver added the c++ interop Feature: Interoperability with C++ label Jun 14, 2020

zoecarver requested review from gribozavr, MForster and martinboehme June 14, 2020 20:24

zoecarver marked this pull request as draft June 14, 2020 20:25

zoecarver commented Jun 14, 2020

View reviewed changes

martinboehme suggested changes Jun 15, 2020

View reviewed changes

gribozavr reviewed Jun 15, 2020

View reviewed changes

test/Interop/Cxx/constructor/copy-basic.swift Outdated Show resolved Hide resolved

gribozavr reviewed Jun 15, 2020

View reviewed changes

test/Interop/Cxx/constructor/copy-basic.swift Outdated Show resolved Hide resolved

gribozavr reviewed Jun 15, 2020

View reviewed changes

test/Interop/Cxx/constructor/copy-basic.swift Outdated Show resolved Hide resolved

gribozavr reviewed Jun 15, 2020

View reviewed changes

test/Interop/Cxx/constructor/copy-basic.swift Outdated Show resolved Hide resolved

gribozavr reviewed Jun 15, 2020

View reviewed changes

test/Interop/Cxx/constructor/copy-constructor-irgen.swift Outdated Show resolved Hide resolved

gribozavr mentioned this pull request Jun 15, 2020

[cxx-interop] Add support for custom C++ destructors. #32291

Merged

zoecarver force-pushed the cxx/copy-const branch from 8a28baa to 36d8c28 Compare June 16, 2020 05:39

zoecarver force-pushed the cxx/copy-const branch 3 times, most recently from e4fb5c4 to 1ef546b Compare June 17, 2020 15:54

zoecarver requested review from martinboehme and gribozavr June 17, 2020 16:04

zoecarver force-pushed the cxx/copy-const branch from 1ef546b to dbddd9d Compare June 17, 2020 16:07

martinboehme suggested changes Jun 18, 2020

View reviewed changes

zoecarver force-pushed the cxx/copy-const branch 3 times, most recently from 4b87be5 to ecd1674 Compare June 24, 2020 02:15

zoecarver marked this pull request as ready for review June 24, 2020 16:10

zoecarver changed the base branch from master to main October 2, 2020 04:03

zoecarver force-pushed the cxx/copy-const branch from 2b5e0ef to 11c8898 Compare November 28, 2020 07:59

zoecarver force-pushed the cxx/copy-const branch from 11c8898 to 1d2ac78 Compare November 28, 2020 08:01

zoecarver force-pushed the cxx/copy-const branch from 1d2ac78 to 4e6e029 Compare November 28, 2020 21:44

zoecarver commented Nov 28, 2020

View reviewed changes

gribozavr reviewed Jan 18, 2021

View reviewed changes

zoecarver force-pushed the cxx/copy-const branch from 4e6e029 to c33e672 Compare January 26, 2021 20:53

zoecarver force-pushed the cxx/copy-const branch from c33e672 to 4b11c54 Compare January 27, 2021 04:11

zoecarver force-pushed the cxx/copy-const branch from 4b11c54 to de4e80e Compare January 27, 2021 21:32

zoecarver force-pushed the cxx/copy-const branch from de4e80e to 825342d Compare January 31, 2021 21:21

gribozavr approved these changes Feb 3, 2021

View reviewed changes

test/Interop/Cxx/class/type-classification-module-interface.swift Outdated Show resolved Hide resolved

[cxx-interop] Use user defined copy constructor to copy C++ objects.

b2b7f7b

If a user-defined copy constructor exists, use that to copy imported C++ types.

zoecarver force-pushed the cxx/copy-const branch from 825342d to b2b7f7b Compare February 3, 2021 22:34

zoecarver merged commit 74b7341 into swiftlang:main Feb 4, 2021

		@@ -410,7 +410,19 @@ clang::QualType ClangTypeConverter::visitProtocolType(ProtocolType *type) {
		// Metatypes can be converted to Class when they are metatypes for concrete

		@@ -0,0 +1,21 @@
		// Target-specific tests for C++ copy constructor code generation.

[cxx-interop] Use user defined copy constructor to copy C++ objects. #32378

[cxx-interop] Use user defined copy constructor to copy C++ objects. #32378

Conversation

zoecarver commented Jun 14, 2020

Choose a reason for hiding this comment

martinboehme left a comment

Choose a reason for hiding this comment

zoecarver commented Jun 16, 2020

zoecarver commented Jun 17, 2020

martinboehme left a 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

zoecarver commented Jun 24, 2020

shahmishal commented Oct 1, 2020

zoecarver commented Nov 1, 2020

zoecarver commented Nov 28, 2020

zoecarver commented Nov 28, 2020

zoecarver commented Nov 28, 2020

swift-ci commented Nov 28, 2020

swift-ci commented Nov 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoecarver commented Nov 28, 2020

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

zoecarver commented Jan 26, 2021

zoecarver commented Jan 26, 2021

zoecarver commented Jan 26, 2021

zoecarver commented Jan 27, 2021

zoecarver commented Jan 27, 2021

zoecarver commented Jan 27, 2021

zoecarver commented Jan 31, 2021

zoecarver commented Feb 1, 2021

zoecarver commented Feb 3, 2021

zoecarver commented Feb 4, 2021