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

[SR-1813] Fix archetype access path binding #3093

Merged
merged 1 commit into from Jun 29, 2016

Conversation

Projects
None yet
6 participants
@CodaFi
Copy link
Collaborator

commented Jun 20, 2016

What's in this pull request?

Fixes a regression from Swift 2.2 where the configuration reported in SR-1813 would cause the compiler to forget to setup archetype access paths which would blow up witness table gen.

Resolved bug number: (SR-1813)


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@jrose-apple

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

Probably best reviewed by @slavapestov or @rjmccall.

@CodaFi CodaFi force-pushed the CodaFi:witness-protection-program branch Jun 21, 2016

@rjmccall

View changes

lib/IRGen/GenProto.cpp Outdated
void EmitPolymorphicParameters::bindArchetypeAccessPaths() {
if (!Generics) return;
static void bindArchetypeAccessPaths(IRGenFunction &IGF,
ArrayRef<Requirement> Generics,

This comment has been minimized.

Copy link
@rjmccall

rjmccall Jun 23, 2016

Member

Please take a GenericSignature instead of an array of Requirements.

@rjmccall

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

Generally looks okay, but:

  • The diff is strange; I'm really not sure why github's diff is removing and re-adding EmitPolymorphicParameters::emit. It might just be poor tooling, but it's odd enough to make me wonder whether you've accidentally changed line terminators or something.
  • Try to always pass around generic signatures as GenericSignatures rather than an independent collection of requirements.
@CodaFi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 23, 2016

The diff is strange;

I did have to shuffle a non-trivial amount of code around.

@CodaFi CodaFi force-pushed the CodaFi:witness-protection-program branch Jun 23, 2016

@rjmccall

This comment has been minimized.

Copy link
Member

commented Jun 24, 2016

Yeah, maybe it's just that the function is small.

@rjmccall

This comment has been minimized.

Copy link
Member

commented Jun 24, 2016

@swift-ci Please test.

Fix archetype access path binding
When considering a type that declares an archetype bound to an
associated type, we were not generating the access paths to any
underlying conformance data and so were not properly able to grab the
witness table for those conformances resulting in a crash.  This way,
all bound requirements also make sure to bind archetype access paths so
we can see actually see the conformances.

@CodaFi CodaFi force-pushed the CodaFi:witness-protection-program branch to 5492690 Jun 25, 2016

@CodaFi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2016

@rjmccall Mind giving it another go?

@rjmccall

This comment has been minimized.

Copy link
Member

commented Jun 27, 2016

LGTM.

@harlanhaskins

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2016

@swift-ci please test

@CodaFi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2016

@rjmccall ping

@harlanhaskins

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2016

@rjmccall please ping

@banjun

This comment has been minimized.

Copy link

commented Jun 29, 2016

Thank you @CodaFi 👍
I'm reporter of the original issue and have confirmed that your commit resolve the issue.

However, I found another crash on this commit but I cannot tell whether this is same issue or not. (around emitArchetypeWitnessTableRef).
Should I submit another report?

The new case is below:

protocol P1 {
    associatedtype A1: Equatable
}

protocol P2 {
    associatedtype A2: Equatable
}

class C1<T, U: P1 where U.A1 == T>: P2 {
    typealias A2 = T
}

will result in:

no relation found that declares conformance to target

6  swift                    0x0000000110b8da27 llvm::llvm_unreachable_internal(char const*, char const*, unsigned int) + 471
7  swift                    0x000000010df9ff00 llvm::Value* llvm::function_ref<llvm::Value* (unsigned int)>::callback_fn<swift::irgen::emitArchetypeWitnessTableRef(swift::irgen::IRGenFunction&, swift::CanTypeWrapper<swift::ArchetypeType>, swift::ProtocolDecl*)::$_0>(long, unsigned int) + 368
8  swift                    0x000000010e06e921 swift::irgen::emitImpliedWitnessTableRef(swift::irgen::IRGenFunction&, llvm::ArrayRef<swift::irgen::ProtocolEntry>, swift::ProtocolDecl*, llvm::function_ref<llvm::Value* (unsigned int)> const&) + 273
9  swift                    0x000000010df9d8c4 swift::irgen::emitArchetypeWitnessTableRef(swift::irgen::IRGenFunction&, swift::CanTypeWrapper<swift::ArchetypeType>, swift::ProtocolDecl*) + 164

Moreover, removing 'protocol P2' and using 'P1' instead of 'P2' still reproduce same error message and make the case shortest example. (I think using P1 and P2 is more general case).

@CodaFi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 29, 2016

Yes, please submit another SR to bugs.swift.org. This should have shown up as a diagnostic and not survived to SILGen.

@rjmccall rjmccall merged commit 2141eac into apple:master Jun 29, 2016

2 checks passed

Swift Test Linux Platform Build finished.
Details
Swift Test OS X Platform Build finished.
Details

@CodaFi CodaFi deleted the CodaFi:witness-protection-program branch Jun 29, 2016

@banjun

This comment has been minimized.

Copy link

commented Jun 30, 2016

I submitted the other case to SR-1939. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.