Page MenuHomePhabricator

[libc++][RFC] Disable incomplete library features.
ClosedPublic

Authored by Mordante on Jul 25 2021, 5:57 AM.

Details

Summary

At the moment the implementation of std::format is incomplete and
there will be ABI breaking changes in the future:

  • Some shortcuts were taken to get a working basic version. Resolving these shortcuts will cause ABI breaking changes. For example the storage of the formatting arguments is currently done in a simple fashion leading to inefficient memory usage. Improving this is required by the Standard. However validating the required implementation is easier with the basics of std::format working. At that point there are proper tests to guard against regressions.
  • The C++ committee accepts LWG issues that contain ABI breaks. For example http://wg21.link/P2216.

As long as a Standard isn't ratified libc++ makes no promises on ABI
stability for new features. After a Standard is ratified there's no good
way for libc++ to communicate to its users a feature isn't complete yet.
This is now the case with std::format, but the same may occur in the
future with other 'monolithic' features. For example, when Networking TS
will become part of the Standard.

This is a RFC to see how we can solve this problem. It aims to:

  • Make it possible to develop high quality implementations in main, while not being afraid to make ABI breaking changes to improve the implementation.
  • Make it possible to work on large features in main that can't be completed in one LLVM release cycle.
  • Warn users when using not Standard conforming code.
  • Warn users when using ABI unstable features.

Obviously using this escape hatch isn't ideal and when possible we
should attempt to avoid it.

In theory there's a simple solution. C++ has feature test macros that
allows users to query whether a feature is complete. However it's
uncertain whether users use this macro or just try to compile the code
and if it compiles they expect it to behave properly.

The current implementation in <format> uses #error this has the
advantage that including the header gives a nice diagnostic. For other
headers it might be better to only guard certain features. For example,
when the C++ committee's ABI break only affects a single function it's
possible to only disable this function. In that case there will be no
clear diagnostic how to enable the feature.

Diff Detail

Event Timeline

Mordante requested review of this revision.Jul 25 2021, 5:57 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2021, 5:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I think this is pretty interesting. We could do the same for ranges.

Yes we can do the same for ranges.
I think that partly boils down to what kind of errors we want to give the user, a clear diagnostic explaining how to opt-in to the feature.
Or a less clear message due to a not implemented algorithm.
I know there are some ABI breaking ranges papers retroactively accepted in the June plenary, I don't know whether these ABI breaking changes will affect libc++.

@cjdb @zoecarver is our ranges implementation in main affected by the ABI breaking papers accepted in the June plenary?

Mordante updated this revision to Diff 361651.Jul 26 2021, 7:37 AM

Add a macro for the <ranges> header.
Note still need to adjust the test runners to enable the incomplete features by default.

@cjdb @zoecarver is our ranges implementation in main affected by the ABI breaking papers accepted in the June plenary?

I don't think there's any remaining ABI breaks to be performed because of changes in the spec, however we still want to reserve the right for ABI breaks for a little bit. For example, I'm not sure the ABI for non-propagating-cache is final.

I actually gave more thought to this, and I think what we want is:

  1. Add a macro in __config_site like _LIBCPP_HAS_NO_INCOMPLETE_FEATURES.
  2. Add a CMake option LIBCXX_ENABLE_INCOMPLETE_FEATURES that sets this macro in __config_site. By default, that option should be set to ON so that we enable incomplete features by default.
  3. Add a macro in features.py like: '_LIBCPP_HAS_NO_INCOMPLETE_FEATURES': 'libcpp-has-no-incomplete-features', and mark the relevant tests as UNSUPPORTED.
  4. Set set(LIBCXX_ENABLE_INCOMPLETE_FEATURES OFF CACHE BOOL "") in Apple.cmake.

That way, incomplete features are always enabled by default (i.e. the current state of things). Enabling them does not require any specific action. However, when we want to ship libc++ (as a vendor or as part of the LLVM distribution), we can disable the incomplete features when configuring CMake.

The benefits of doing it that way is that it's better integrated with the rest of the code -- that's how we disable all kinds of other features. Also, it's not clear to me that we want to give users the ability to override this option. It's not like using something in std::experimental, it's really more like using something that isn't fully baked yet.

If you agree and we go in that direction, we should also move the documentation for the option to BuildingLibcxx.rst, which is aimed at vendors instead of end-users.

libcxx/include/ranges
187
Mordante marked an inline comment as done.Jul 27 2021, 2:06 AM

Thanks for your feedback, this is exactly the kind of feedback I hoped to get for this RFC!

I actually gave more thought to this, and I think what we want is:

  1. Add a macro in __config_site like _LIBCPP_HAS_NO_INCOMPLETE_FEATURES.

