[libc++] Make std::allocator_arg and friends conforming in C++17

[ldionne]

Summary

This patch makes global tag variables like std::allocator_arg
conform to C++17 by defining them as inline constexpr variables.
This is possible without creating an ODR violation now that we don't
define strong definitions of those variables in the shared library
anymore.

[Mordante]

Thanks for working on this! This makes helps to enable modules in libc++.

LGTM, but I really would like to use a helper macro over #ifdefs.

libcxx/include/__functional/bind.h

66

I thought you had a marco _LIBCPP_INLINE_CONSTEXPR_VAR in a different patch, that could be sued here too.

[philnik]

In D145589#4178824, @Mordante wrote:

LGTM, but I really would like to use a helper macro over #ifdefs.

Another option would be to just always mark them inline. Both clang and GCC support this as an extension.

[Mordante]

In D145589#4178836, @philnik wrote:

In D145589#4178824, @Mordante wrote:

LGTM, but I really would like to use a helper macro over #ifdefs.

Another option would be to just always mark them inline. Both clang and GCC support this as an extension.

Really? I wasn't aware of that. In that case +1 for that solution.

[philnik]

In D145589#4178843, @Mordante wrote:

In D145589#4178836, @philnik wrote:

In D145589#4178824, @Mordante wrote:

LGTM, but I really would like to use a helper macro over #ifdefs.

Another option would be to just always mark them inline. Both clang and GCC support this as an extension.

Really? I wasn't aware of that. In that case +1 for that solution.

I think the main problem with that approach is that it will be hard to only use extensions in the libc++ headers and not in the tests. We could probably add some magic into _LIBCPP_BEGIN_NAMESPACE_STD and _LIBCPP_END_NAMESPACE_STD, since we have 99% of our code in there, but Clang and GCC support different extension in some cases, making it potentially quite annoying to find out which ones are supported.

[ldionne]

libcxx/include/__functional/bind.h

66

The reason why I didn't go for that is that the specification changed from non-inline variables to inline variables.

Using something like _LIBCPP_INLINE_CONSTEXPR_VAR makes it look as-if we're trying to get inline whenever we can, which is not really the intent. LMK if that alleviates your concerns, otherwise I can introduce a macro.

[ldionne]

In D145589#4178836, @philnik wrote:

In D145589#4178824, @Mordante wrote:

LGTM, but I really would like to use a helper macro over #ifdefs.

Another option would be to just always mark them inline. Both clang and GCC support this as an extension.

C++11 and C++14 didn't mark those as inline, so I'm not sure we should (even though I agree having them non-inline makes little sense).

[ldionne]

Rebase.

[ldionne]

Fix rebase issue.

[philnik]

In D145589#4281652, @ldionne wrote:

In D145589#4178836, @philnik wrote:

In D145589#4178824, @Mordante wrote:

LGTM, but I really would like to use a helper macro over #ifdefs.

Another option would be to just always mark them inline. Both clang and GCC support this as an extension.

C++11 and C++14 didn't mark those as inline, so I'm not sure we should (even though I agree having them non-inline makes little sense).

I don't think this will be an issue in reality. I don't think any program observes the pointer of these values in a meaningful way, so this will at most be a QoI issue in C++11 and C++14, more likely just fewer #ifdefs in our code.

[ldionne]

Make inline as an extension

[ldionne]

Rebase for CI

[ldionne]

Revert to previous behavior: don't use inline before C++17.

It turns out this is flagged as a C++17 extension by Clang and breaks our test suite. We need to finish the discussion on using extensions in our code before we do that, and if we decide to do it (I think we should), then we'll need additional CI to make sure e.g. GCC is happy in standard modes like C++17.

[ldionne]

This passed, see https://buildkite.com/llvm-project/libcxx-ci/builds/22706.

[thakis]

Early heads-up: This breaks some links in our windows component-build (ie. one shared library per larger library) debug bots – https://bugs.chromium.org/p/chromium/issues/detail?id=1440130

Not yet clear if that's due to us doing something silly or this not being compatible with Windows. Will post again once we've figured it out.

[hans]

