Skip to content

Resilient witness table emission cleanup #22775

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 4 commits into from
Feb 23, 2019
Merged

Resilient witness table emission cleanup #22775

merged 4 commits into from
Feb 23, 2019

Conversation

slavapestov
Copy link
Contributor

I found some of the code in GenProto.cpp hard to follow while working on #22737. Now that the immediate bug fix is done here is a larger cleanup. Should be mostly NFC.

@slavapestov
IRGen: Don't emit generic witness table if conformance doesn't need one
22259a6 
The condition for "needs a generic witness table" was wrong; any
conformance with associated conformances would get one, even though
its only necessary if the associated conformances are dependent.

Fixing this also allows simplifying some code.
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

slavapestov
slavapestov commented Feb 21, 2019
View reviewed changes
test/IRGen/protocol_conformance_records.swift
@@ -136,7 +157,7 @@ extension Int : OtherResilientProtocol { }
// -- witness table pattern
// CHECK-SAME: @"$s28protocol_conformance_records9DependentVyxGAA9AssociateAAWp"
// -- flags
// CHECK-SAME: i32 0
// CHECK-SAME: i32 131072,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is not a change in test output -- the test was matching the wrong "i32 0"

@slavapestov
IRGen: Remove ConformanceDescriptor::hasDependentAssociatedTypeWitnesses
d20ca1f 
This sets the 'requires instantiation' bit in a couple more cases, which
doesn't really matter because the runtime already checks a few other
conditions, such as the presence of resilient witnesses.
@slavapestov
IRGen: Remove considerResilience parameter to isDependentConformance()
bdac8ad
@slavapestov
Runtime: Clarify meaning of a flag bit in GenericWitnessTable
6ebd992
@slavapestov slavapestov force-pushed the resilient-witness-table-cleanup branch from 65dac58 to 6ebd992 Compare February 21, 2019 05:02
@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 65dac5855af3b7ecf65ebf20331215b6b7d370bc

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 65dac5855af3b7ecf65ebf20331215b6b7d370bc

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

apple/swift-lldb#1314
@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6ebd992

@slavapestov
Copy link
Contributor Author

This PR is cursed. Now I'm seeing

/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/swift-corelibs-libdispatch/src/swift/Dispatch.swift:13:19: error: no such module 'Dispatch'
00:38:50.035 @_exported import Dispatch
00:38:50.035                   ^

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test Linux

@shahmishal
Copy link
Member

cc/ @millenomi have you seen this issue?

@shahmishal
Copy link
Member

@millenomi Sorry, this is dispatch issue.

@shahmishal
Copy link
Member

/cc @ktopley-apple have you seen this issue before?

@ktopley-apple
Copy link
Contributor

/cc @ktopley-apple have you seen this issue before?

No. Which build was this? I looked at #11255 and I did not see any dispatch errors.

@slavapestov
Copy link
Contributor Author

Now we're failing with:

01:25:45.115 /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift-corelibs-foundation/TestFoundation/TestURLSession.swift:170: error: TestURLSession.test_dataTaskWithHttpInputStream : XCTAssertEqual failed: ("1316 bytes") is not equal to ("1323 bytes") - Response Data and Data is not equal
01:25:45.115 Test Case 'TestURLSession.test_dataTaskWithHttpInputStream' failed (0.003 seconds)

@shahmishal Can we disable Foundation tests in PR testing for now?

@shahmishal
Copy link
Member

Yes, we should disable this test. cc: @millenomi

@slavapestov
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#1939
@swift-ci Please smoke test Linux

@slavapestov slavapestov merged commit 88bc29a into swiftlang:master Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@slavapestov @swift-ci @shahmishal @ktopley-apple