Page MenuHomePhabricator

[libc++] Implement format_error.
ClosedPublic

Authored by Mordante on Nov 27 2020, 1:10 AM.

Details

Summary

This is the first step at implementing <format>. It adds the <format> header
and implements the format_error. class.

Implemnts parts of:
-P0645 Text Formatting

Diff Detail

Event Timeline

Mordante created this revision.Nov 27 2020, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 1:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante requested review of this revision.Nov 27 2020, 1:10 AM
miscco accepted this revision.Nov 27 2020, 1:25 AM
miscco added a subscriber: miscco.
miscco added inline comments.
libcxx/include/format
232

It pains me to see this in C++20 feature

Mordante planned changes to this revision.Nov 27 2020, 3:11 AM
Mordante marked an inline comment as done.

Thanks for the review!

It seems a build bot fails. I'll investigate this further.

libcxx/include/format
232

I agree.

Mordante updated this revision to Diff 308009.Nov 27 2020, 3:35 AM
Mordante marked an inline comment as done.

Properly guard the code in format.cpp to only be used in C++20 mode.
Update the x86_64 ABI list.
Note the Apple ABI list is still out of date.

To fix Apple back-deployment build, you'll need to mark your test with sth like:

// This test requires the dylib support introduced in D92214.
// XFAIL: with_system_cxx_lib=macosx10.15
// XFAIL: with_system_cxx_lib=macosx10.14
// XFAIL: with_system_cxx_lib=macosx10.13
// XFAIL: with_system_cxx_lib=macosx10.12
// XFAIL: with_system_cxx_lib=macosx10.11
// XFAIL: with_system_cxx_lib=macosx10.10
// XFAIL: with_system_cxx_lib=macosx10.9

because this build uses a released dylibs for the given version of macOS (here 10.9) and does not use whatever changes you put in src/.
So, in your case, the dtor nor the vtable of format_error is just not in the libc++.dylib that the tests use.
Hence the error message: dyld: Symbol not found: __ZTVNSt3__112format_errorE.

To fix Apple back-deployment build, you'll need to mark your test with sth like:

Thanks for the hint!

Mordante updated this revision to Diff 308155.Nov 28 2020, 5:04 AM

Disable the Apple unit tests as suggested by @curdeius.
Formatted the manually written code with the .clang-format settings as proposed in D92229.

ldionne requested changes to this revision.Dec 1 2020, 2:28 PM

Looks okay except for the synopsis nitpick, however as you say the Apple ABI list needs to be updated. If you rebase on top of master and re-submit, the failing jobs should tell you what symbols you're missing (in the logs). You could add them to the Apple ABI lists (please make sure to sort :-), and the CI should pass.

libcxx/include/format
17

The synopsis should only contain the parts that we implement. Or at least it's always been my impression.

This revision now requires changes to proceed.Dec 1 2020, 2:28 PM
Mordante updated this revision to Diff 308991.Dec 2 2020, 9:11 AM

Rebase to trigger CI to generate the Apple ABI list.

Mordante planned changes to this revision.Dec 2 2020, 9:12 AM
Mordante marked an inline comment as done.Dec 2 2020, 10:48 AM

Thanks for the review.

Looks okay except for the synopsis nitpick, however as you say the Apple ABI list needs to be updated. If you rebase on top of master and re-submit, the failing jobs should tell you what symbols you're missing (in the logs). You could add them to the Apple ABI lists (please make sure to sort :-), and the CI should pass.

@curdeius already committed the Buildkite ABI list improvements. So I copied the Apple ABI list artifact of the failed build :-) A lot easier than looking in the logs ;-)

libcxx/include/format
17

I'll update it in the next version.

Mordante updated this revision to Diff 309010.Dec 2 2020, 10:55 AM
Mordante marked an inline comment as done.

Updated the Apple ABI list and synopsis.

curdeius requested changes to this revision.Dec 2 2020, 12:04 PM
curdeius added inline comments.
libcxx/test/libcxx/double_include.sh.cpp
96

I almost forgot. Please have a look at the updated notes. There are two more places to add a new header.
https://libcxx.llvm.org/docs/Contributing.html#id3

