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

Replace the --defsym linker argument with an alias in code #1011

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

spevans
Copy link
Contributor

@spevans spevans commented May 26, 2017

  • This has the same effect as the
  • defsym=__CFConstantStringClassReference=_T010Foundation19_NSCFConstantStringCN
    option but does it in code which allows simplifying the linker arguments.

This will help with the -static-stdlib option since less linker arguments will need to be provided by swiftpm

@aciidgh
Copy link

aciidgh commented May 26, 2017

@swift-ci please smoke test

1 similar comment
@aciidgh
Copy link

aciidgh commented May 26, 2017

@swift-ci please smoke test

@pushkarnk
Copy link

@swift-ci please test

Copy link
Contributor

@phausler phausler left a comment

Choose a reason for hiding this comment

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

Please make sure this works on Xcode based builds on macOS since that is a decent swath of the development environment for this

@@ -349,7 +349,7 @@ CF_EXPORT void * __CFConstantStringClassReferencePtr;

#if DEPLOYMENT_RUNTIME_SWIFT

CF_EXPORT void *__CFConstantStringClassReference[];
CF_EXPORT void *__CFConstantStringClassReference[] asm("_T010Foundation19_NSCFConstantStringCN");
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work on Darwin builds since the module name is SwiftFoundation and not Foundation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cant currently build foundation on Xcode due to lots of other unrelated errors but given that it seems to be defined in CoreFoundation/Base.subproj/SymbolAliases and there looks to be a fix in #1006 which I assume if for macOS do I just need to make this change for non macOS? something like:

#if TARGET_OS_MAC
CF_EXPORT void *__CFConstantStringClassReference[];
#else
CF_EXPORT void *__CFConstantStringClassReference[] asm("_T010Foundation19_NSCFConstantStringCN");
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we had some way to avoid this disparity between the two when the mangling changes. Which seems to be a common occurrence... we had the same problem with swift 3.

Is this mangled name based off of the swift-4.0-branch names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this mangled name seems to be due to the metadata changes in march.
For Xcode, it looks like the change in #1006 is required

@spevans
Copy link
Contributor Author

spevans commented May 27, 2017

Ive made the alias conditional on it not being on macOS which worked when I built it on xcode and linux

@phausler
Copy link
Contributor

There is one other symbol to watch out for: __CFSwiftGetBaseClass

As it stands there is actually a android specific define for __CFConstantStringClassReference in CFInternal.h; perhaps we can elide that if check and convert it to something like this:

#if DEPLOYMENT_RUNTIME_SWIFT
#if DEPLOYMENT_TARGET_LINUX
#define __CFConstantStringClassReference _T010Foundation19_NSCFConstantStringCN
#else
#define __CFConstantStringClassReference _T015SwiftFoundation19_NSCFConstantStringCN
#endif
#endif

Since CF is only used in Foundation via swift we should be ok by defining the compiler reference as listed and wholesale remove the symbolic aliases file definitions.

SwiftPM should not need to access the __CFConstantStringClassReference since no CF code should ever be used outside of Foundation on linux (in the realms of swift projects)

Moreover it might be nice to have them as defined values since we could perhaps somehow have a define that matches the toolchain emission (as a future enhancement) to prevent this breaking if the mangling changes ever again.

#ifndef __CFSwiftGetBaseClass
#if TARGET_OS_LINUX
#define __CFSwiftGetBaseClass _T010Foundation21__CFSwiftGetBaseClasss9AnyObject_pXpyF
#elif TARGET_OS_MAC
#define __CFSwiftGetBaseClass _T015SwiftFoundation21__CFSwiftGetBaseClasss9AnyObject_pXpyF
#endif
#endif

@spevans
Copy link
Contributor Author

spevans commented May 29, 2017

I dont really want to do too many changes in this PR especially since I cant test the android one, Id rather do a separate one for that.

Is the CoreFoundation//Base.subproj/SymbolAliases only used for the one __CFConstantStringClassReference alias and the rest are ignored? Also, what is the difference between that file and DarwinSymbolAliases, they currently have the same contents.

- This has the same effect as the --defsym=__CFConstantStringClassReference=_T010Foundation19_NSCFConstantStringCN
  option but does it in code which allows simplifying the linker arguments.

- Unify the Linux and Android #define and add a seperate one for macOS.

- Remove the entry from CoreFoundation/Base.subproj/SymbolAliases now
  that it is set in code.
@spevans
Copy link
Contributor Author

spevans commented Jun 13, 2017

@phausler I redid the PR to use the #define for all targets as you suggested and removed the entry from the SymbolAliases file. I couldnt find a way of extracting the mangled name since libFoundation cant be built until CoreFoundation is compiled. Perhaps there needs to be a swift mangle command added in the future.

@modocache Do you still run the CI for Android? If so could you check that my change hasnt broken it on Android - I dont think it has since its still the same #define just in a different #if

I tested it on both Linux and Xcode and didnt see any issues

@spevans
Copy link
Contributor Author

spevans commented Jun 17, 2017

@phausler Do these changes look ok now? I merged the linux and android #define

@phausler
Copy link
Contributor

Yea, ideally we will want to migrate to something similar to @_cdecl in the future but for now I think this is fine

@phausler
Copy link
Contributor

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Jun 20, 2017

@phausler I think the test worked but didn't update GitHub. Can this be merged now?

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