Fix MSVC-related build issues #7439

kobykahane · 2020-09-26T12:35:39Z

This PR addresses some build and functional issues on MSVC targets, as a step towards an eventual goal of having RocksDB build successfully for Windows on ARM64.

Addressed issues include:

BitsSetToOne and CountTrailingZeroBits do not compile on non-x64 MSVC targets. A fallback implementation of BitsSetToOne when Intel intrinsics are not available is added, based on the C++20 <bit> popcount implementation in Microsoft's STL.
The implementation of FloorLog2 for MSVC targets (including x64) gives incorrect results. The unit test easily detects this, but CircleCI is currently configured to only run a specific set of tests for Windows CMake builds, so this seems to have been unnoticed.
AsmVolatilePause does not use YieldProcessor on Windows ARM64 targets, even though it is available.
When CondVar::TimedWait calls Microsoft STL's condition_variable::wait_for, it can potentially trigger a bug (just recently fixed in the upcoming VS 16.8's STL) that deadlocks various tests that wait for a timer to execute, since Timer::Run doesn't get a chance to execute before being blocked by the test function acquiring the mutex.
In c_test, GetTempDir assumes a POSIX-style temp path.
NormalizePath did not eliminate consecutive POSIX-style path separators on Windows, resulting in test failures in e.g., wal_manager_test.
Various other test failures.

In a followup PR I hope to modify CircleCI's config.yml to invoke all RocksDB unit tests in Windows CMake builds with CTest, instead of the current use of run_ci_db_test.ps1 which requires individual tests to be specified and is missing many of the existing tests.

