Enable MSVC builds in CI

.github/workflows/ci.yml

@@ -34,16 +34,26 @@ jobs:
     - uses: actions/checkout@v2
 
     - name: bazel build
-      run: CC=clang bazel build //...
+      run: CC=clang bazel build -s --verbose_failures //...
     - name: bazel test
-      run: CC=clang bazel test //...
+      run: CC=clang bazel test -s --verbose_failures --test_output=streamed //...
 
   gcc-tests:
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
 
     - name: bazel build
-      run: CC=gcc bazel build //...
+      run: CC=gcc bazel build -s --verbose_failures //...
     - name: bazel test
-      run: CC=gcc bazel test //...
+      run: CC=gcc bazel test -s --verbose_failures --test_output=streamed //...
+
+  msvc-tests:
+    runs-on: windows-2022
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: bazel build
+      run: bazel.exe build -s --verbose_failures //...
+    - name: bazel test
+      run: bazel.exe test -s --verbose_failures --test_output=streamed //...

BUILD

@@ -13,13 +13,13 @@
 # limitations under the License.
 
 load(
-    "//:copts.bzl",
-    "CWISS_DEFAULT_COPTS",
-    "CWISS_DEFAULT_LINKOPTS",
-    "CWISS_SAN_COPTS",
-    "CWISS_TEST_COPTS",
-    "CWISS_C_VERSION",
-    "CWISS_CXX_VERSION",
+    "//toolchain:copts.bzl",
+    "DEFAULT_COPTS",
+    "DEFAULT_LINKOPTS",
+    "SAN_COPTS",
+    "TEST_COPTS",
+    "C_VERSION",
+    "CXX_VERSION",
 )
 
 filegroup(
@@ -49,8 +49,8 @@ cc_library(
     name = "cwisstable_split",
     hdrs = [":public_headers"],
     srcs = [":private_headers"],
-    copts = CWISS_DEFAULT_COPTS + CWISS_C_VERSION,
-    linkopts = CWISS_DEFAULT_LINKOPTS,
+    copts = DEFAULT_COPTS + C_VERSION,
+    linkopts = DEFAULT_LINKOPTS,
     visibility = ["//visibility:public"],
 )
 
@@ -74,26 +74,26 @@ genrule(
 cc_library(
     name = "cwisstable",
     hdrs = ["cwisstable.h"],
-    copts = CWISS_DEFAULT_COPTS + CWISS_C_VERSION,
-    linkopts = CWISS_DEFAULT_LINKOPTS,
+    copts = DEFAULT_COPTS + C_VERSION,
+    linkopts = DEFAULT_LINKOPTS,
     visibility = ["//visibility:public"],
 )
 
 cc_library(
     name = "debug",
     hdrs = ["cwisstable/internal/debug.h"],
     srcs = ["cwisstable/internal/debug.cc"],
-    copts = CWISS_DEFAULT_COPTS + CWISS_CXX_VERSION,
-    linkopts = CWISS_DEFAULT_LINKOPTS,
+    copts = DEFAULT_COPTS + CXX_VERSION,
+    linkopts = DEFAULT_LINKOPTS,
     deps = [":cwisstable"],
     visibility = ["//:__subpackages__"],
 )
 
 cc_library(
     name = "test_helpers",
     hdrs = ["cwisstable/internal/test_helpers.h"],
-    copts = CWISS_DEFAULT_COPTS + CWISS_CXX_VERSION,
-    linkopts = CWISS_DEFAULT_LINKOPTS,
+    copts = DEFAULT_COPTS + CXX_VERSION,
+    linkopts = DEFAULT_LINKOPTS,
     deps = ["//:cwisstable"],
     visibility = ["//:__subpackages__"],
 )
@@ -109,8 +109,8 @@ cc_test(
         "@com_google_absl//absl/cleanup",
         "@com_google_googletest//:gtest_main",
     ],
-    copts = CWISS_TEST_COPTS + CWISS_CXX_VERSION + CWISS_SAN_COPTS,
-    linkopts = CWISS_DEFAULT_LINKOPTS + CWISS_SAN_COPTS,
+    copts = TEST_COPTS + CXX_VERSION + SAN_COPTS,
+    linkopts = DEFAULT_LINKOPTS + SAN_COPTS,
 )
 
 cc_test(
@@ -128,8 +128,8 @@ cc_test(
         "CWISS_HAVE_SSE2=0",
         "CWISS_HAVE_SSSE3=0",
     ],
-    copts = CWISS_TEST_COPTS + CWISS_CXX_VERSION + CWISS_SAN_COPTS,
-    linkopts = CWISS_DEFAULT_LINKOPTS + CWISS_SAN_COPTS,
+    copts = TEST_COPTS + CXX_VERSION + SAN_COPTS,
+    linkopts = DEFAULT_LINKOPTS + SAN_COPTS,
 )
 
 
@@ -146,25 +146,7 @@ cc_binary(
         "@com_google_absl//absl/strings:str_format",
         "@com_github_google_benchmark//:benchmark_main",
     ],
-    copts = CWISS_TEST_COPTS + CWISS_CXX_VERSION,
-    linkopts = CWISS_DEFAULT_LINKOPTS,
+    copts = TEST_COPTS + CXX_VERSION,
+    linkopts = DEFAULT_LINKOPTS,
     testonly = 1,
-)
-
-config_setting(
-    name = "clang_compiler",
-    flag_values = {"@bazel_tools//tools/cpp:compiler": "clang"},
-    visibility = [":__subpackages__"],
-)
-
-config_setting(
-    name = "msvc_compiler",
-    flag_values = {"@bazel_tools//tools/cpp:compiler": "mscv-cl"},
-    visibility = [":__subpackages__"],
-)
-
-config_setting(
-    name = "clang-cl_compiler",
-    flag_values = {"@bazel_tools//tools/cpp:compiler": "clang-cl"},
-    visibility = [":__subpackages__"],
 )