This revision now requires changes to proceed.Dec 2 2020, 12:04 PM
Mordante marked an inline comment as done.Dec 3 2020, 9:31 AM
Mordante added inline comments.
libcxx/test/libcxx/double_include.sh.cpp
96

Thanks! I'll update them in the next release. I'll also properly move the format header to keep them sorted.

Mordante updated this revision to Diff 309285.Dec 3 2020, 9:34 AM
Mordante marked an inline comment as done.

Implement all new header tests as suggested by @curdeius.

@ldionne Can you have a look?

ldionne requested changes to this revision.Jan 19 2021, 8:08 AM
ldionne added inline comments.
libcxx/include/format
17

Ping on this. We should remove the part of the synopsis that we don't implement (and of course we should re-add it as we implement more parts of it).

libcxx/src/format.cpp
14

We muse always compile this into the library, so there should be no guard. The built library must be able to work with headers in any standard mode, which means it needs to contain the required definitions for all standard modes. Furthermore, this is always true anyway since we compile the library with C++20.

This revision now requires changes to proceed.Jan 19 2021, 8:08 AM
Mordante marked 2 inline comments as done.Jan 22 2021, 8:44 AM
Mordante added inline comments.
libcxx/include/format
17

I thought I had done this, but apparently not :-( I've removed it now.

libcxx/src/format.cpp
14

Thanks for the explanation. I wasn't aware the library should always be compiled with the latest C++ version.

Mordante updated this revision to Diff 318538.Jan 22 2021, 8:48 AM
Mordante marked 2 inline comments as done.
  • Addresses review comments:
    • Removed unimplemented part of the synopsis
    • Removed cpp version check in format.cpp
  • Replace c++ version number 2a with 20 where appropriate. This reflects changes done in main by unrelated commits.
  • Rebased to trigger CI again.
Mordante updated this revision to Diff 318744.Jan 23 2021, 4:12 AM

Rebase to trigger CI. The last build failed, but so did main.

ldionne accepted this revision.Jan 28 2021, 8:19 AM

Thanks! The CI has been a bit grumpy the last days, but hopefully it should be back to normal now.

This revision is now accepted and ready to land.Jan 28 2021, 8:19 AM
This revision was automatically updated to reflect the committed changes.

Hello, this again breaks building with -std=c++17: the cpp file has the definition but not the declaration in that case. Should libc++ sources be built with -std=c++20? Or should the cpp file also check _LIBCPP_STD_VER?

To answer my question: Yes, C++20 should be used, see 3b625060fc915.

... but it looks like CXX_STANDARD_REQUIRED should now become YES after this change here, right?

Hello, this again breaks building with -std=c++17: the cpp file has the definition but not the declaration in that case. Should libc++ sources be built with -std=c++20? Or should the cpp file also check _LIBCPP_STD_VER?

I had this in my original version, but I removed it after review this review comment
https://reviews.llvm.org/D92214#inline-887948

To answer my question: Yes, C++20 should be used, see 3b625060fc915.

That doesn't really answer the question since that made C++20 preferred but not mandatory. That was the intention.

@ldionne Can you clarify the status of libc++'s C++20 requirements. If optional should I re-add the version check macro's in the .cpp file?

dmajor added a subscriber: dmajor.Thu, Jan 28, 11:55 AM

Hello, this again breaks building with -std=c++17: the cpp file has the definition but not the declaration in that case. Should libc++ sources be built with -std=c++20? Or should the cpp file also check _LIBCPP_STD_VER?

I had this in my original version, but I removed it after review this review comment
https://reviews.llvm.org/D92214#inline-887948

To answer my question: Yes, C++20 should be used, see 3b625060fc915.

That doesn't really answer the question since that made C++20 preferred but not mandatory. That was the intention.

@ldionne Can you clarify the status of libc++'s C++20 requirements. If optional should I re-add the version check macro's in the .cpp file?

Ugh. That's tough.

First, we can't always build as C++17 because then the dylib can't provide the features it needs to support c++20 (unless we jump through some hoops to compile with C++17 but enable stuff like char8_t, which is a no-no).
Second, allowing the library to be built either as C++17 or C++20, but provide different features depending on that, is also a pretty bad idea because then someone could link against that library, try to use some C++20 feature from the headers, and they would get a linker error. That's bad too. That's the current state of things with char8_t support AFAICT (@curdeius can confirm or infirm).
Third, people are going to be grumpy if we bump the requirement of building libc++ to C++20. I personally think that's reasonable, but this is obviously causing some trouble since @thakis is here. @thakis, can you explain what's the issue with building libc++ with C++20? I assume Chrome is using an older compiler that doesn't support c++20 yet?

Second, allowing the library to be built either as C++17 or C++20, but provide different features depending on that, is also a pretty bad idea because then someone could link against that library, try to use some C++20 feature from the headers, and they would get a linker error. That's bad too. That's the current state of things with char8_t support AFAICT (@curdeius can confirm or infirm).

Indeed, not using C++20 to compile the library would prevent us from implementing any C++20 features, char8_t support is one example, format_error is another.

Just my 2 cents.
Personally, as a sort of transition before requiring C++20 to compile libc++, I'd go for allowing the build in both C++17 and C++20.
BTW, there's already some amount of #if _LIBCPP_STD_VER > 11 and #ifndef _LIBCPP_NO_HAS_CHAR8_T checks in src/.
So people already can shoot themselves in a foot compiling and linking in different modes.
I'd argue that compiling the library in C++17 and using it in C++20 (and using these C++20 features) is silly.
I imagine it would be to much of a pain to guarantee that it works the other way round (compiling in C++20, using in C++17), but it should work in most of the cases.

Anyway, guarding C++20 features in src/ so as to allow the build and use in C++17 would probably be the way to go for the moment.
I know it's a maintenance burden though.

I agree it would be nice to allow C++17 and C++20 for a limited time. It would be a bit of a maintenance burden, but I feel it benefits our users. I think we can state users are allowed to use C++17, but it's not officially supported. We could make a Cmake switch to opt-in to this behavior, but I feel that would be overkill.

I think once C++20 becomes mandatory we should remove the #if _LIBCPP_STD_VER > 17 from the files in src. Then the extra burden is only there for a limited time.

I'll make a patch to remove the 2 occurrences of #if _LIBCPP_STD_VER > 11 from src.

I'd argue that compiling the library in C++17 and using it in C++20 (and using these C++20 features) is silly.

Yes, I agree.

I imagine it would be to much of a pain to guarantee that it works the other way round (compiling in C++20, using in C++17), but it should work in most of the cases.

No, that's actually what we do all the time. We build the dylib in C++20, but users are free to use the headers in C++03, C++11, C++14, C++17 or C++20 mode.

@Mordante Feel free to add back the check. However, in the short/mid term, I want the default way to build libc++ to switch to the bootstrap build (i.e. the Runtimes build). Once that happens, we will be guaranteed that we always build the dylib with basically Clang trunk (or a recent compiler anyway). It won't make sense anymore to allow anything but the latest standard then. It is indeed a maintenance burden to support more than that, and a burden that (I claim) doesn't provide much benefit. If you're compiling the C++ Standard Library from scratch, I think you have the time and dedication to keep your compiler up to date.

I imagine it would be to much of a pain to guarantee that it works the other way round (compiling in C++20, using in C++17), but it should work in most of the cases.

No, that's actually what we do all the time. We build the dylib in C++20, but users are free to use the headers in C++03, C++11, C++14, C++17 or C++20 mode.

Yes, right. A brain fart from my side.

@Mordante Feel free to add back the check.

I'll commit a fix.

However, in the short/mid term, I want the default way to build libc++ to switch to the bootstrap build (i.e. the Runtimes build). Once that happens, we will be guaranteed that we always build the dylib with basically Clang trunk (or a recent compiler anyway). It won't make sense anymore to allow anything but the latest standard then. It is indeed a maintenance burden to support more than that, and a burden that (I claim) doesn't provide much benefit. If you're compiling the C++ Standard Library from scratch, I think you have the time and dedication to keep your compiler up to date.

Sounds good to me.