Author: Louis Dionne Date: 2023-02-10T08:19:32+01:00 New Revision: d956ed0d76afd1a0f778faad920cc4a52618b5f8
URL: https://github.com/llvm/llvm-project/commit/d956ed0d76afd1a0f778faad920cc4a52618b5f8 DIFF: https://github.com/llvm/llvm-project/commit/d956ed0d76afd1a0f778faad920cc4a52618b5f8.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 (cherry picked from commit 02718433a0dd3f8d3f3719b97aa1685b699ac785) 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 8846e5146f89a..51b100fa55699 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> _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
