Skip to content

Fix crash when optimizing protocol composition #28012

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

Merged
merged 2 commits into from
Nov 2, 2019

Conversation

zoecarver
Copy link
Contributor

@zoecarver zoecarver commented Nov 1, 2019

This patch fixes a compiler crash when trying to optimize a protocol composition between a class and protocol. This is a super simple fix where I simply add a condition to check if the nominal arg type is null. Here's an example program:

class ClassA<T> { }
protocol ProtocolA { }

class MainClass<H> {
    init(x: ClassA<H> & ProtocolA) { }
}

final class ClassB: ClassA<String> { }
extension ClassB: ProtocolA { }

_ = MainClass(x: ClassB())

Resolves #54035

Once this has been reviewed, can someone with commit access trigger swift-ci, please?

// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors

// RUN: not %target-swift-frontend %s -O -whole-module-optimization
Copy link
Contributor

Choose a reason for hiding this comment

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

Does WMO make a difference for this test?

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, it only crashes with WMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Crashes for me with just -O as well. Also, I think the above invocation is not correct - it should be not %target-swift-frontend -O -whole-module-optimization %s -emit-sil.

@@ -904,7 +904,7 @@ SILCombiner::buildConcreteOpenedExistentialInfoFromSoleConformingType(
(archetypeTy->getConformsTo().size() == 1)) {
PD = archetypeTy->getConformsTo()[0];
} else if (ArgType.isExistentialType() && !ArgType.isAnyObject() &&
!SwiftArgType->isAny()) {
!SwiftArgType->isAny() && SwiftArgType->getAnyNominal()) {
PD = dyn_cast<ProtocolDecl>(SwiftArgType->getAnyNominal());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent to dyn_cast_or_null on the next line. But it sounds like this code needs to handle protocol compositions in a more principled way. It seems like there’s a missing case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What missing case is that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure @atrick is more familiar with this code.

@CodaFi CodaFi requested a review from atrick November 1, 2019 18:02
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

LGTM. @CodaFi is right, dyn_cast_or_null is cleaner.

There are various scenarios related to protocol composition that we should be able to optimize. This optimization is not really equipped to handle them though so it's best to just bail here.

I filed a bug for these issues:
SR-11695: SILOptimizer should devirtualize calls to a protocol composition type

@atrick
Copy link
Contributor

atrick commented Nov 1, 2019

@swift-ci test

@atrick
Copy link
Contributor

atrick commented Nov 2, 2019

Review comments addressed in #28032

@zoecarver
Copy link
Contributor Author

@atrick, thank you for the review and for fixing the review comments!

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.

4 participants