[libcxx-commits] [libcxx] 0271843 - [libc++] Guard the fix to CityHash behind ABI v2

[Louis Dionne via libcxx-commits]

Author: Louis Dionne
Date: 2023-02-09T17:10:02-08:00
New Revision: 02718433a0dd3f8d3f3719b97aa1685b699ac785

URL: https://github.com/llvm/llvm-project/commit/02718433a0dd3f8d3f3719b97aa1685b699ac785
DIFF: https://github.com/llvm/llvm-project/commit/02718433a0dd3f8d3f3719b97aa1685b699ac785.diff

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

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.

Differential Revision: https://reviews.llvm.org/D143688

Added: 
    

Modified: 
    libcxx/include/__config
    libcxx/include/__functional/hash.h
    libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__config b/libcxx/include/__config
index 384bc6badccae..be15ce2457ac9 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -134,6 +134,15 @@
 #    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 
diff erent result,
+// which means that hashing the same object in translation units built against
+// 
diff erent 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.

diff  --git a/libcxx/include/__functional/hash.h b/libcxx/include/__functional/hash.h
index dfd8ea2553dc8..39382aa9bff42 100644
--- a/libcxx/include/__functional/hash.h
+++ b/libcxx/include/__functional/hash.h
@@ -140,7 +140,11 @@ struct __murmur2_or_cityhash<_Size, 64>
     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]);

diff  --git a/libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp b/libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp
index e2905a450a097..13fc9c33d68f7 100644
--- a/libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp
+++ b/libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp
@@ -10,6 +10,10 @@
 
 // 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>