Skip to content

Fix "Undefined symbol" linker error with default argument for inherited initializer #7171

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

Conversation

jrose-apple
Copy link
Contributor

@jrose-apple jrose-apple commented Feb 1, 2017

  • Explanation: When a subclass inherits initializers from a base class in another module, it wasn't correctly configuring parameters with default argument values. This led to callers expecting to find a symbol that doesn't exist. Additionally, trying to use the initializer in a generic context led to a crash in SILGen, so the PR also puts in a narrow fix for that, with SR-3808 filed to investigate more later.
  • Scope: Affects inherited initializers, plus a few other places where parameter lists are cloned.
  • Issue: rdar://problem/30167924
  • Reviewed by: @DougGregor, @slavapestov
  • Risk: Low. The intent is that this only changes code that would have led to either linker errors or compiler crashers in 3.0.
  • Testing: Added compiler regression tests.

@jrose-apple jrose-apple added this to the Swift 3.1 milestone Feb 1, 2017
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Feb 1, 2017

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 89bfedb53cccf601127b5e050bffa09a97fed10d
Test requested by - @jrose-apple

@swift-ci
Copy link
Contributor

swift-ci commented Feb 1, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 89bfedb53cccf601127b5e050bffa09a97fed10d
Test requested by - @jrose-apple

@jrose-apple
Copy link
Contributor Author

Ah, I forgot to use the old mangling again.

…wiftlang#7169)

In cases where a default value is used for a parameter with generic
type, the argument list might be empty. In that case, we don't need
to emit any arguments!
…wiftlang#7156)

It was checking the wrong predicate, and therefore failing to mark
inherited default arguments as actually being inherited.

While here, explicitly clear out default arguments from non-inherited
cloned parameter lists. I don't think this case can come up today, but
it's better to be correct when we do hit it.

rdar://problem/30167924
@jrose-apple jrose-apple force-pushed the 3.1-inherited-default-arguments branch from 89bfedb to 822c979 Compare February 1, 2017 02:22
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Feb 1, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 89bfedb53cccf601127b5e050bffa09a97fed10d
Test requested by - @jrose-apple

@swift-ci
Copy link
Contributor

swift-ci commented Feb 1, 2017

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 89bfedb53cccf601127b5e050bffa09a97fed10d
Test requested by - @jrose-apple

@jrose-apple
Copy link
Contributor Author

/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.3.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVOpenGLESTexture.h:23:2: error: malformed or corrupted AST file: 'SourceLocation remap refers to unknown module'

(https://ci.swift.org/job/swift-PR-osx/5339/consoleFull#-1445105749ee1a197b-acac-4b17-83cf-a53b95139a76)

@bcardosolopes, look familiar?

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test macOS

@tkremenek tkremenek merged commit 6b19f27 into swiftlang:swift-3.1-branch Feb 1, 2017
@bcardosolopes
Copy link
Contributor

@jrose-apple, never seen this one - looks like some inconsistency in ModuleMgr. Is this reproducible? any chance to give it a try with a clean cache?

@jrose-apple jrose-apple deleted the 3.1-inherited-default-arguments branch February 1, 2017 17:56
@jrose-apple
Copy link
Contributor Author

This is the first time I've seen it, and the retest didn't reproduce it. Guess we'll have to leave it for now. Sorry!

@bcardosolopes
Copy link
Contributor

@jrose-apple np, thanks for pointing out though!

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