Notes from peterd: FloorLog2 is not yet used in production code (it's for something in progress). I also added a few more inexpensive platform-dependent tests to Windows CircleCI runs. And included facebook/folly#1461 as requested

_M_I86 is the macro for very legacy 16-bit Visual C++ targets, not 32-bit, for which the macro is _M_IX86.

…Pause.

…g and reacquiring mutex.

…targets.

… NowCPUNanos.

…nEnv rather than PosixEnv is in use.

pdillinger

LGTM! Thanks for the fix!

pdillinger · 2020-09-30T16:26:39Z

util/xxh3p.h

-#  if defined(_MSC_VER) && (defined(_M_X64) || defined(_M_I86))  /* _mm_prefetch() is not defined outside of x86/x64 */
+#if defined(_MSC_VER) && \
+    (defined(_M_X64) ||  \
+     defined(_M_IX86)) /* _mm_prefetch() is not defined outside of x86/x64 */


@Cyan4973 This compilation bug (_M_I86) is still upstream also https://github.com/Cyan4973/xxHash/blob/dev/xxhash.h#L2484

Thanks for the pointer @pdillinger !

My understanding of this bug is that,
under x86 (32-bit) compilation on MSVC,
it would disable prefetching, hence lose performance,

but nothing would crash at runtime, nor fail compilation,
neither on x86 nor on any other target ?

The fix is straightforward,
I'm just wondering is there is any relevant test to add in CI
that would have been able to detect such an issue.
It's a tiny nit, and hard to notice without external impact.
But @kobykahane was nonetheless able to detect and fix it.

edit : maybe this could impact compilation for 16-bit x86 target.
Though I don't see a point supporting such a target nowadays...

The reason I found it was because I grepped the entire tree for _M_X64 to find x64-isms as part of my goal to get the code to build on ARM64 MSVC targets.

kobykahane · 2020-09-30T16:56:28Z

@pdillinger Please also consider cherry-picking facebook/folly#1461 so the vendored Folly in this project also has that fix.

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-01T16:43:21Z

@pdillinger merged this pull request in 3e74505.

Summary: This PR addresses some build and functional issues on MSVC targets, as a step towards an eventual goal of having RocksDB build successfully for Windows on ARM64. Addressed issues include: - BitsSetToOne and CountTrailingZeroBits do not compile on non-x64 MSVC targets. A fallback implementation of BitsSetToOne when Intel intrinsics are not available is added, based on the C++20 `<bit>` popcount implementation in Microsoft's STL. - The implementation of FloorLog2 for MSVC targets (including x64) gives incorrect results. The unit test easily detects this, but CircleCI is currently configured to only run a specific set of tests for Windows CMake builds, so this seems to have been unnoticed. - AsmVolatilePause does not use YieldProcessor on Windows ARM64 targets, even though it is available. - When CondVar::TimedWait calls Microsoft STL's condition_variable::wait_for, it can potentially trigger a bug (just recently fixed in the upcoming VS 16.8's STL) that deadlocks various tests that wait for a timer to execute, since `Timer::Run` doesn't get a chance to execute before being blocked by the test function acquiring the mutex. - In c_test, `GetTempDir` assumes a POSIX-style temp path. - `NormalizePath` did not eliminate consecutive POSIX-style path separators on Windows, resulting in test failures in e.g., wal_manager_test. - Various other test failures. In a followup PR I hope to modify CircleCI's config.yml to invoke all RocksDB unit tests in Windows CMake builds with CTest, instead of the current use of `run_ci_db_test.ps1` which requires individual tests to be specified and is missing many of the existing tests. Notes from peterd: FloorLog2 is not yet used in production code (it's for something in progress). I also added a few more inexpensive platform-dependent tests to Windows CircleCI runs. And included facebook/folly#1461 as requested Pull Request resolved: facebook#7439 Reviewed By: jay-zhuang Differential Revision: D24021563 Pulled By: pdillinger fbshipit-source-id: 0ec2027c0d6a494d8a0fe38d9667fc2f7e29f7e7

Summary: This PR addresses some build and functional issues on MSVC targets, as a step towards an eventual goal of having RocksDB build successfully for Windows on ARM64. Addressed issues include: - BitsSetToOne and CountTrailingZeroBits do not compile on non-x64 MSVC targets. A fallback implementation of BitsSetToOne when Intel intrinsics are not available is added, based on the C++20 `<bit>` popcount implementation in Microsoft's STL. - The implementation of FloorLog2 for MSVC targets (including x64) gives incorrect results. The unit test easily detects this, but CircleCI is currently configured to only run a specific set of tests for Windows CMake builds, so this seems to have been unnoticed. - AsmVolatilePause does not use YieldProcessor on Windows ARM64 targets, even though it is available. - When CondVar::TimedWait calls Microsoft STL's condition_variable::wait_for, it can potentially trigger a bug (just recently fixed in the upcoming VS 16.8's STL) that deadlocks various tests that wait for a timer to execute, since `Timer::Run` doesn't get a chance to execute before being blocked by the test function acquiring the mutex. - In c_test, `GetTempDir` assumes a POSIX-style temp path. - `NormalizePath` did not eliminate consecutive POSIX-style path separators on Windows, resulting in test failures in e.g., wal_manager_test. - Various other test failures. In a followup PR I hope to modify CircleCI's config.yml to invoke all RocksDB unit tests in Windows CMake builds with CTest, instead of the current use of `run_ci_db_test.ps1` which requires individual tests to be specified and is missing many of the existing tests. Notes from peterd: FloorLog2 is not yet used in production code (it's for something in progress). I also added a few more inexpensive platform-dependent tests to Windows CircleCI runs. And included facebook/folly#1461 as requested Pull Request resolved: facebook/rocksdb#7439 Reviewed By: jay-zhuang Differential Revision: D24021563 Pulled By: pdillinger fbshipit-source-id: 0ec2027c0d6a494d8a0fe38d9667fc2f7e29f7e7 Signed-off-by: Changlong Chen <levisonchen@live.cn>

kobykahane added 10 commits September 26, 2020 14:01

Fix BitsSetToOne compilation on MSVC non-x64 targets.

3033837

Fix CountTrailingZeroBits compilation on MSVC non-x64 targets.

e4a616b

Fix incorrect FloorLog2 implementation for MSVC targets.

f742dbe

Fix incorrect detection of x86 MSVC targets in xxhash.

fc5ce99

_M_I86 is the macro for very legacy 16-bit Visual C++ targets, not 32-bit, for which the macro is _M_IX86.

Invoke YieldProcessor on ARM and ARM64 Windows targets in AsmVolatile…

df7c699

…Pause.

Work around Microsoft STL condition_variable::wait_for(0) not droppin…

75f491e

…g and reacquiring mutex.

Fix c_test GetTempDir on Windows targets.

06e3c93

Fix NormalizePath to also normalize POSIX path separators on Windows …

a379d1a

…targets.

Skip PerfContextTest.CPUTimer on targets like Windows that don't have…

eae83c3

… NowCPUNanos.

Don't expect thread id in status in LockTest.LockBySameThread when Wi…

8bbfa96

…nEnv rather than PosixEnv is in use.

facebook-github-bot added the CLA Signed label Sep 26, 2020

kobykahane mentioned this pull request Sep 26, 2020

[rocksdb] update to v6.11.4 microsoft/vcpkg#13643

Merged

make format

76ae019

pdillinger self-requested a review September 30, 2020 16:06

pdillinger added 2 commits September 30, 2020 09:21

Add some more unit tests to Windows CircleCI runs

d6d8153

Merge remote-tracking branch 'origin/master' into pr/7439

29ccd40

pdillinger approved these changes Sep 30, 2020

View reviewed changes

pdillinger added 2 commits September 30, 2020 10:16

Improve readability and <limits> unnecesary

c9a957d

Include facebook/folly#1461

c612ae2

facebook-github-bot reviewed Sep 30, 2020

View reviewed changes

facebook-github-bot closed this in 3e74505 Oct 1, 2020

facebook-github-bot added the Merged label Oct 1, 2020

kobykahane deleted the fix_msvc_non_x64_build branch October 1, 2020 16:55

kobykahane mentioned this pull request Oct 19, 2020

[rocksdb] update to v6.13.3 microsoft/vcpkg#14085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MSVC-related build issues #7439

Fix MSVC-related build issues #7439

kobykahane commented Sep 26, 2020 •

edited by pdillinger

Loading

pdillinger left a comment

pdillinger Sep 30, 2020

Cyan4973 Sep 30, 2020 •

edited

Loading

kobykahane Sep 30, 2020

kobykahane commented Sep 30, 2020

facebook-github-bot left a comment

facebook-github-bot commented Oct 1, 2020

Fix MSVC-related build issues #7439

Fix MSVC-related build issues #7439

Conversation

kobykahane commented Sep 26, 2020 • edited by pdillinger Loading

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Sep 30, 2020

Choose a reason for hiding this comment

Cyan4973 Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

kobykahane Sep 30, 2020

Choose a reason for hiding this comment

kobykahane commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2020

kobykahane commented Sep 26, 2020 •

edited by pdillinger

Loading

Cyan4973 Sep 30, 2020 •

edited

Loading