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

zoecarver · 2020-06-10T02:58:43Z

This patch adds support for custom C++ destructors. The most notable thing here, I think, is that this is the first place a struct type has a custom destructor. I suspect with more code we will expose a few places where optimization passes need to be fixed to account for this.

One of many patches to fix SR-12797.

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

test/Interop/Cxx/destructor/Inputs/basic.h

lib/AST/ASTVerifier.cpp

lib/ClangImporter/ImportDecl.cpp

gribozavr · 2020-06-10T15:26:58Z

lib/SIL/IR/TypeLowering.cpp

@@ -1455,6 +1455,13 @@ namespace {
        properties.setAddressOnly();
      }

+      // If a struct has a destructor, it's not trivial. Currently this is only


I think it is redundant with the code right above -- if (D->isCxxNotTriviallyCopyable()). However, you're raising a good point -- should we also set the non-trivial flag? Seems like trivial and address-only is an invalid combination of flags, and we are setting that combination now. If you agree, could you send that fix as a separate PR?

On the other hand, if we are building towards the future Swift feature of move-only structs then checking for the destructor makes sense. However, I'm not sure that Swift move-only structs would necessarily be non-trivial. They can be certainly trivial to move (imagine a Swift equivalent of std::unique_ptr). So I'm reluctant to add code that checks for non-C++ destructors at this point, we know too little about the future Swift feature yet.

should we also set the non-trivial flag?

Yes. This is very important. Otherwise, the compiler won't create or will remove calls to destroy_addr which will mean the destructor will not be invoked.

Seems like trivial and address-only is an invalid combination of flags

I don't think this is the case. I don't see any reason we couldn't have a trivial, address-only type.

So I'm reluctant to add code that checks for non-C++ destructors at this point, we know too little about the future Swift feature yet.

I see the concern but, I think the risk of omitting destroy_addr calls is greater than the risk of uncertainty in the future. If and when Swift changes its mind about structs with custom destructors, there's no reason this code couldn't be updated.

That being said, it might just be easier to set the properties flag based on isCxxNotTriviallyCopyable.

So I'm reluctant to add code that checks for non-C++ destructors at this point, we know too little about the future Swift feature yet.

Thinking about this more, I think you're right. I'll update this to check isCxxNotTriviallyCopyable and remove the llvm::any_of.

I don't think this is the case. I don't see any reason we couldn't have a trivial, address-only type.

Could you provide a sample use case? What would prevent a trivial type from being loadable?

Here's an example:

struct Foo { unsigned x : 3; };

That type is trivial and also address-only. If you remove the bitfield you'll see the SIL gen produces a builtin "zeroInitializer"<Foo>() : $Foo instruction whereas with the bitfield (as above) it is stored on the stack.

However, there are no destory_addrs indicating that this type is trivial (and if you want, I suspect the compiler would confirm this if I add an assertion or something to double-check).

gribozavr · 2020-06-10T15:38:13Z

lib/SILGen/SILGenStmt.cpp

