[libc++] Guard the fix to CityHash behind ABI v2

[ldionne]

Summary

As explained in a comment in https://reviews.llvm.org/D134124, we tried
landing this unconditionally but this actually bit some users who were
sharing std::unordered_map across an ABI boundary. This shows that the
ABI break is not benign and it should be guarded behind ABI v2.

[ldionne]

I will land this once CI passes and then cherry-pick it onto release/16.x.

@oToToT Once you have a suggestion for a better hashing algorithm, we can rename the _LIBCPP_ABI_FIX_CITYHASH_IMPLEMENTATION macro to something else and expand the scope of the #ifdef to include basically all of the implementation.

[Mordante]

libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp

15

Is there also a test that ABI=V1 keeps returning the same "wrong" value?

[ldionne]

libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp

15

Good point, see D144107.

[Mordante]

libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp

15

Thanks!

libcxx/include/__config

 // about ABI compatibility.
 #    define _LIBCPP_ABI_NO_RANDOM_DEVICE_COMPATIBILITY_LAYOUT
 // Don't export the legacy __basic_string_common class and its methods from the built library.
 #    define _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
 // Don't export the legacy __vector_base_common class and its methods from the built library.
 #    define _LIBCPP_ABI_DO_NOT_EXPORT_VECTOR_BASE_COMMON
 // According to the Standard, `bitset::operator[] const` returns bool
 #    define _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL
+// Fix the implementation of CityHash used for std::hash<fundamental-type>.
+// This is an ABI break because `std::hash` will return a different result,
+// which means that hashing the same object in translation units built against
+// different versions of libc++ can return inconsistent results. This is especially
+// tricky since std::hash is used in the implementation of unordered containers.
+//
+// The incorrect implementation of CityHash has the problem that it drops some
+// bits on the floor.
+#    define _LIBCPP_ABI_FIX_CITYHASH_IMPLEMENTATION
 // Remove the base 10 implementation of std::to_chars from the dylib.
 // The implementation moved to the header, but we still export the symbols from
 // the dylib for backwards compatibility.
 #    define _LIBCPP_ABI_DO_NOT_EXPORT_TO_CHARS_BASE_10
 #  elif _LIBCPP_ABI_VERSION == 1
 #    if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries. This is unnecessary with

libcxx/include/__functional/hash.h

static _Size __hash_len_0_to_16(const char* __s, _Size __len)
     if (__len > 8) {
       const _Size __a = std::__loadword<_Size>(__s);
       const _Size __b = std::__loadword<_Size>(__s + __len - 8);
       return __hash_len_16(__a, __rotate_by_at_least_1(__b + __len, __len)) ^ __b;
     }
     if (__len >= 4) {
       const uint32_t __a = std::__loadword<uint32_t>(__s);
       const uint32_t __b = std::__loadword<uint32_t>(__s + __len - 4);
+#ifdef _LIBCPP_ABI_FIX_CITYHASH_IMPLEMENTATION
       return __hash_len_16(__len + (static_cast<_Size>(__a) << 3), __b);
+#else
+      return __hash_len_16(__len + (__a << 3), __b);
+#endif
     }
     if (__len > 0) {
       const unsigned char __a = static_cast<unsigned char>(__s[0]);
       const unsigned char __b = static_cast<unsigned char>(__s[__len >> 1]);
       const unsigned char __c = static_cast<unsigned char>(__s[__len - 1]);
       const uint32_t __y = static_cast<uint32_t>(__a) +
                            (static_cast<uint32_t>(__b) << 8);
       const uint32_t __z = __len + (static_cast<uint32_t>(__c) << 2);

libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Test the CityHash implementation is correct.
 
 // UNSUPPORTED: c++03
 
+// In ABI v1, our CityHash implementation is incorrect and fixing it would
+// be an ABI break.
+// REQUIRES: libcpp-abi-version=2
+
 #include <cassert>
 #include <string>
 #include <utility>
 
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
 #  define CHOOSE_BY_ENDIANESS(little, big) (little)
 #else
 #  define CHOOSE_BY_ENDIANESS(little, big) (big)