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.
Details
- Reviewers
Mordante - Group Reviewers
Restricted Project - Commits
- rG8643bdd0165c: [libc++] Make std::allocator_arg and friends conforming in C++17
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
Another option would be to just always mark them inline. Both clang and GCC support this as an extension.
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.
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. |
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.
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.
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.
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?
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
Thanks for the heads up and the analysis here folks. See D149292 and the discussion starting at https://discord.com/channels/636084430946959380/636732894974312448/1100867027331199117
I thought you had a marco _LIBCPP_INLINE_CONSTEXPR_VAR in a different patch, that could be sued here too.