Initially I went this path, but I wasn't too happy with it. I split it in one macro for format and one macro for ranges. This means there will be libcpp-has-no-incomplete-ranges and libcpp-has-no-incomplete-format. I prefer two different names. Once one feature is completed it's a lot easier to grep whether all occurrences have been removed.

  1. Add a CMake option LIBCXX_ENABLE_INCOMPLETE_FEATURES that sets this macro in __config_site. By default, that option should be set to ON so that we enable incomplete features by default.
  2. Add a macro in features.py like: '_LIBCPP_HAS_NO_INCOMPLETE_FEATURES': 'libcpp-has-no-incomplete-features', and mark the relevant tests as UNSUPPORTED.
  3. Set set(LIBCXX_ENABLE_INCOMPLETE_FEATURES OFF CACHE BOOL "") in Apple.cmake.

I prefer to have some Apple CI bots to override this default. That way we can validate whether the feature would work on Apple. Otherwise when we flip the switch we get all Apple build issues at the same time. I was thinking about using MacOS x86_64 and MacOS arm64. WDYT?

That way, incomplete features are always enabled by default (i.e. the current state of things). Enabling them does not require any specific action. However, when we want to ship libc++ (as a vendor or as part of the LLVM distribution), we can disable the incomplete features when configuring CMake.

The benefits of doing it that way is that it's better integrated with the rest of the code -- that's how we disable all kinds of other features. Also, it's not clear to me that we want to give users the ability to override this option. It's not like using something in std::experimental, it's really more like using something that isn't fully baked yet.

If you agree and we go in that direction, we should also move the documentation for the option to BuildingLibcxx.rst, which is aimed at vendors instead of end-users.

I like this direction, it refines the general direction of the original proposal.

Mordante updated this revision to Diff 361946.Jul 27 2021, 2:10 AM

Addresses the review comments.
The documentation will be done in a follow-up.
Want to get some feedback of the CI first.

Mordante added inline comments.Jul 27 2021, 2:14 AM
libcxx/CMakeLists.txt
127

Update documentation.

