[libcxx-commits] [libcxx] [libc++] Fix unintended ABI break in associative containers with reference comparators (PR #118685)
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Dec 4 14:04:40 PST 2024
https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/118685
>From e7d3f31effdf5b49f4fe69e9d005f6ea4c7c2f50 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 4 Dec 2024 13:55:43 -0500
Subject: [PATCH 1/2] [libc++] Fix unintended ABI break in associative
containers with reference comparators
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
---
libcxx/include/__memory/compressed_pair.h | 15 ++++-
.../reference_comparator_abi.compile.pass.cpp | 57 +++++++++++++++++++
2 files changed, 70 insertions(+), 2 deletions(-)
create mode 100644 libcxx/test/libcxx/containers/associative/reference_comparator_abi.compile.pass.cpp
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)>), "");
>From 4eaa6c5ecb678790a8bd0f7430ff1823af5be5e1 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 4 Dec 2024 17:04:16 -0500
Subject: [PATCH 2/2] Fix comment
---
libcxx/include/__memory/compressed_pair.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/libcxx/include/__memory/compressed_pair.h b/libcxx/include/__memory/compressed_pair.h
index 7687615bd00195..38798a21fa3c9c 100644
--- a/libcxx/include/__memory/compressed_pair.h
+++ b/libcxx/include/__memory/compressed_pair.h
@@ -52,15 +52,16 @@ _LIBCPP_BEGIN_NAMESPACE_STD
// 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.
+// 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)> {};
+inline const size_t __compressed_pair_alignment = _LIBCPP_ALIGNOF(_Tp);
template <class _Tp>
-struct __compressed_pair_alignment<_Tp&> : integral_constant<size_t, _LIBCPP_ALIGNOF(void*)> {};
+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) ||
@@ -73,16 +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__(::std::__compressed_pair_alignment<T2>::value))) 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__(::std::__compressed_pair_alignment<T2>::value), \
- __aligned__(::std::__compressed_pair_alignment<T3>::value))) 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__, _); \
More information about the libcxx-commits
mailing list