Skip to content

[cxx-interop] Mark types with a destructor a non-trivial. #32402

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

Jump to bottom
Merged
merged 1 commit into from
Jul 10, 2020
Merged

[cxx-interop] Mark types with a destructor a non-trivial. #32402

merged 1 commit into from
Jul 10, 2020

Conversation

zoecarver
Copy link
Contributor

Any C++ type that isn't trivially copyable is now also not a trivial type. This will preserve the destructor (among other things).

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Jun 16, 2020
@zoecarver zoecarver requested a review from gribozavr June 16, 2020 02:52
@zoecarver zoecarver mentioned this pull request Jun 16, 2020
Merged
gribozavr
gribozavr reviewed Jun 16, 2020
View reviewed changes
@gribozavr
Copy link
Contributor

gribozavr commented Jun 16, 2020 •
edited
Loading

@gottesmm, could you review SILGen changes?

@gribozavr gribozavr requested a review from gottesmm June 16, 2020 11:12
@zoecarver zoecarver force-pushed the cxx/non-trival-with-destructor branch from 5c0cac2 to 3c9b1be Compare June 17, 2020 16:35
@zoecarver zoecarver requested a review from gribozavr June 17, 2020 16:36
gottesmm
gottesmm reviewed Jun 17, 2020
View reviewed changes
lib/SILGen/SILGenStmt.cpp Outdated
tupleInit->SubInitializations.push_back(std::move(eltInit));
}
// If this is an empty tuple, we don't want to decompose it.
if (resultTupleType.getElementTypes().size()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this need to be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

IOW, I don't understand why this change requires this other fix. Does a crash result? Please explain this to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Using any type with a user-provided destructor crashes without this fix.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I'm not sure this is the right fix. It did fix the problem and (seems like?) it doesn't have any unwanted side effects but, it doesn't fix the "root" of the issue. I can investigate this a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Maybe @jckarter or @slavapestov have ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jckarter the assertion on line 451: assert(allResults.empty());.

Because in prepareIndirectResultInit the tuple path is taken and the tuple is empty, the results aren't recursively sliced off allResults.

Copy link
Contributor Author

@zoecarver zoecarver Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW I suspect this is probably slower (it goes from essentially a no-op to a recursive function that will dynamically allocate memory).

Edit: scratch that; both paths will allocate something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zoecarver If the result type is an empty tuple, though, then there shouldn't be anything in allResults to slice off to begin with. Do you know what the root of the change in behavior is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jckarter sorry this comment slipped through my inbox somehow. I can spend some time today looking into this in more depth.

Do you know what the root of the change in behavior is?

I don't know.

If the result type is an empty tuple, though, then there shouldn't be anything in allResults to slice off to begin with.

I think allResults holds indirect results (also?).

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 was able to find and fix the root of the problem (the return statement was given an expression when it shouldn't have).

@gribozavr gribozavr requested a review from jckarter June 19, 2020 11:11
@zoecarver zoecarver force-pushed the cxx/non-trival-with-destructor branch 2 times, most recently from c7b7369 to 9b1d651 Compare June 24, 2020 19:19
zoecarver
zoecarver commented Jun 24, 2020
View reviewed changes
lib/ClangImporter/ImportDecl.cpp Outdated
result->setType(emptyTuple);

auto ret = new (ctx) ReturnStmt(SourceLoc(), result, /*Implicit=*/true);
auto ret = new (ctx) ReturnStmt(SourceLoc(), nullptr, /*Implicit=*/true);
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'm going to make separate PRs for this and the change to IRGen/GenBuiltin.cpp with tests.

This was referenced Jun 24, 2020
Closed
Merged
Closed
@zoecarver zoecarver force-pushed the cxx/non-trival-with-destructor branch from 9b1d651 to 094e325 Compare July 7, 2020 17:02
@zoecarver
Copy link
Contributor Author

@gribozavr friendly ping. This PR no longer has any SILGen changes.

gribozavr
gribozavr approved these changes Jul 9, 2020
View reviewed changes
Copy link
Contributor

@gribozavr gribozavr left a comment

Choose a reason for hiding this comment

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

LGTM with suggested changes.

test/Interop/Cxx/class/Inputs/type-classification.h Outdated
@@ -155,4 +155,12 @@ struct StructDeletedDestructor {
~StructDeletedDestructor() = delete;
};

struct HasDestructor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use StructWithDestructor instead?

test/Interop/Cxx/class/Inputs/type-classification.h Outdated
~HasDestructor() {}
};

struct HasMemberWithDestructor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use StructWithSubobjectDestructor instead?

@zoecarver
[cxx-interop] Mark types with a destructor a non-trivial.
13632d4 
Any C++ type that isn't trivially copyable is now also not a trivial type. This will preserve the destructor (among other things).
@zoecarver zoecarver force-pushed the cxx/non-trival-with-destructor branch from 094e325 to 13632d4 Compare July 10, 2020 15:51
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 8cf6e93 into swiftlang:master Jul 10, 2020
gribozavr
gribozavr reviewed Jul 17, 2020
View reviewed changes
test/Interop/Cxx/class/type-classification-non-trivial-silgen.swift
let d = StructWithDestructor()
}

// Make sure that "HasMemberWithDestructor" is marked as non-trivial by checking
Copy link
Contributor

Choose a reason for hiding this comment

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

HasMemberWithDestructor => StructWithSubobjectDestructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

@zoecarver ^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops forgot about this. #33150.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gottesmm gottesmm gottesmm left review comments

@gribozavr gribozavr gribozavr approved these changes

@jckarter jckarter Awaiting requested review from jckarter

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zoecarver @gribozavr @jckarter @gottesmm @swift-ci