cwisstable/internal/base.h

@@ -23,28 +23,12 @@
 
 /// C++11 compatibility macros.
 ///
-/// Atomic support, due to incompatibilities between C++ and C11 atomic syntax.
-/// - `CWISS_ATOMIC_T(Type)` names an atomic version of `Type`. We must use this
-///   instead of `_Atomic(Type)` to name an atomic type.
-/// - `CWISS_ATOMIC_INC(value)` will atomically increment `value` without
-///   performing synchronization. This is used as a weak entropy source
-///   elsewhere.
-///
 /// `extern "C"` support via `CWISS_END_EXTERN` and `CWISS_END_EXTERN`,
 /// which open and close an `extern "C"` block in C++ mode.
 #ifdef __cplusplus
-  #include <atomic>
-  #define CWISS_ATOMIC_T(Type_) std::atomic<Type_>
-  #define CWISS_ATOMIC_INC(val_) (val_).fetch_add(1, std::memory_order_relaxed)
-
   #define CWISS_BEGIN_EXTERN extern "C" {
   #define CWISS_END_EXTERN }
 #else
-  #include <stdatomic.h>
-  #define CWISS_ATOMIC_T(Type_) _Atomic(Type_)
-  #define CWISS_ATOMIC_INC(val_) \
-    atomic_fetch_add_explicit(&(val_), 1, memory_order_relaxed)
-
   #define CWISS_BEGIN_EXTERN
   #define CWISS_END_EXTERN
 #endif
@@ -87,6 +71,31 @@
   #define CWISS_GCC_POP
 #endif
 
+/// Atomic support, due to incompatibilities between C++ and C11 atomic syntax.
+/// - `CWISS_ATOMIC_T(Type)` names an atomic version of `Type`. We must use this
+///   instead of `_Atomic(Type)` to name an atomic type.
+/// - `CWISS_ATOMIC_INC(value)` will atomically increment `value` without
+///   performing synchronization. This is used as a weak entropy source
+///   elsewhere.
+///
+/// MSVC, of course, being that it does not support _Atomic in C mode, forces us
+/// into `volatile`. This is *wrong*, but MSVC certainly won't miscompile it any
+/// worse than it would a relaxed atomic. It doesn't matter for our use of
+/// atomics.
+#ifdef __cplusplus
+  #include <atomic>
+  #define CWISS_ATOMIC_T(Type_) volatile std::atomic<Type_>
+  #define CWISS_ATOMIC_INC(val_) (val_).fetch_add(1, std::memory_order_relaxed)
+#elif CWISS_IS_MSVC
+  #define CWISS_ATOMIC_T(Type_) volatile Type_
+  #define CWISS_ATOMIC_INC(val_) (val_ += 1)
+#else
+  #include <stdatomic.h>
+  #define CWISS_ATOMIC_T(Type_) volatile _Atomic(Type_)
+  #define CWISS_ATOMIC_INC(val_) \
+    atomic_fetch_add_explicit(&(val_), 1, memory_order_relaxed)
+#endif
+
 /// Warning control around `CWISS` symbol definitions. These macros will
 /// disable certain false-positive warnings that `CWISS` definitions tend to
 /// emit.
