Support for Apple Silicon (2nd try) #4110
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpilman
changed the title
| @@ -0,0 +1,16 @@ | |||
| set(SRCS pthread/thread.cpp | |||
There was a problem hiding this comment.
Should pthread/thread.cpp be included for WIN32?
There was a problem hiding this comment.
Probably not... Though I copied this code from the boost-cmake repository. I think a cleaner way of doing this is to change the cmake code to compile thread and context in ExternalProject_add (copying the files into the repository seems dangerous and painful)
|
One thing to be careful about here is that the run loop profiler had a bad interaction with our existing coroutine implementation that had to be resolved by temporarily disabling the profiler when context switching: foundationdb/fdbrpc/libcoroutine/Coro.c Line 315 in ea4c510 It seems possible that you could have a similar problem with a new library. |
@sfc-gh-ajbeamon do you know why it's only disabling profiling for the ucontext implementation? I would imagine that other implementations would have this problem. It makes sense that trying to capture a backtrace in the middle of changing stacks wouldn't work |
I don't remember off the top of my head, possibly checking old commit logs might have something. It could also just be that the ucontext implementation was the relevant one to our usage. |
|
@fdb-build run ctest please |
xis19
reviewed
Dec 1, 2020
| if(APPLE) | ||
| # Boost coroutine currently doesn't support Apple Silicon, so we have to fall | ||
| # back to the slower ucontext implementation | ||
| add_compile_definitions(BOOST_USE_UCONTEXT) |
There was a problem hiding this comment.
One thing I am not quite clear is that, previously we just use this code to download and deploy a local boost library:
find_package(Boost 1.72)
if(Boost_FOUND)
add_library(boost_target INTERFACE)
target_link_libraries(boost_target INTERFACE Boost::boost)
else()
include(ExternalProject)
ExternalProject_add(boostProject
URL "https://dl.bintray.com/boostorg/release/1.72.0/source/boost_1_72_0.tar.bz2"
URL_HASH SHA256=59c9b274bc451cf91a9ba1dd2c7fdcaf5d60b1b3aa83f2c9fa143417cc660722
CONFIGURE_COMMAND ""
BUILD_COMMAND ""
BUILD_IN_SOURCE ON
INSTALL_COMMAND ""
UPDATE_COMMAND ""
BUILD_BYPRODUCTS <SOURCE_DIR>/boost/config.hpp)
ExternalProject_Get_property(boostProject SOURCE_DIR)
set(BOOST_INCLUDE_DIR ${SOURCE_DIR})
message(STATUS "Boost include dir ${BOOST_INCLUDE_DIR}")
add_library(boost_target INTERFACE)
add_dependencies(boost_target boostProject)
target_include_directories(boost_target SYSTEM INTERFACE ${BOOST_INCLUDE_DIR})
endif()
Can we switch to this strategy rather than just injecting boost code into our code?
There was a problem hiding this comment.
The problem here is that boost coroutine2 depends on boost context - which is a library that needs to be compiled (so far we have only used header-only libraries). I agree that building this through ExternalProject_Add is preferable, but this will need some more effort (I might still do this - but I am working on this in my free time so this is more a "I'll do it when I feel like it" kind of situation ;) )
There was a problem hiding this comment.
to make this even worse, it doesn't seem that boost compiles on my machine if I use the boost build system... Overall it might be easier to just wait for other stuff to support Apple Silicon (before I continue down the path of patching all of our dependencies...)
There was a problem hiding this comment.
I gotcha... I agree BJam is indeed hard to use -- I always use Debian prebuilt binaries.
|
The boost.coroutine thing might also unblock the #2235 musl work. |
|
Yes, I think we should merge this - even if it doesn't run on M1 macs. It seems that the only things missing are:
|
|
Tried using the C bindings from Rust produced by this branch and I'm getting a SIGBUS crash. I'm curious if anyone else was able to use them. Here's the dump: |
I have the same SIGBUS crash when trying to run my golang application with go bindings |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is resolves #4111
Changes in this PR:
General guideline:
Please verify that all things listed below were considered and check them. If an item doesn't apply to this type of PR (for example a documentation change doesn't need to be performance tested), you should make a
strikethrough(markdown syntax:~~strikethrough~~). More infos on the guidlines can be found here.Style
git clang-format).Performance
std::vectorvsVectorRef).SlowTasktraces.Testing
ASSERT,ASSERT_WE_THINK, andTESTmacros are added in appropriate places.