-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Replace the --defsym linker argument with an alias in code #1011
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
@swift-ci please smoke test |
1 similar comment
@swift-ci please smoke test |
@swift-ci please test |
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Ive made the alias conditional on it not being on macOS which worked when I built it on xcode and linux |
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 |
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.
c5b5574
to
13aa8ec
Compare
@phausler I redid the PR to use the #define for all targets as you suggested and removed the entry from the @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 |
@phausler Do these changes look ok now? I merged the linux and android |
Yea, ideally we will want to migrate to something similar to @_cdecl in the future but for now I think this is fine |
@swift-ci please test |
@phausler I think the test worked but didn't update GitHub. Can this be merged now? |
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