[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

diff --git a/libcxx/include/__functional/bind.h b/libcxx/include/__functional/bind.h
index 8e5d598..790111c 100644 (file)
--- 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 b71aae8..ac40869 100644 (file)
--- 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;
 }