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

Jump to bottom

[SR-1502] Enabling whole-module optimization causes faulty control flow #44111

Closed
swift-ci opened this issue May 12, 2016 · 11 comments
Closed

[SR-1502] Enabling whole-module optimization causes faulty control flow #44111

swift-ci opened this issue May 12, 2016 · 11 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself optimized only Flag: An issue whose reproduction requires optimized compilation

Comments

@swift-ci
Copy link
Collaborator

Previous ID SR-1502
Radar rdar://problem/26255167
Original Reporter andymatuschak (JIRA User)
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug, OptimizedOnly
Assignee andymatuschak (JIRA)
Priority Medium

md5: e1c2fd46de0482f4efa7bca7f3c4ff2c

Issue Description:

We recently tried turning on whole-module optimization on the Khan Academy project. Three tests newly started failing, apparently due to faulty switch pattern matching. Possibly related to [SR-1217]?

I tried and failed to create a reduced test case, but the following patch resolved the issue for us—perhaps that will be a clue?

diff --git a/KHATests/URLRoutingTest.swift b/KHATests/URLRoutingTest.swift
index 40702e5..6c6baae 100644
--- a/KHATests/URLRoutingTest.swift
+++ b/KHATests/URLRoutingTest.swift
@@ -83,7 +83,8 @@ class URLRoutingTest: XCTestCase {
                if let target = URLTarget(url: testCase.inputURL) {
                        switch target {
                        case .ContentItem: return true
-                       case .TopicSlug, .Domain: return false
+                       case .TopicSlug: return false
+                       case .Domain: return false
                        }
                } else {
                        return false
@@ -105,7 +106,8 @@ class URLRoutingTest: XCTestCase {
                if let target = URLTarget(url: testCase.inputURL) {
                        switch target {
                        case .Domain: return true
-                       case .TopicSlug, .ContentItem: return false
+                       case .TopicSlug: return false
+                       case .ContentItem: return false
                        }
                } else {
                        return false

If there's other useful diagnostic information I can generate or supply, please do let me know. We recently shared the KA source with @akyrtzi, so if an Apple-internal engineer takes a look at this, you can reproduce by running the unit tests with whole-module optimization enabled. -O alone does not produce the issue.
@rudkx
Copy link
Member

rudkx commented May 13, 2016

Thanks for the report.

I'll take a look!

@rudkx
Copy link
Member

rudkx commented May 13, 2016

andymatuschak (JIRA User), what version of Xcode were you using when you saw this behavior?

I ask because I am actually seeing a different bug that is blocking this from compiling when whole module optimization is enabled.

Feel free to get in touch with me at <first> dot <last> at apple.com.

@NachoSoto
Copy link
Contributor

It reproduces in both 7.3 and 7.3.1. Forgot to mention that I had to make a few workarounds to get to compile (like nested types in extensions falling to link and something regarding Obj-C initializers without explicit nullability).

I can send you guys an updated project and the diff with the workarounds.

As a side note: I'm glad we're running our (limited) test suite in release mode. I wonder how many people are affected by this miss-compilation and they don't know.

@rudkx
Copy link
Member

rudkx commented May 13, 2016

Yes, I was hitting the Obj-C initializer issue, and after commenting out the two places I hit that, I am now seeing a linking issue in an extension.

If you can get me a new project that works around these and demonstrates the control-flow issue, I can take a look at that and also try to get some traction on the initializer and linkage issues as I would not expect to see a difference here with whole module optimization enabled. If you want to file separate bugs for those issues to track progress and so that you can get notification when a fix goes in, please feel free.

@NachoSoto
Copy link
Contributor

I just e-mailed you the updated project and the patch with all the work arounds. Thanks for your time! 🙂

@rudkx
Copy link
Member

rudkx commented May 20, 2016

This turned out to be a bug in the LLVM MergeFunctions optimization pass, and that bug is still present in the LLVM repository.

I am preparing a patch to submit.

@rudkx
Copy link
Member

rudkx commented May 20, 2016

Up for review: http://reviews.llvm.org/D20462

@NachoSoto
Copy link
Contributor

That's huge, thanks Mark!

@rudkx
Copy link
Member

rudkx commented May 20, 2016

Fixed in llvm.org trunk in r270250 and swift.org/swift-llvm swift-3.0-branch in 26ee5ef61d62b555f81f3a7373dc2479ede9ba70.

@jckarter
Copy link
Member

jckarter commented Jun 3, 2016

andymatuschak (JIRA User) or @NachoSoto, when/if you make the jump to Swift 3, would you be able to verify this fix (if you haven't already)?

@NachoSoto
Copy link
Contributor

We haven't yet, but we'll do soon 🙂

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself optimized only Flag: An issue whose reproduction requires optimized compilation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jckarter @rudkx @NachoSoto @swift-ci