From 77ac36547a2d15dc465d723179ce7047a59583ce Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Wed, 26 Apr 2023 16:35:13 -0400 Subject: [PATCH] [libc++] Fix ODR violation with placeholders In D145589, we made the std::bind placeholders inline constexpr to satisfy C++17. It turns out that this causes ODR violations since the shared library provides strong definitions for those placeholders, and the linker on Windows actually complains about this. Fortunately, C++17 only encourages implementations to use `inline constexpr`, it doesn't force them. So instead, we unconditionally define the placeholders as `extern const`, which avoids the ODR violation and is indistinguishable from `inline constexpr` for most purposes, since the placeholders are empty types anyway. Note that we could also go back to the pre-D145589 state of defining them as non-inline constexpr variables in C++17, however that is definitely non-conforming since that means the placeholders have different addresses in different TUs. This is all a bit pedantic, but all in all I feel that `extern const` provides the best bang for our buck, and I can't really find any downsides to that solution. Differential Revision: https://reviews.llvm.org/D149292 --- libcxx/include/__functional/bind.h | 32 ++--- .../func.bind.place/placeholders.pass.cpp | 129 +++++++++------------ 2 files changed, 65 insertions(+), 96 deletions(-) diff --git a/libcxx/include/__functional/bind.h b/libcxx/include/__functional/bind.h index 8e5d598..790111c 100644 --- a/libcxx/include/__functional/bind.h +++ b/libcxx/include/__functional/bind.h @@ -51,7 +51,14 @@ namespace placeholders template struct __ph {}; -#if defined(_LIBCPP_CXX03_LANG) || defined(_LIBCPP_BUILDING_LIBRARY) +// C++17 recommends that we implement placeholders as `inline constexpr`, but allows +// implementing them as `extern `. Libc++ implements them as +// `extern const` in all standard modes to avoid an ABI break in C++03: making them +// `inline constexpr` requires removing their definition in the shared library to +// avoid ODR violations, which is an ABI break. +// +// In practice, since placeholders are empty, `extern const` is almost impossible +// to distinguish from `inline constexpr` from a usage stand point. _LIBCPP_FUNC_VIS extern const __ph<1> _1; _LIBCPP_FUNC_VIS extern const __ph<2> _2; _LIBCPP_FUNC_VIS extern const __ph<3> _3; @@ -62,29 +69,6 @@ _LIBCPP_FUNC_VIS extern const __ph<7> _7; _LIBCPP_FUNC_VIS extern const __ph<8> _8; _LIBCPP_FUNC_VIS extern const __ph<9> _9; _LIBCPP_FUNC_VIS extern const __ph<10> _10; -#elif _LIBCPP_STD_VER >= 17 -inline constexpr __ph<1> _1{}; -inline constexpr __ph<2> _2{}; -inline constexpr __ph<3> _3{}; -inline constexpr __ph<4> _4{}; -inline constexpr __ph<5> _5{}; -inline constexpr __ph<6> _6{}; -inline constexpr __ph<7> _7{}; -inline constexpr __ph<8> _8{}; -inline constexpr __ph<9> _9{}; -inline constexpr __ph<10> _10{}; -#else -constexpr __ph<1> _1{}; -constexpr __ph<2> _2{}; -constexpr __ph<3> _3{}; -constexpr __ph<4> _4{}; -constexpr __ph<5> _5{}; -constexpr __ph<6> _6{}; -constexpr __ph<7> _7{}; -constexpr __ph<8> _8{}; -constexpr __ph<9> _9{}; -constexpr __ph<10> _10{}; -#endif // defined(_LIBCPP_CXX03_LANG) || defined(_LIBCPP_BUILDING_LIBRARY) } // namespace placeholders diff --git a/libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp b/libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp index b71aae8..ac40869 100644 --- a/libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp +++ b/libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp @@ -8,9 +8,25 @@ // -// placeholders -// The placeholders are constexpr in C++17 and beyond. -// libc++ provides constexpr placeholders in C++11 and beyond. +// namespace placeholders { +// // M is the implementation-defined number of placeholders +// extern unspecified _1; +// extern unspecified _2; +// . +// . +// . +// extern unspecified _Mp; +// } + +// The Standard recommends implementing them as `inline constexpr` in C++17. +// +// Libc++ implements the placeholders as `extern const` in all standard modes +// to avoid an ABI break in C++03: making them `inline constexpr` requires removing +// their definition in the shared library to avoid ODR violations, which is an +// ABI break. +// +// Concretely, `extern const` is almost indistinguishable from constexpr for the +// placeholders since they are empty types. #include #include @@ -18,81 +34,50 @@ #include "test_macros.h" template -void -test(const T& t) -{ - // Test default constructible. - T t2; - ((void)t2); - // Test copy constructible. - T t3 = t; - ((void)t3); +TEST_CONSTEXPR_CXX17 void test(const T& t) { + // Test default constructible. + { + T x; (void)x; + } + + // Test copy constructible. + { + T x = t; (void)x; static_assert(std::is_nothrow_copy_constructible::value, ""); static_assert(std::is_nothrow_move_constructible::value, ""); -} + } -#if TEST_STD_VER >= 11 -constexpr decltype(std::placeholders::_1) default1{}; -constexpr decltype(std::placeholders::_2) default2{}; -constexpr decltype(std::placeholders::_3) default3{}; -constexpr decltype(std::placeholders::_4) default4{}; -constexpr decltype(std::placeholders::_5) default5{}; -constexpr decltype(std::placeholders::_6) default6{}; -constexpr decltype(std::placeholders::_7) default7{}; -constexpr decltype(std::placeholders::_8) default8{}; -constexpr decltype(std::placeholders::_9) default9{}; -constexpr decltype(std::placeholders::_10) default10{}; - -constexpr decltype(std::placeholders::_1) cp1 = std::placeholders::_1; -constexpr decltype(std::placeholders::_2) cp2 = std::placeholders::_2; -constexpr decltype(std::placeholders::_3) cp3 = std::placeholders::_3; -constexpr decltype(std::placeholders::_4) cp4 = std::placeholders::_4; -constexpr decltype(std::placeholders::_5) cp5 = std::placeholders::_5; -constexpr decltype(std::placeholders::_6) cp6 = std::placeholders::_6; -constexpr decltype(std::placeholders::_7) cp7 = std::placeholders::_7; -constexpr decltype(std::placeholders::_8) cp8 = std::placeholders::_8; -constexpr decltype(std::placeholders::_9) cp9 = std::placeholders::_9; -constexpr decltype(std::placeholders::_10) cp10 = std::placeholders::_10; -#endif // TEST_STD_VER >= 11 - -void use_placeholders_to_prevent_unused_warning() { -#if TEST_STD_VER >= 11 - ((void)cp1); - ((void)cp2); - ((void)cp3); - ((void)cp4); - ((void)cp5); - ((void)cp6); - ((void)cp7); - ((void)cp8); - ((void)cp9); - ((void)cp10); - ((void)default1); - ((void)default2); - ((void)default3); - ((void)default4); - ((void)default5); - ((void)default6); - ((void)default7); - ((void)default8); - ((void)default9); - ((void)default10); + // It is implementation-defined whether placeholder types are CopyAssignable. + // CopyAssignable placeholders' copy assignment operators shall not throw exceptions. +#ifdef _LIBCPP_VERSION + { + T x; + x = t; + static_assert(std::is_nothrow_copy_assignable::value, ""); + static_assert(std::is_nothrow_move_assignable::value, ""); + } #endif } -int main(int, char**) -{ - use_placeholders_to_prevent_unused_warning(); - test(std::placeholders::_1); - test(std::placeholders::_2); - test(std::placeholders::_3); - test(std::placeholders::_4); - test(std::placeholders::_5); - test(std::placeholders::_6); - test(std::placeholders::_7); - test(std::placeholders::_8); - test(std::placeholders::_9); - test(std::placeholders::_10); +TEST_CONSTEXPR_CXX17 bool test_all() { + test(std::placeholders::_1); + test(std::placeholders::_2); + test(std::placeholders::_3); + test(std::placeholders::_4); + test(std::placeholders::_5); + test(std::placeholders::_6); + test(std::placeholders::_7); + test(std::placeholders::_8); + test(std::placeholders::_9); + test(std::placeholders::_10); + return true; +} + +int main(int, char**) { + test_all(); +#if TEST_STD_VER >= 17 + static_assert(test_all(), ""); +#endif return 0; } -- 2.7.4