[libcxx-commits] [libcxx] 2393ab6 - [libc++] Fix unintended ABI break in associative containers with reference comparators (#118685)

[via libcxx-commits]

Author: Louis Dionne
Date: 2024-12-05T15:56:00-05:00
New Revision: 2393ab65edda14cc416068ebee0b512228723877

URL: https://github.com/llvm/llvm-project/commit/2393ab65edda14cc416068ebee0b512228723877
DIFF: https://github.com/llvm/llvm-project/commit/2393ab65edda14cc416068ebee0b512228723877.diff

LOG: [libc++] Fix unintended ABI break in associative containers with reference comparators (#118685)

While reference comparators are a terrible idea and it's not entirely
clear whether they are supported, fixing the unintended ABI break is
straightforward so we should do it as a first step.

Fixes #118559

Added: 
    libcxx/test/libcxx/containers/associative/reference_comparator_abi.compile.pass.cpp

Modified: 
    libcxx/include/__memory/compressed_pair.h

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__memory/compressed_pair.h b/libcxx/include/__memory/compressed_pair.h
index a7acaaff9da099..38798a21fa3c9c 100644
--- a/libcxx/include/__memory/compressed_pair.h
+++ b/libcxx/include/__memory/compressed_pair.h
@@ -50,9 +50,19 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 // member1 - offset: 0, size: 4
 // member2 - offset: 4, size: 4
 // member3 - offset: 8, size: 8
+//
+// Furthermore, that alignment must be the same as what was used in the old __compressed_pair layout, so we must
+// handle reference types specially since alignof(T&) == alignof(T).
+// See https://github.com/llvm/llvm-project/issues/118559.
 
 #ifndef _LIBCPP_ABI_NO_COMPRESSED_PAIR_PADDING
 
+template <class _Tp>
+inline const size_t __compressed_pair_alignment = _LIBCPP_ALIGNOF(_Tp);
+
+template <class _Tp>
+inline const size_t __compressed_pair_alignment<_Tp&> = _LIBCPP_ALIGNOF(void*);
+
 template <class _ToPad,
           bool _Empty = ((is_empty<_ToPad>::value && !__libcpp_is_final<_ToPad>::value) ||
                          is_reference<_ToPad>::value || sizeof(_ToPad) == __datasizeof_v<_ToPad>)>
@@ -64,14 +74,15 @@ template <class _ToPad>
 class __compressed_pair_padding<_ToPad, true> {};
 
 #  define _LIBCPP_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2)                                                  \
-    _LIBCPP_NO_UNIQUE_ADDRESS __attribute__((__aligned__(_LIBCPP_ALIGNOF(T2)))) T1 Initializer1;                       \
+    _LIBCPP_NO_UNIQUE_ADDRESS __attribute__((__aligned__(::std::__compressed_pair_alignment<T2>))) T1 Initializer1;    \
     _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _);          \
     _LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2;                                                                         \
     _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T2> _LIBCPP_CONCAT3(__padding2_, __LINE__, _)
 
 #  define _LIBCPP_COMPRESSED_TRIPLE(T1, Initializer1, T2, Initializer2, T3, Initializer3)                              \
     _LIBCPP_NO_UNIQUE_ADDRESS                                                                                          \
-    __attribute__((__aligned__(_LIBCPP_ALIGNOF(T2)), __aligned__(_LIBCPP_ALIGNOF(T3)))) T1 Initializer1;               \
+    __attribute__((__aligned__(::std::__compressed_pair_alignment<T2>),                                                \
+                   __aligned__(::std::__compressed_pair_alignment<T3>))) T1 Initializer1;                              \
     _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _);          \
     _LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2;                                                                         \
     _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T2> _LIBCPP_CONCAT3(__padding2_, __LINE__, _);          \

diff  --git a/libcxx/test/libcxx/containers/associative/reference_comparator_abi.compile.pass.cpp b/libcxx/test/libcxx/containers/associative/reference_comparator_abi.compile.pass.cpp
new file mode 100644
index 00000000000000..f364fc817c164a
--- /dev/null
+++ b/libcxx/test/libcxx/containers/associative/reference_comparator_abi.compile.pass.cpp
@@ -0,0 +1,57 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// Pin down the ABI of associative containers with respect to their size and alignment
+// when passed a comparator that is a reference.
+//
+// While it's not even clear that reference comparators are legal in containers, an
+// unintended ABI break was discovered after implementing the new compressed pair
+// mechanism based on [[no_unique_address]], and this is a regression test for that.
+// If we decide to make reference comparators ill-formed, this test would become
+// unnecessary.
+//
+// See https://github.com/llvm/llvm-project/issues/118559 for more details.
+
+#include <set>
+#include <map>
+
+#include "test_macros.h"
+
+struct TEST_ALIGNAS(16) Cmp {
+  bool operator()(int, int) const;
+};
+
+template <class Compare>
+struct Set {
+  char b;
+  std::set<int, Compare> s;
+};
+
+template <class Compare>
+struct Multiset {
+  char b;
+  std::multiset<int, Compare> s;
+};
+
+template <class Compare>
+struct Map {
+  char b;
+  std::map<int, char, Compare> s;
+};
+
+template <class Compare>
+struct Multimap {
+  char b;
+  std::multimap<int, char, Compare> s;
+};
+
+static_assert(sizeof(Set<Cmp&>) == sizeof(Set<bool (*)(int, int)>), "");
+static_assert(sizeof(Multiset<Cmp&>) == sizeof(Multiset<bool (*)(int, int)>), "");
+
+static_assert(sizeof(Map<Cmp&>) == sizeof(Map<bool (*)(int, int)>), "");
+static_assert(sizeof(Multimap<Cmp&>) == sizeof(Multimap<bool (*)(int, int)>), "");