[libc++] Fix unintended ABI break in associative containers with reference comparators

[ldionne]

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

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/118685.diff

2 Files Affected:

• (modified) libcxx/include/__memory/compressed_pair.h (+13-2)
• (added) libcxx/test/libcxx/containers/associative/reference_comparator_abi.compile.pass.cpp (+57)

diff --git a/libcxx/include/__memory/compressed_pair.h b/libcxx/include/__memory/compressed_pair.h
index a7acaaff9da099..7687615bd00195 100644
--- a/libcxx/include/__memory/compressed_pair.h
+++ b/libcxx/include/__memory/compressed_pair.h
@@ -50,9 +50,18 @@ _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>
+struct __compressed_pair_alignment : integral_constant<size_t, _LIBCPP_ALIGNOF(_Tp)> {};
+
+template <class _Tp>
+struct __compressed_pair_alignment<_Tp&> : integral_constant<size_t, _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 +73,16 @@ 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>::value))) 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>::value),                                         \
+                   __aligned__(::std::__compressed_pair_alignment<T3>::value))) 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)>), "");

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[philnik777]

I didn't have the time to look at this closely yet, so thanks for taking it on!