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

Mangling: enable new mangling for symbols #8126

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

eeckstein
Copy link
Contributor

@eeckstein eeckstein commented Mar 15, 2017

Finally
rdar://problem/24085509

@eeckstein
Copy link
Contributor Author

@swift-ci Please test and merge

@eeckstein
Copy link
Contributor Author

@swift-ci Please clean test linux

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 89bb2d169a3d285064d97957537b762c6eab7965
Test requested by - @eeckstein

@@ -32,6 +32,7 @@ llvm::cl::opt<bool> NewManglingForTests(

#ifndef USE_NEW_MANGLING

#ifndef USE_NEW_MANGLING
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this #ifndef is a duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll remove it

@eeckstein
Copy link
Contributor Author

This PR must be in sync with swiftlang/swift-corelibs-foundation#919

@eeckstein
Copy link
Contributor Author

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 89bb2d169a3d285064d97957537b762c6eab7965
Test requested by - @eeckstein

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 89bb2d169a3d285064d97957537b762c6eab7965
Test requested by - @eeckstein

@eeckstein
Copy link
Contributor Author

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 475634645da1decd55a4a8312127566f54ae11b3
Test requested by - @eeckstein

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 475634645da1decd55a4a8312127566f54ae11b3
Test requested by - @eeckstein

@eeckstein
Copy link
Contributor Author

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 0f4a5c5eb8cb3511001238a7d43b79a894d6eb97
Test requested by - @eeckstein

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 0f4a5c5eb8cb3511001238a7d43b79a894d6eb97
Test requested by - @eeckstein

@eeckstein
Copy link
Contributor Author

@swift-ci Please clean test linux

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 2a55b26
Test requested by - @eeckstein

@eeckstein
Copy link
Contributor Author

swiftlang/swift-corelibs-foundation#919

@swift-ci Please clean test linux

@eeckstein eeckstein merged commit e78445b into swiftlang:master Mar 16, 2017
@jrose-apple
Copy link
Contributor

Triple-checking: does this break serialized archives that have old mangled names in them, which will be looked up at runtime on both Apple (ObjC) and non-Apple platforms?

@eeckstein
Copy link
Contributor Author

@jrose-apple Not this change. It affects only symbol mangling. AFAIK, archives use the type names stored at runtime.
I changed the runtime type mangling a while ago (but only for non-top-level or generic classes).

@eeckstein eeckstein deleted the newmangling branch March 16, 2017 22:05
@jrose-apple
Copy link
Contributor

On Linux we do something else and I can't remember what it is and now I'm worried. :-) @jckarter, @dgrove-oss, @phausler? Is this something we'll have to worry about?

@phausler
Copy link
Contributor

This breaks darwin archives but only in a specific case they encode the NSStringFromClass which emits the mangled name in the cases of sub types IIRC.

import Foundation

class Foo { class Bar { } }

// emulate what NSKeyedArchiver does to store the class name
print(NSStringFromClass(Foo.self))
print(NSStringFromClass(Foo.Bar.self))

@phausler
Copy link
Contributor

do we have a work-around to support archives that use the old names?

@eeckstein
Copy link
Contributor Author

@phausler You are right, only the names of classes changed if the class is
*) not-top level (as in your example) or
*) generic or
*) private

do we have a work-around to support archives that use the old names?

No

@phausler
Copy link
Contributor

So this will result in external developers having applications that will no longer be able to open archives from previous versions of swift. That is a data loss case. Marking a class private is totally reasonable for developers to do. Changing the type such that it is serialized to disk as something different in the archive will cause users to not be able to open documents they previously saved in older versions of apps.

@eeckstein
Copy link
Contributor Author

Unfortunately in this respect, the archive format is ABI dependent and as we didn't freeze the ABI yet, there will be a breakage.
What we could do is to implement some kind of name translation as a workaround. This would be not so difficult for non-top level and private classes, and a bit of effort for generic classes.
This should be done where the class name lookup is done when reading an archive. Although I don't know where this is.

@eeckstein
Copy link
Contributor Author

As a side note, the name for private classes depend on the module name and the source file name where the class is defined. So if the developer changes any of those, it will also break the archive.

@jrose-apple
Copy link
Contributor

Right, we've talked about this before. Private classes are currently out of scope unless the user specifies a custom objc name or uses the NSKeyedArchiver callbacks to control the name. Generic classes have to use the callbacks because they may not exist yet.

@eeckstein
Copy link
Contributor Author

So the only remaining type of classes is non-top level classes. Can we just require to use the callback for those classes as a workaround?
Alternatively we could implement a old->new name translation function in the swift stdlib (It would be implemented by demangling with the old demangler and creating the new name with the new re-mangler.) This function could be called by the archiver to automatically translate names.
What do you think?

@jrose-apple
Copy link
Contributor

I don't think users should ever have to think about mangling. I'd rather have the old -> new translation as a fallback path.

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.

5 participants