Skip to content

Adapt for the mangling prefix change ($S -> $s) #1699

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 1 commit into from
Sep 20, 2018
Merged

Adapt for the mangling prefix change ($S -> $s) #1699

merged 1 commit into from
Sep 20, 2018

Conversation

eeckstein
Copy link
Contributor

No description provided.

@eeckstein eeckstein mentioned this pull request Sep 19, 2018
Merged
@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci smoke test Linux platform

@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci test Linux platform

@parkera
Copy link
Contributor

parkera commented Sep 19, 2018

Can you test this manually on macOS to make sure we don't break our developers there? The CI doesn't do it.

@parkera
Copy link
Contributor

parkera commented Sep 19, 2018

Last time this mangling changed it really messed us up for a while since everyone had to have ToT tools installed.

@eeckstein
Copy link
Contributor Author

@parkera What test should I run?

@parkera
Copy link
Contributor

parkera commented Sep 19, 2018

You can run all the unit tests (I hope) on macOS using the Xcode workspace.

@parkera
Copy link
Contributor

parkera commented Sep 19, 2018

I assume in order for this to work you need to install a toolchain based on your compiler PR?

@eeckstein
Copy link
Contributor Author

yeah, this change has to be in sync with the compiler mangling change

@eeckstein
Copy link
Contributor Author

OK, I did run the tests locally by setting SWIFT_EXEC and SWIFT_LIBRARY_PATH to my ToT compiler build.
I worked around one compiler error and got 12 failures in TestNSNumberBridging, which seem to be unrelated to the mangling change.

How can we proceed with that?

@spevans
Copy link
Contributor

spevans commented Sep 19, 2018

@eeckstein The compiler error (related to NSError) and the 12 test failures are known issues so it sounds like the tests worked ok.

@eeckstein
Copy link
Contributor Author

ok thanks. Are you ok with this change (which means you guys have to work with a ToT compiler/toolchain for a while)?

@parkera
Copy link
Contributor

parkera commented Sep 19, 2018

I know we need to do this, but is there any way we can detect which version of the compiler we're building with to continue to support shipped tools?

Not to overstate it, but it introduces a barrier to contributions which is difficult for some people to get past.

@eeckstein
Copy link
Contributor Author

eeckstein commented Sep 19, 2018 •
edited
Loading

What you can do is to add a pre-build configuration step. For example:

echo '#define __CFConstantStringClassReference' `swift-demangle --remangle-new '$S15SwiftFoundation19_NSCFConstantStringCN'` > config.h

using the swift-demangle from the toolchain. And then include config.h in CFInternal.h

Or maybe you can just define a build setting outside the Xcode project and then conditionally compile the macro definition in CFInternal.h. A little bit more manual, but simpler.

parkera
parkera requested changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

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

Marking as requesting changes while we figure out how to preserve some manner of compatibility for our contributors while still making forward progress on mangling change.

@eeckstein
Copy link
Contributor Author

@parkera I added a workaround which enables to compile with both compiler versions (old and new mangling prefix)

parkera
parkera approved these changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

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

Great!

@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci test Linux platform

1 similar comment
@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci test Linux platform

@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci clean test Linux platform

2 similar comments
@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci clean test Linux platform

@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci clean test Linux platform

@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci test Linux platform

1 similar comment
@eeckstein
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#19388

@swift-ci test Linux platform

@eeckstein eeckstein merged commit 59df77d into swiftlang:master Sep 20, 2018
@eeckstein eeckstein deleted the mangling-prefix branch September 20, 2018 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eeckstein @parkera @spevans