cross-platform compatibility improvements #2199
Conversation
benesch
force-pushed
the
build-feature-detection
branch
from
9ead96a
to
0b997a9
Compare
|
@benesch updated the pull request - view changes |
benesch
force-pushed
the
build-feature-detection
branch
from
0b997a9
to
1dbce9d
Compare
|
@benesch updated the pull request - view changes |
benesch
force-pushed
the
build-feature-detection
branch
from
1dbce9d
to
6959d1e
Compare
|
@benesch updated the pull request - view changes |
benesch
force-pushed
the
build-feature-detection
branch
from
6959d1e
to
2ebc708
Compare
|
@benesch updated the pull request - view changes |
benesch
force-pushed
the
build-feature-detection
branch
from
2ebc708
to
8d4efab
Compare
|
@benesch updated the pull request - view changes |
benesch
force-pushed
the
build-feature-detection
branch
from
8d4efab
to
c96751d
Compare
|
@benesch updated the pull request - view changes |
|
Sorry to be a pest, but now that this is green, could someone take a peek to see if this is something you're interested in? Landing any of these changes would be a huge win for us at CockroachDB, and would love to work with you to excise any bits you feel are too risky. |
The previous approach to enabling SSE4.2 intrinsics was rather
convoluted, and conflated AVX support with SSE4.2 on Windows. This
commit attempts to provide more sensible logic across both CMake and
non-CMake builds. The new behavior is as follows:
* SSE4.2 support is detected during build configuration by compiling
a test program that uses SSE4.2 intrinsics. If this succeeds, then
the macro -DHAVE_SSE42 is defined. This detection occurs
identically in both CMake and non-CMake builds.
* Compile-time tests for whether SSE4.2 is available simplify
to `#ifdef HAVE_SSE42` on all platforms.
* AVX support is no longer incorrectly equated with SSE4.2 support.
Compiling with `/arch:SSE2` on MSVC will, confusingly, allow use
of SSE4.2 intrinsics, provided the user is running a version of
Visual Studio with SSE4.2 support. Since `/arch:SSE2` is the
default when compiling with MSVC, so no special treatment is
necessary to enable SSE. The WITH_AVX2 option is thus removed.
* Instead, CMake builds learn to build PORTABLE binaries, for
consistency with non-CMake builds. Non-PORTABLE builds add
`-march=native` to CXXFLAGS, except on MSVC, where they add the
roughly-equivalent `/arch:AVX2` flag.
* The WITH_SSE42 option for CMake builds is renamed to FORCE_SSE42,
which can be used to abort the build if the compiler does not
support SSE4.2. To avoid breaking people's workflows,
build_detect_platform will print a warning if USE_SSE=1 is
specified when SSE4.2 is not supported, but it will not abort the
build.
Previously, checks for thread-local storage (TLS) support were inconsistent and based on the target OS. I.e., in several places, thread-local storage was disabled on macOS and Windows; in others, it was only disabled when cross-compiling for iOS. This commit instead detects support for thread-local storage by attempting to compile a test program that uses thread-local storage at build-configuration time. If this compilation succeeds, then the ROCKSDB_SUPPORT_THREAD_LOCAL macro is defined. With this change, RocksDB can now be compiled on OpenBSD with Clang. (Clang on OpenBSD does not yet support thread-local storage.) This also enables several TLS-requiring features on macOS and Windows that were presumably disabled in the days before macOS and Windows had TLS support.
benesch
force-pushed
the
build-feature-detection
branch
from
c96751d
to
357e810
Compare
|
@benesch updated the pull request - view changes |
librt doesn't exist on macOS or BSD.
benesch
force-pushed
the
build-feature-detection
branch
from
357e810
to
47075c9
Compare
|
@benesch updated the pull request - view changes |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@benesch Thank you for your contribution and sorry for late reply! The patch looking good. I'm merging it with two minor changes. You don't need to do anything.
|
|
No problem, @yiwu-arbug. Glad to hear the good news! @tamird just noticed that the last commit doesn't quite do what it says on the tin: it stops linking against librt on all platforms, not just Linux. Curiously, the Travis CI build still succeeds, suggesting that, at least with sufficiently recent versions of glibc, librt is no longer needed. Still, I think we should either adjust the commit message or fix the commit to do what the commit message claims it does. I don't want to interfere with any commits you might be basing off this branch, though. What do you suggest? |
|
Hey @benesch and @yiwu-arbug , this patch will break the compile for all the people that include
|
|
Hey @igorcanadi . I already had a PR for this: #2336 |
|
Thanks @lightmark ! |
|
Sorry about that, @igorcanadi, and thanks, @lightmark! |
Summary: … headers #2199 should not reference RocksDB-specific macros (like ROCKSDB_SUPPORT_THREAD_LOCAL in this case) to public headers, `iostats_context.h` and `perf_context.h`. We shouldn't do that because users have to provide these compiler flags when building their binary with RocksDB. We should hide the thread local global variable inside our implementation and just expose a function api to retrieve these variables. It may break some users for now but good for long term. make check -j64 Closes #2380 Differential Revision: D5177896 Pulled By: lightmark fbshipit-source-id: 6fcdfac57f2e2dcfe60992b7385c5403f6dcb390
In RocksDB 5.5, the correct way to build a portable binary was to disable SSE4.2. In RocksDB 5.6+ (specifically, since facebook/rocksdb#2199), the correct way to build a portable binary is to specify -DPORTABLE=ON; non-portable binaries otherwise pass -march=native to the compiler. Fixes cockroachdb#19909.
In RocksDB 5.5, the correct way to build a portable binary was to disable SSE4.2. In RocksDB 5.6+ (specifically, since facebook/rocksdb#2199), the correct way to build a portable binary is to specify -DPORTABLE=ON; non-portable binaries otherwise pass -march=native to the compiler. Fixes cockroachdb#19909.
We've had a couple CockroachDB users fail to build RocksDB on exotic platforms, so I figured I'd try my hand at solving these issues upstream. The problems stem from a)
USE_SSE=1being too aggressive about turning on SSE4.2, even on toolchains that don't support SSE4.2 and b) RocksDB attempting to detect support for thread-local storage based on OS, even though it can vary by compiler on the same OS.See the individual commit messages for details. Regarding SSE support, this PR should change virtually nothing for non-CMake based builds.
make,PORTABLE=1 make,USE_SSE=1 make, andPORTABLE=1 USE_SSE=1 makefunction exactly as before, except that SSE support will be automatically disabled when a simple SSE4.2-using test program fails to compile, as it does on OpenBSD. (OpenBSD's ports GCC supports SSE4.2, but its binutils do not, so__SSE_4_2__is defined but an SSE4.2-using program will fail to assemble.) A warning is emitted in this case. The CMake build is modified to support the same set of options, except thatUSE_SSEis spelledFORCE_SSE42becauseUSE_SSEis rather useless now that we can automatically detect SSE support, and I figure changing options in the CMake build is less disruptive than changing the non-CMake build.I've tested these changes on all the platforms I can get my hands on (macOS, Windows MSVC, Windows MinGW, and OpenBSD) and it all works splendidly. Let me know if there's anything you object to—I obviously don't mean to break any of your build pipelines in the process of fixing ours downstream.