[libc++] Fix ODR violation with placeholders

[ldionne]

Summary

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.

[ldionne]

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp

56–57

This is added test coverage.

[philnik]

IMO it would be nice to have them inline constexpr at some point, but I'm fine with this as a bug fix. Anything else should be evaluated separately.

libcxx/include/__functional/bind.h

 inline constexpr int is_placeholder_v = is_placeholder<_Tp>::value;
 #endif
 
 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;
 _LIBCPP_FUNC_VIS extern const __ph<4>   _4;
 _LIBCPP_FUNC_VIS extern const __ph<5>   _5;
 _LIBCPP_FUNC_VIS extern const __ph<6>   _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
 
 template<int _Np>
 struct is_placeholder<placeholders::__ph<_Np> >
     : public integral_constant<int, _Np> {};
 
 

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.place/placeholders.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // <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>
 
 #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) {
+  {
+    T x; (void)x;
+  }
+
+  {
+    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() {
+  return true;
+}
+
+int main(int, char**) {
+  test_all();
+#if TEST_STD_VER >= 17
+  static_assert(test_all(), "");
+#endif
 
   return 0;
 }