This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Mar 8 2023, 8:04 AM.

Details

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.

Diff Detail

Event Timeline

ldionne created this revision.Mar 8 2023, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 8:04 AM
ldionne requested review of this revision.Mar 8 2023, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 8:04 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision.Mar 8 2023, 10:59 AM
Mordante added a subscriber: 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.

This revision is now accepted and ready to land.Mar 8 2023, 10:59 AM

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.

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.

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 added inline comments.Apr 19 2023, 2:29 PM
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.

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 updated this revision to Diff 515386.Apr 20 2023, 10:41 AM

Fix rebase issue.

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 updated this revision to Diff 515686.Apr 21 2023, 5:07 AM

Make inline as an extension

ldionne updated this revision to Diff 515693.Apr 21 2023, 5:38 AM

Rebase for CI

ldionne updated this revision to Diff 515839.Apr 21 2023, 11:03 AM

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.

thakis added a subscriber: thakis.Apr 26 2023, 4:55 AM

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 added a subscriber: hans.Apr 26 2023, 5:19 AM

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