From e7d3f31effdf5b49f4fe69e9d005f6ea4c7c2f50 Mon Sep 17 00:00:00 2001 From: Louis Dionne 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 a7acaaff9da09..7687615bd0019 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 +struct __compressed_pair_alignment : integral_constant {}; + +template +struct __compressed_pair_alignment<_Tp&> : integral_constant {}; + template ::value && !__libcpp_is_final<_ToPad>::value) || is_reference<_ToPad>::value || sizeof(_ToPad) == __datasizeof_v<_ToPad>)> @@ -64,14 +73,16 @@ template 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::value))) T1 Initializer1; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \ _LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _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::value), \ + __aligned__(::std::__compressed_pair_alignment::value))) T1 Initializer1; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \ _LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _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 0000000000000..f364fc817c164 --- /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 +#include + +#include "test_macros.h" + +struct TEST_ALIGNAS(16) Cmp { + bool operator()(int, int) const; +}; + +template +struct Set { + char b; + std::set s; +}; + +template +struct Multiset { + char b; + std::multiset s; +}; + +template +struct Map { + char b; + std::map s; +}; + +template +struct Multimap { + char b; + std::multimap s; +}; + +static_assert(sizeof(Set) == sizeof(Set), ""); +static_assert(sizeof(Multiset) == sizeof(Multiset), ""); + +static_assert(sizeof(Map) == sizeof(Map), ""); +static_assert(sizeof(Multimap) == sizeof(Multimap), ""); From 4eaa6c5ecb678790a8bd0f7430ff1823af5be5e1 Mon Sep 17 00:00:00 2001 From: Louis Dionne 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 7687615bd0019..38798a21fa3c9 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 -struct __compressed_pair_alignment : integral_constant {}; +inline const size_t __compressed_pair_alignment = _LIBCPP_ALIGNOF(_Tp); template -struct __compressed_pair_alignment<_Tp&> : integral_constant {}; +inline const size_t __compressed_pair_alignment<_Tp&> = _LIBCPP_ALIGNOF(void*); template ::value && !__libcpp_is_final<_ToPad>::value) || @@ -73,16 +74,15 @@ template class __compressed_pair_padding<_ToPad, true> {}; # define _LIBCPP_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2) \ - _LIBCPP_NO_UNIQUE_ADDRESS \ - __attribute__((__aligned__(::std::__compressed_pair_alignment::value))) T1 Initializer1; \ + _LIBCPP_NO_UNIQUE_ADDRESS __attribute__((__aligned__(::std::__compressed_pair_alignment))) T1 Initializer1; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \ _LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _LIBCPP_CONCAT3(__padding2_, __LINE__, _) # define _LIBCPP_COMPRESSED_TRIPLE(T1, Initializer1, T2, Initializer2, T3, Initializer3) \ _LIBCPP_NO_UNIQUE_ADDRESS \ - __attribute__((__aligned__(::std::__compressed_pair_alignment::value), \ - __aligned__(::std::__compressed_pair_alignment::value))) T1 Initializer1; \ + __attribute__((__aligned__(::std::__compressed_pair_alignment), \ + __aligned__(::std::__compressed_pair_alignment))) T1 Initializer1; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \ _LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2; \ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding _LIBCPP_CONCAT3(__padding2_, __LINE__, _); \