-      tupleInit->SubInitializations.push_back(std::move(eltInit));
-    }
+    // If this is an empty tuple, we don't want to decompose it.
+    if (resultTupleType.getElementTypes().size()) {


Could you provide some information about why this change was needed? Was it necessary for destructors to work (if so, how does it come up?), or is it just a drive-by peephole optimization? From my reading of TupleInitialization it seems like it should handle the empty tuple.

Yeah, this as actually causing issues (probably should have put that in the comment). When we have an empty tuple, we get the wrong return type later on. Because the value is returned with an indirect result, the return type should be @out Foo but instead is () (because that's what's returned).

I think this should now be part of #32402. I'll leave it in this PR too (just so the tests don't crash) but, I think #32402 is where review/discussion should happen.

Thanks! Now that the other PR is merged, could you rebase to eliminate this diff from this PR?

lib/SIL/IR/SILDeclRef.cpp

test/Interop/Cxx/destructor/Inputs/basic.h

test/Interop/Cxx/destructor/basic.swift

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

test/Interop/Cxx/destructor/IRGen.swift

test/Interop/Cxx/destructor/basic.swift

test/Interop/Cxx/destructor/Inputs/basic.h

gribozavr · 2020-06-15T13:54:08Z

lib/SIL/IR/TypeLowering.cpp

@@ -1453,6 +1453,7 @@ namespace {

      if (D->isCxxNotTriviallyCopyable()) {
        properties.setAddressOnly();
+        properties.setNonTrivial();


Is it possible to factor out this fix into a separate PR and write a test for it?

#32402 done :)

Thanks! Now that PR is merged, would you mind rebasing to minimize the diff?

test/Interop/Cxx/destructor/basic.swift

lib/AST/ASTVerifier.cpp

lib/IRGen/GenStruct.h

gribozavr · 2021-01-15T10:48:29Z

test/Interop/Cxx/value-witness-table/custom-virtual-destructors.swift

@@ -0,0 +1,61 @@
+// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-cxx-interop -Xcc -fno-rtti)


Thanks for repeating the comment. Each file will likely live its own life, and figuring out a couple of years from now why there is an -fno-rtti in one of the files but not in others would be difficult if we didn't have local comments.

test/Interop/Cxx/value-witness-table/custom-virtual-destructors-irgen.swift

test/Interop/Cxx/value-witness-table/custom-virtual-destructors.swift

test/Interop/Cxx/value-witness-table/custom-destructors.swift

test/Interop/Cxx/value-witness-table/Inputs/custom-destructor.h

gribozavr · 2021-01-15T10:52:36Z

test/Interop/Cxx/value-witness-table/Inputs/custom-destructor.h

+  // we try to delete the base object. Until we can link against libc++, use
+  // this dummy implementation.
+  static void operator delete(void *p) { __builtin_unreachable(); }
+  virtual ~HasVirtualDestructor(){};


Suggested change

virtual ~HasVirtualDestructor(){};

virtual ~HasVirtualDestructor() {}

Fixed. I agree that this is better, but I also like to use clang-format on all my changes, and clang-format seems to disagree about this. I'm going to look into fixing our clang-format config so that this is formatted correctly.

test/Interop/Cxx/value-witness-table/Inputs/custom-destructor.h

test/Interop/Cxx/class/destructor-correct-abi-irgen.swift

test/Interop/Cxx/value-witness-table/non-trivial-implicit-destructor-irgen.swift

test/Interop/Cxx/value-witness-table/custom-destructors.swift

zoecarver · 2021-01-17T20:20:49Z

@swift-ci please smoke test.

test/Interop/Cxx/class/non-trivial-implicit-destructor-irgen.swift

test/Interop/Cxx/value-witness-table/Inputs/custom-destructors.h

test/Interop/Cxx/value-witness-table/custom-virtual-destructors-irgen.swift

test/Interop/Cxx/value-witness-table/custom-virtual-destructors.swift

gribozavr · 2021-01-18T15:11:48Z

test/Interop/Cxx/value-witness-table/custom-destructors-irgen.swift

@@ -0,0 +1,41 @@
+// RUN: %target-swift-frontend -enable-cxx-interop -I %S/Inputs %s -emit-ir | %FileCheck %s


Assuming you rename the other test to custom-destructors-virtual-irgen.swift, please rename this one to custom-destructors-non-virtual-irgen.swift.

I'm going to do the same thing with custom-destructors.swift -> custom-destructors-non-virtual.swift

test/Interop/Cxx/value-witness-table/Inputs/module.modulemap

zoecarver · 2021-01-19T20:37:34Z

@swift-ci please test.

zoecarver · 2021-01-19T20:37:52Z

@swift-ci please test Windows.

zoecarver · 2021-01-19T20:38:23Z

@swift-ci please test Windows.

swift-ci · 2021-01-19T22:03:24Z

Build failed
Swift Test Linux Platform
Git Sha - a49aef2d5c3dd1a17ca4b513fe740002900b7706

zoecarver · 2021-01-20T06:02:49Z

@swift-ci please test Windows.

zoecarver · 2021-01-20T06:02:57Z

@swift-ci please test Linux.

zoecarver · 2021-01-20T06:03:29Z

@swift-ci please test Linux.

swift-ci · 2021-01-20T07:18:02Z

Build failed
Swift Test Linux Platform
Git Sha - a49aef2d5c3dd1a17ca4b513fe740002900b7706

test/Interop/Cxx/value-witness-table/custom-destructors-non-virtual.swift

This patch adds support for custom C++ destructors. The most notable thing here, I think, is that this is the first place a struct type has a custom destructor. I suspect with more code we will expose a few places where optimization passes need to be fixed to account for this. One of many patches to fix SR-12797.

zoecarver · 2021-01-27T20:55:44Z

@swift-ci please test Linux.

swift-ci · 2021-01-27T22:08:11Z

Build failed
Swift Test Linux Platform
Git Sha - ffa0d1c

zoecarver · 2021-01-27T22:17:19Z

@swift-ci please test Linux.

zoecarver · 2021-01-28T04:39:01Z

@swift-ci please test macOS.

zoecarver · 2021-01-28T04:39:10Z

@swift-ci please test Windows.

zoecarver · 2021-01-28T17:37:07Z

@swift-ci please test Windows.

zoecarver · 2021-01-28T17:59:04Z

I'm going to land this as soon as the Windows bot finishes.

Adds `CodeGen::getCXXDestructorImplicitParam`, to retrieve a C++ destructor's implicit parameter (after the "this" pointer) based on the ABI in the given CodeGenModule. This will allow other frontends (Swift, for example) to easily emit calls to object destructors with correct ABI semantics and calling convetions. This is needed for Swift C++ interop. Here's the corresponding Swift change: swiftlang/swift#32291 Differential Revision: https://reviews.llvm.org/D82392

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

zoecarver requested review from gribozavr and MForster June 10, 2020 02:58

zoecarver force-pushed the cxx/irgen/destructor branch from fcee2ad to 2a6fc7e Compare June 10, 2020 02:59

gottesmm reviewed Jun 10, 2020

View reviewed changes

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp Outdated Show resolved Hide resolved

gribozavr reviewed Jun 10, 2020

View reviewed changes

zoecarver force-pushed the cxx/irgen/destructor branch 2 times, most recently from 7ec0794 to 77463ea Compare June 10, 2020 17:50

zoecarver requested review from gottesmm and gribozavr June 10, 2020 18:04

zoecarver force-pushed the cxx/irgen/destructor branch from 77463ea to 6aaf4a3 Compare June 10, 2020 18:04

gottesmm reviewed Jun 10, 2020

View reviewed changes

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp Outdated Show resolved Hide resolved

zoecarver mentioned this pull request Jun 11, 2020

[opt] Re-work broadenSingleElementStores to use projections. #32318

Merged

zoecarver force-pushed the cxx/irgen/destructor branch from 78b0ef5 to 4d81be9 Compare June 12, 2020 17:49