[libcxx-commits] [libcxx] 77ac365 - [libc++] Fix ODR violation with placeholders
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 27 07:57:28 PDT 2023
Author: Louis Dionne
Date: 2023-04-27T10:57:15-04:00
New Revision: 77ac36547a2d15dc465d723179ce7047a59583ce
URL: https://github.com/llvm/llvm-project/commit/77ac36547a2d15dc465d723179ce7047a59583ce
DIFF: https://github.com/llvm/llvm-project/commit/77ac36547a2d15dc465d723179ce7047a59583ce.diff
LOG: [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
Added:
Modified:
libcxx/include/__functional/bind.h
libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/include/__functional/bind.h b/libcxx/include/__functional/bind.h
index 8e5d598ad257b..790111c5b37fa 100644
--- a/libcxx/include/__functional/bind.h
+++ b/libcxx/include/__functional/bind.h
@@ -51,7 +51,14 @@ namespace placeholders
template <int _Np> 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 <implementation-defined>`. 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 b71aae818a062..ac408699434a4 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 @@
// <functional>
-// 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 <functional>
#include <type_traits>
@@ -18,81 +34,50 @@
#include "test_macros.h"
template <class T>
-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<T>::value, "");
static_assert(std::is_nothrow_move_constructible<T>::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<T>::value, "");
+ static_assert(std::is_nothrow_move_assignable<T>::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;
}
More information about the libcxx-commits
mailing list