libcxx/benchmarks/dummy.cpp
4 ↗(On Diff #361946)

Remove this file after the CI passes.

libcxx/docs/BuildingLibcxx.rst
411

Update documentation.

libcxx/docs/Contributing.rst
73 ↗(On Diff #361946)

Update documentation. Note I might revert this hunk and do it in a separate patch when that means we can get this patch is before branching LLVM 13.

Mordante updated this revision to Diff 361958.Jul 27 2021, 2:50 AM

Temporary disable canceling a build after a failure.
Just to get the status of the other builds.

Mordante updated this revision to Diff 361963.Jul 27 2021, 3:31 AM

Attempt to fix the Apple build.
Remove the dummy benchmark since the build has passed.

  1. Set set(LIBCXX_ENABLE_INCOMPLETE_FEATURES OFF CACHE BOOL "") in Apple.cmake.

I prefer to have some Apple CI bots to override this default. That way we can validate whether the feature would work on Apple. Otherwise when we flip the switch we get all Apple build issues at the same time. I was thinking about using MacOS x86_64 and MacOS arm64. WDYT?

Based on the build failures I got I had a look at the configuration and it actually seems these two nodes already exactly do that.
So you can ignore this comment.

Mordante updated this revision to Diff 361969.Jul 27 2021, 3:53 AM

Attempt to fix all build issues.

Mordante updated this revision to Diff 361977.Jul 27 2021, 4:42 AM

Restore default CI building
Fix build tests
Improve documentation

Mordante planned changes to this revision.Jul 27 2021, 4:44 AM

There're still a few design choices to be evaluated. After deciding they can be implemented and the documentation updated accordingly.

Mordante updated this revision to Diff 362098.Jul 27 2021, 10:49 AM

As clarified on Discord made the feature a build-time only configuration.
The documentation still needs to be updated.

I will update the Buiding documentation after the current build is finished.
I want to update the contributor part for the incomplete features in a separate patch. It makes sense to do that after we're certain about the approach. Normally I wouldn't mind an extra iteration too much, but in this case I prefer to land the real patch as soon as possible so it can be in RC1.

libcxx/docs/BuildingLibcxx.rst
411

@ldionne Is it intended these options are documented? There seem to be quite some missing options. For example LIBCXX_ENABLE_RANDOM_DEVICE and LIBCXX_ENABLE_LOCALIZATION

Mordante updated this revision to Diff 362135.Jul 27 2021, 12:21 PM

Update the documentation.
Rebased against main.

ldionne accepted this revision.Jul 27 2021, 12:40 PM

Thanks a lot for this! That's exactly what I had in mind.

The fact that we changed the Apple.cmake cache means that we already have a tester for this configuration, so no need to add one.

Once the LLVM 13 branch is cut, I'll cherry-pick a change that passes LIBCXX_ENABLE_INCOMPLETE_FEATURES=OFF on the release branch (so that LLVM 13 doesn't ship half-implemented features). In the future, it would be even better to consider the LLVM distribution as being a "vendor" and have a "vendor specific" cache for it (that would be used when building LLVM for releasing). We'd set LIBCXX_ENABLE_INCOMPLETE_FEATURES=OFF in that cache.

libcxx/docs/BuildingLibcxx.rst
411

Oh yeah, the documentation is not up to date in that respect. I'm actually thinking that it's quite bad that there's so much duplication (here and in the CMakeLists.txt). Since this is aimed at vendors, perhaps we could have a single copy of those options in the CMakeLists.txt and have the documentation point to that? If we grouped configure-time options together, it would make them discoverable. Just a thought.

libcxx/include/format
64

Maybe this should say something like The Format library is not complete and stable yet, so this configuration of libc++ does not allow its usage.. Or anything else that explains a bit _why_ it's not supported (otherwise users tend to be angry).

This revision is now accepted and ready to land.Jul 27 2021, 12:40 PM
Mordante marked 2 inline comments as done.Jul 27 2021, 12:51 PM
Mordante added inline comments.
libcxx/docs/BuildingLibcxx.rst
411

I agree it would be nice if there was an automatic way to convert the CMake options to restructured text.

libcxx/include/format
64

I'm not objecting against making the text a bit more verbose. I just looked at what similar code did. I'll make a separate patch to improve these places.

FWIW, my general reaction is that this patch does more harm (to end-users) than good. But (1) it's above my pay grade, and (2) from the end-user's perspective, Clang 13 won't be any less featureful than Clang 12; it'll just be more self-deprecatingly whiny about its continued lack of features.

Consider that __has_include(<format>) will now return the "wrong" value; it would be better and simpler if you could just figure out a way to stop the build system from installing that header in include/c++/v1/ at all. However, this is not a new problem: e.g., __has_include(<variant>) already has the "wrong" value in C++03/11/14 mode; we definitely have no way to fix that.

libcxx/include/format
64

#error "This configuration of libc++ was built without support for C++20 <format> because its ABI is not yet stable."
and below
#error "This configuration of libc++ was built without support for C++20 <ranges> because its ABI is not yet stable."

Mordante updated this revision to Diff 362163.Jul 27 2021, 1:24 PM
Mordante marked 2 inline comments as done.

Update the error message.

FWIW, my general reaction is that this patch does more harm (to end-users) than good.

How so? If we don't do something like this, we'll let users use incomplete and unstable features, and then we'll break them in the future. I don't see how that's acceptable?

Consider that __has_include(<format>) will now return the "wrong" value; it would be better and simpler if you could just figure out a way to stop the build system from installing that header in include/c++/v1/ at all.

Honestly, using __has_include to check for features is already broken enough as it is, I don't think this makes it any worse. People need to learn that they can't use it that way, period.

This revision was landed with ongoing or failed builds.Jul 27 2021, 1:37 PM
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.Jul 27 2021, 2:07 PM

FWIW, my general reaction is that this patch does more harm (to end-users) than good. But (1) it's above my pay grade, and (2) from the end-user's perspective, Clang 13 won't be any less featureful than Clang 12; it'll just be more self-deprecatingly whiny about its continued lack of features.

It makes it possible for vendors to avoid shipping ABI unstable features to their customers. This gives us the freedom the break the ABI in our <ranges> implementation when that gives us the opportunity to create a better implementation.

Consider that __has_include(<format>) will now return the "wrong" value; it would be better and simpler if you could just figure out a way to stop the build system from installing that header in include/c++/v1/ at all. However, this is not a new problem: e.g., __has_include(<variant>) already has the "wrong" value in C++03/11/14 mode; we definitely have no way to fix that.

IMO __has_include(<variant>) is the wrong tool for the job. If a user wants to to test the status of the headers they can use __has_include(<version>) and when that's available test the appropriate feature-test macro.

libcxx/include/format
64

I read this message after committing.
I settled with a slightly different wording with @ldionne, it now shows which configuration option disabled the header.

Mordante marked an inline comment as done.Jul 30 2021, 12:26 AM

Consider that __has_include(<format>) will now return the "wrong" value; it would be better and simpler if you could just figure out a way to stop the build system from installing that header in include/c++/v1/ at all. However, this is not a new problem: e.g., __has_include(<variant>) already has the "wrong" value in C++03/11/14 mode; we definitely have no way to fix that.

IMO __has_include(<variant>) is the wrong tool for the job. If a user wants to to test the status of the headers they can use __has_include(<version>) and when that's available test the appropriate feature-test macro.

@Quuxplusone It seem http://wg21.link/SD6 recommends this practice. D107134 should allow this usage pattern.