Skip to content
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.

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

[Serialization] Force destructor creation for imported class for SIB deserialization #33255

Merged
merged 1 commit into from Aug 26, 2020

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Aug 3, 2020

Swift class deinit decl can be implicitly synthesized when emitting swiftmodule, so swiftmodule always have deinit decl. So resolution of x-refs to deinit of swift class always success.
But when x-refs points deinit of clang imported class, it always failed because clang importer doesn't force to synthesize deinit before looking up.

x-refs to deinit decl appears in only deinit of its subclasses, so it's serialized only when deinit have body. And deinit has body only on SIB because deinit is always non-inlinable.

It means that this missing of deinit creation can be problem only on SIB.

This commit changes to force to synthesize class deinit decl before looking up members.

@kateinoigakukun kateinoigakukun changed the title [Serialization] Force destructor creation for imported class [Serialization] Force destructor creation for imported class for SIB deserialization Aug 3, 2020
@kateinoigakukun kateinoigakukun force-pushed the katei/fix-sib-xref-deinit branch 3 times, most recently from a8e8bac to ff0d742 Compare August 3, 2020 12:07
@MaxDesiatov
Copy link
Member

Is this ready for review and CI runs?

@kateinoigakukun
Copy link
Member Author

@MaxDesiatov Yes 👍

@MaxDesiatov
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Collaborator

swift-ci commented Aug 3, 2020

Build failed
Swift Test OS X Platform
Git Sha - ff0d742ffcc0f6d723055497d4591b2c6344ad8c

@swift-ci
Copy link
Collaborator

swift-ci commented Aug 3, 2020

Build failed
Swift Test Linux Platform
Git Sha - ff0d742ffcc0f6d723055497d4591b2c6344ad8c

@MaxDesiatov
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Collaborator

swift-ci commented Aug 4, 2020

Build failed
Swift Test Linux Platform
Git Sha - ff0d742ffcc0f6d723055497d4591b2c6344ad8c

@swift-ci
Copy link
Collaborator

swift-ci commented Aug 4, 2020

Build failed
Swift Test OS X Platform
Git Sha - ff0d742ffcc0f6d723055497d4591b2c6344ad8c

@MaxDesiatov
Copy link
Member

@swift-ci please test Windows platform

@kateinoigakukun
Copy link
Member Author

@gottesmm @brentdax Could you take a look at this change?

@kateinoigakukun
Copy link
Member Author

@slavapestov Would you mind to take a look at this change?

@kateinoigakukun
Copy link
Member Author

@gottesmm @brentdax @slavapestov This change will unblock another lto work, so it will make the project move forward if this PR move forward. 🙏

assert(isa<ClassDecl>(nominal));
// Force creation of an implicit destructor
auto CD = dyn_cast<ClassDecl>(nominal);
(void) CD->getDestructor();
Copy link
Member

Choose a reason for hiding this comment

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

How about just adding the destructor to the values vector and skipping the second lookup below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I updated.

Swift class deinit decl can be implicitly synthesized when emitting
swiftmodule, so swiftmodule always have deinit decl. So resolution of
x-refs to deinit of swift class always success.
But when x-refs points deinit of clang imported class, it always failed
because clang importer doesn't force to synthesize deinit before looking
up.

x-refs to deinit decl appears in only deinit of its subclasses, so it's
serialized only when deinit have body. And deinit has body only on SIB
because deinit is always non-inlinable. It means that this missing of
deinit creation can be problem only on SIB

This commit changes to force to synthesize class deinit
decl before looking up members.
@MaxDesiatov
Copy link
Member

@swift-ci please test

@MaxDesiatov
Copy link
Member

@swift-ci please smoke test

@beccadax beccadax requested a review from CodaFi August 14, 2020 09:43
@MaxDesiatov
Copy link
Member

@swift-ci smoke test

@MaxDesiatov
Copy link
Member

@swift-ci please smoke test

@kateinoigakukun
Copy link
Member Author

@slavapestov @DougGregor Thanks for your reviews! Would you mind merging this patch?

@MaxDesiatov MaxDesiatov merged commit e387b36 into apple:master Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants