[FreeBSD] Fix a few issues #12528
[FreeBSD] Fix a few issues #12528
Conversation
|
@swift-ci please test |
|
The FreeBSD bits look good to me but please wait for explicit approval by somebody on the swift team before merging. |
|
Build failed |
|
@antonmes your code is failing on Linux, PTAL. |
|
@dcci I think it's not related to the PR, something with swift-corelibs-foundation in regression tests. Swift itself passes all the tests. This additions shouldn't affect any Darwin or Linux build instructions. |
| @@ -72,9 +72,14 @@ static SectionInfo getSectionInfo(const char *imageName, | |||
| SectionInfo sectionInfo = { 0, nullptr }; | |||
| void *handle = dlopen(imageName, RTLD_LAZY | RTLD_NOLOAD); | |||
| if (!handle) { | |||
| #ifdef __ANDROID__ | |||
| #if defined(__ANDROID__) || defined(__FreeBSD__) | |||
| #if defined(__ANDROID__) | |||
There was a problem hiding this comment.
Not extremely happy about this macro dance but it seems consistent with what's there (and probably OK).
There was a problem hiding this comment.
Why is this necessary on FreeBSD? It is necessary on Android because the OS hides the contents of /system/lib from dlopen().
There was a problem hiding this comment.
I'm not sure about the reasons but this part is called and fails with /libexec/ld-elf.so.1 and then with the main executable path. As for the latter, from #10836 (comment) I assumed that dl_iterate_phdr has the same behavior on FreeBSD.
cmake/modules/AddSwift.cmake
Outdated
| @@ -1528,7 +1528,10 @@ function(add_swift_library name) | |||
| "${SWIFT_${sdk}_ICU_UC}") | |||
| # temporary fix for atomic needing to be | |||
| # after object files for libswiftCore.so | |||
| if("${sdk}" STREQUAL "ANDROID") | |||
| if("${sdk}" STREQUAL "FREEBSD") | |||
| list(APPEND swiftlib_private_link_libraries_targets | |||
There was a problem hiding this comment.
Can you please add a comment here explaining why we need this?
There was a problem hiding this comment.
I thought the existing comment is enough, it's the same issue but we use builtins instead of -latomic. I don't know how to explain this or the reason. 😊
There was a problem hiding this comment.
You should update the comment, then, to mention builtins in addition to atomic.
There was a problem hiding this comment.
I mean it imports __atomic_store __atomic_compare_exchange __atomic_load from builtins. should i just list them there?
|
You probably need to wait until the failure goes away, rebase on top of master and retry. Some minors, otherwise I think this is fine. |
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
| # __atomic_store, __atomic_compare_exchange, __atomic_load | ||
| elseif("${sdk}" STREQUAL "FREEBSD") | ||
| list(APPEND swiftlib_private_link_libraries_targets | ||
| "${SWIFTLIB_DIR}/clang/lib/freebsd/libclang_rt.builtins-${arch}.a") |
There was a problem hiding this comment.
This seems like a very specific path to hardcode, but I'll take your word for it that it's a reasonable. (Especially for a CMake section we'd like to clean up later.)
There was a problem hiding this comment.
This hard-coded path does not work on my build. The .a file has been moved to build/Ninja-ReleaseAssert/llvm-freebsd-x86_64/lib/clang/5.0.0/lib/freebsd/libclang_rt.builtins-${arch}.a.
There was a problem hiding this comment.
Ah, ${SWIFTLIB_DIR}/clang should be a symlink to lib/clang/5.0.0, so if you're seeing issues here, something else has gone wrong.
There was a problem hiding this comment.
Note that if you haven't built in a long time, the Clang version of the symlink may not match the actual Clang version.
There was a problem hiding this comment.
Interesting. I checked and the symlink is there and correct. I wonder why cmake doesn't follow it...
There was a problem hiding this comment.
Also, I commented out these lines and swift built just fine. So it might be the case that they can be removed in the future.
There was a problem hiding this comment.
It's possible you just weren't using anything (from C) that needed atomics.
|
If @gparker42 has no further objections then I think this can go in. It's certainly not going to break anyone else. |
| #ifdef __ANDROID__ | ||
| #if defined(__ANDROID__) || defined(__FreeBSD__) | ||
| #if defined(__ANDROID__) | ||
| const char *systemPath = "/system/lib"; |
There was a problem hiding this comment.
Any chance you could change this to “/system” while you’re in the neighbourhood as there are libraries that are blocked in “/system/vendor” as well as “/system/lib" on some devices. None of these libraries will have Swift implementations to worry about conformance for. Saves us all a PR.
There was a problem hiding this comment.
:-/ Let's not mix FreeBSD and Android changes.
|
Looks good. @dcci? |
|
LGTM, thanks! |
This PR fixes a few issues on FreeBSD 11 / 12
__atomic_store__atomic_compare_exchange__atomic_load*dlopen() failed on ./main: (NULL)Abort (core dumped)Related issues: #10836 (comment)
* Couldn't find a better way.