@@ -135,6 +144,29 @@
   #include <tmmintrin.h>
 #endif
 
+/// `CWISS_HAVE_MUL128` is nonzero if there is compiler-specific
+/// intrinsics for 128-bit multiplication.
+///
+/// `-DCWISS_HAVE_MUL128=0` can be used to explicitly fall back onto the pure
+/// C implementation.
+#ifndef DCWISS_HAVE_MUL128
+  #if defined(__SIZEOF_INT128__) && \
+      ((CWISS_IS_CLANG && !CWISS_IS_MSVC) || CWISS_IS_GCC)
+    #define DCWISS_HAVE_MUL128 1
+  #else
+    #define DCWISS_HAVE_MUL128 0
+  #endif
+#endif
+
+/// `CWISS_ALIGN` is a cross-platform `alignas()`: specifically, MSVC doesn't
+/// quite believe in it.
+#if CWISS_IS_MSVC
+  #define CWISS_alignas(align_) __declspec(align(align_))
+#else
+  #include <stdalign.h>
+  #define CWISS_alignas(align_) alignas(align_)
+#endif
+
 /// `CWISS_HAVE_BUILTIN` will, in Clang, detect whether a Clang language
 /// extension is enabled.
 ///

cwisstable/internal/bits.h

@@ -129,12 +129,37 @@ typedef struct {
   uint64_t lo, hi;
 } CWISS_U128;
 
+/// Computes a 64-bit add-with-carry.
+static inline bool CWISS_Adc64(uint64_t* a, uint64_t b) {
+  *a += b;
+  return *a < b;
+}
+
 /// Computes a double-width multiplication operation.
 static inline CWISS_U128 CWISS_Mul128(uint64_t a, uint64_t b) {
-  // TODO: de-intrinsics-ize this.
+#if DCWISS_HAVE_MUL128
+  #if CWISS_IS_MSVC && defined(_M_X64)
+  CWISS_U128 result;
+  result.lo = _umul128(a, b, &result.hi);
+  return result;
+  #else
   __uint128_t p = a;
   p *= b;
   return (CWISS_U128){(uint64_t)p, (uint64_t)(p >> 64)};
+  #endif
+#else
+  // Taken from Abseil.
+  uint64_t a32 = a >> 32;
+  uint64_t a00 = a & 0xffffffff;
+  uint64_t b32 = b >> 32;
+  uint64_t b00 = b & 0xffffffff;
+
+  CWISS_U128 result = {a00 * b00, a32 * b32};
+  result.hi += ((a32 * b00) >> 32) + CWISS_Adc64(&result.lo, (a32 * b00) << 32);
+  result.hi += ((a00 * b32) >> 32) + CWISS_Adc64(&result.lo, (a00 * b32) << 32);
+
+  return result;
+#endif
 }
 
 /// Loads an unaligned u32.

cwisstable/internal/capacity.h

@@ -83,7 +83,7 @@ static inline size_t RandomSeed(void) {
   static CWISS_THREAD_LOCAL size_t counter;
   size_t value = ++counter;
 #else
-  static volatile CWISS_ATOMIC_T(size_t) counter;
+  static CWISS_ATOMIC_T(size_t) counter;
   size_t value = CWISS_ATOMIC_INC(counter);
 #endif
   return value ^ ((size_t)&counter);

cwisstable/internal/control_byte.h

@@ -17,7 +17,6 @@
 
 #include <assert.h>
 #include <limits.h>
-#include <stdalign.h>
 #include <stdint.h>
 #include <string.h>
 
@@ -81,7 +80,7 @@ static_assert(CWISS_kDeleted == -2,
 static inline CWISS_ControlByte* CWISS_EmptyGroup() {
   // A single block of empty control bytes for tables without any slots
   // allocated. This enables removing a branch in the hot path of find().
-  alignas(16) static const CWISS_ControlByte kEmptyGroup[16] = {
+  CWISS_alignas(16) static const CWISS_ControlByte kEmptyGroup[16] = {
       CWISS_kSentinel, CWISS_kEmpty, CWISS_kEmpty, CWISS_kEmpty,
       CWISS_kEmpty,    CWISS_kEmpty, CWISS_kEmpty, CWISS_kEmpty,
       CWISS_kEmpty,    CWISS_kEmpty, CWISS_kEmpty, CWISS_kEmpty,