We're seeing build failures in Chromium: https://crbug.com/1440126
It's not clear why it's only in some builds, but the linker complains about duplicate definitions of placeholders::__ph<1>: both from libc++'s bind.cpp and from some Chromium file which presumably includes __functional/bind.h.

The commit message says "now that we don't define strong definitions of those variables in the shared library anymore." but it seems we do have strong definitions for these in bind.cpp still, or am I missing something?

It looks like D145587 removed the strong definitions of allocator_arg etc. but it did not touch the placeholder variables. Was that intentional, or is the fix here to just remove bind.cpp?

[thakis]

Some more notes:

• We only see this on a bot that does an unoptimized static library build (I said component above, but while the bot where it happens generally does a component build, the failing target is in a sub-toolchain that does static builds)
• The root cause is that on Windows, having the same symbol be inline in some places but strong in others causes duplicate symbol errors; standalone demo (which you can even run on a mac): https://bugs.chromium.org/p/chromium/issues/detail?id=1440126#c7

[ldionne]

Thanks for the heads up and the analysis here folks. See D149292 and the discussion starting at https://discord.com/channels/636084430946959380/636732894974312448/1100867027331199117

libcxx/include/__functional/bind.h

 _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
-/* 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{};
+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/include/__memory/allocator_arg_t.h

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 struct _LIBCPP_TEMPLATE_VIS allocator_arg_t { explicit allocator_arg_t() = default; };
 
-#ifndef _LIBCPP_CXX03_LANG
+#if _LIBCPP_STD_VER >= 17
+inline constexpr allocator_arg_t allocator_arg = allocator_arg_t();
+#elif !defined(_LIBCPP_CXX03_LANG)
+constexpr allocator_arg_t allocator_arg = allocator_arg_t();
+#endif
 
-/* inline */ constexpr allocator_arg_t allocator_arg = allocator_arg_t();
+#ifndef _LIBCPP_CXX03_LANG
 
 // allocator construction
 
 template <class _Tp, class _Alloc, class ..._Args>
 struct __uses_alloc_ctor_imp
 {
     typedef _LIBCPP_NODEBUG __remove_cvref_t<_Alloc> _RawAlloc;
     static const bool __ua = uses_allocator<_Tp, _RawAlloc>::value;

libcxx/include/__mutex/tag_types.h

 struct _LIBCPP_TYPE_VIS try_to_lock_t {
   explicit try_to_lock_t() = default;
 };
 
 struct _LIBCPP_TYPE_VIS adopt_lock_t {
   explicit adopt_lock_t() = default;
 };
 
-#  if !defined(_LIBCPP_CXX03_LANG)
-/* inline */ constexpr defer_lock_t defer_lock   = defer_lock_t();
-/* inline */ constexpr try_to_lock_t try_to_lock = try_to_lock_t();
-/* inline */ constexpr adopt_lock_t adopt_lock   = adopt_lock_t();
+#  if _LIBCPP_STD_VER >= 17
+inline constexpr defer_lock_t defer_lock   = defer_lock_t();
+inline constexpr try_to_lock_t try_to_lock = try_to_lock_t();
+inline constexpr adopt_lock_t adopt_lock   = adopt_lock_t();
+#  elif !defined(_LIBCPP_CXX03_LANG)
+constexpr defer_lock_t defer_lock   = defer_lock_t();
+constexpr try_to_lock_t try_to_lock = try_to_lock_t();
+constexpr adopt_lock_t adopt_lock   = adopt_lock_t();
 #  endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP_HAS_NO_THREADS
 
 #endif // _LIBCPP___MUTEX_TAG_TYPES_H

libcxx/include/__utility/piecewise_construct.h

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 struct _LIBCPP_TEMPLATE_VIS piecewise_construct_t { explicit piecewise_construct_t() = default; };
 
-#if !defined(_LIBCPP_CXX03_LANG)
-/* inline */ constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
+#if _LIBCPP_STD_VER >= 17
+inline constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
+#elif !defined(_LIBCPP_CXX03_LANG)
+constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___UTILITY_PIECEWISE_CONSTRUCT_H