Page MenuHomePhabricator

[libc++] [P0482] [C++20] Implement missing bits for codecvt and codecvt_byname.
ClosedPublic

Authored by curdeius on Nov 15 2020, 11:54 PM.

Details

Summary

Add codecvt*<char16_t, char8_t> and codecvt*<char32_t, char8_t>.
Deprecate codecvt<char(16|32)_t, char>.
Enable disabled tests.
Update _LIBCPP_STD_VER to use 20 for C++20. Add _LIBCPP_DEPRECATED_IN_CXX20 macro.

Diff Detail

Event Timeline

curdeius created this revision.Nov 15 2020, 11:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2020, 11:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius requested review of this revision.Nov 15 2020, 11:54 PM
curdeius updated this revision to Diff 305417.Nov 15 2020, 11:58 PM
  • Clean up.
curdeius updated this revision to Diff 305418.Nov 16 2020, 12:05 AM
  • Get rid of unused __char8_t.

Buildkite shows build failures with undefined references.
That's because libc++ (.so, .a) is compiled using -std=c++17 and I used #ifndef _LIBCPP_NO_HAS_CHAR8_T to guard the char8_t-related things.

_LIBCPP_NO_HAS_CHAR8_T is defined as:

#if _LIBCPP_STD_VER <= 17 || !defined(__cpp_char8_t)
#define _LIBCPP_NO_HAS_CHAR8_T
#endif

So, in the failing cases, src/ compiles with C++17 and so without char8_t, but include/ and test/ compile with C++20.

I haven't seen these failures locally before because I was compiling everything in C++20 mode (CXX_STANDARD 20 in libcxx/CMakeLists.txt).

I see 2 possible solutions:

  1. Use CXX_STANDARD 20, but it might be an overkill.
  2. Change guards in locale.cpp to just #ifdef __cpp_char8_t instead of #ifndef _LIBCPP_NO_HAS_CHAR8_T.

WDYT?

ldionne requested changes to this revision.Nov 17 2020, 5:47 AM

Buildkite shows build failures with undefined references.
That's because libc++ (.so, .a) is compiled using -std=c++17 and I used #ifndef _LIBCPP_NO_HAS_CHAR8_T to guard the char8_t-related things.

_LIBCPP_NO_HAS_CHAR8_T is defined as:

#if _LIBCPP_STD_VER <= 17 || !defined(__cpp_char8_t)
#define _LIBCPP_NO_HAS_CHAR8_T
#endif

So, in the failing cases, src/ compiles with C++17 and so without char8_t, but include/ and test/ compile with C++20.

I haven't seen these failures locally before because I was compiling everything in C++20 mode (CXX_STANDARD 20 in libcxx/CMakeLists.txt).

I see 2 possible solutions:

  1. Use CXX_STANDARD 20, but it might be an overkill.

I think that is the right way to go. I think it makes sense to build the library with the latest standard available.

Furthermore, once we have moved people over to the runtimes build and we know that folks are building the library with a recent compiler, we should even be able to assume that the compiler provides char8_t.

libcxx/include/__locale
1254

I think it should be marked as deprecated whenever the standard says it is.

This revision now requires changes to proceed.Nov 17 2020, 5:47 AM

Ok, thank you for your input. I'll update the things as necessary.

curdeius updated this revision to Diff 305999.Nov 18 2020, 1:17 AM
  • Use C++20 for libc++. Should we require standard? Or should we change to CXX_STANDARD_REQUIRED NO?
  • Remove TODOs.
curdeius marked an inline comment as done.Nov 18 2020, 1:19 AM
curdeius planned changes to this revision.Nov 18 2020, 1:45 AM
curdeius updated this revision to Diff 306009.Nov 18 2020, 2:03 AM
  • Disable deprecation warnings in forgotten tests.
curdeius planned changes to this revision.Nov 18 2020, 2:06 AM

I'll create a new revision with only the bump of the C++ standard version, because that provokes some failures it seems.

curdeius updated this revision to Diff 306016.Nov 18 2020, 2:25 AM
  • Fix more deprecated warnings.
curdeius planned changes to this revision.Nov 18 2020, 2:26 AM
curdeius updated this revision to Diff 306061.Nov 18 2020, 5:03 AM

Use C++20 in libc++abi too.

Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 5:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius updated this revision to Diff 306092.EditedNov 18 2020, 6:43 AM
  • Rebase on top of D91691 to fix patch apply
curdeius planned changes to this revision.Nov 18 2020, 9:24 AM
curdeius updated this revision to Diff 307895.Nov 26 2020, 9:12 AM
  • Fix extra allocation for additional facets.
  • Rebase.
ldionne accepted this revision.Nov 26 2020, 10:03 AM

LGTM if CI passes, thanks!

This revision is now accepted and ready to land.Nov 26 2020, 10:03 AM

Does this pass check-cxx-abilist? It needs to. I think you may need to update the ABI lists.

Ok, thanks. I still need to update ABI list and fix macOS backdeployment.

ldionne requested changes to this revision.Nov 26 2020, 12:47 PM

I would suggest you wait for me to submit D92194 and then rebase on top of it. That'll provide you with additional coverage for the ABI lists. My goal with D92194 was specifically to make sure the CI would catch issues with ABI lists in changes like yours :-). Requesting changes so it shows up right in the queue.

This revision now requires changes to proceed.Nov 26 2020, 12:47 PM
curdeius updated this revision to Diff 307973.Nov 27 2020, 12:28 AM
  • Mark test as XFAIL on macosx10.*.
curdeius planned changes to this revision.Nov 27 2020, 12:29 AM

Ok. Will wait for your patch. I just added XFAIL for macosx dylibs.

Ok. Will wait for your patch. I just added XFAIL for macosx dylibs.

Can you confirm that the issues you were seeing on older macOSes are that the tests are now exercising char8_t codecvt, so you're seeing runtime errors since those are not installed in older dylibs? I'm trying to make sure we understand the issue and don't silent something important.

Ok. Will wait for your patch. I just added XFAIL for macosx dylibs.

Can you confirm that the issues you were seeing on older macOSes are that the tests are now exercising char8_t codecvt, so you're seeing runtime errors since those are not installed in older dylibs? I'm trying to make sure we understand the issue and don't silent something important.

Yes, all the 33 test failures in https://buildkite.com/llvm-project/libcxx-ci/builds/578#4979bae1-6f90-4d3a-a17e-9b9981aaedbf
were due to missing symbols, e.g.:

dyld: Symbol not found: __ZNKSt3__17codecvtIDiDu11__mbstate_tE10do_unshiftERS1_PDuS4_RS4_
  Referenced from: /private/tmp/buildkite-agent/builds/libcxx-mbp-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/projects/libcxx/test/std/localization/locale.categories/category.ctype/locale.codecvt.byname/Output/ctor_char32_t_char8_t.pass.cpp.dir/t.tmp.exe
  Expected in: /private/tmp/buildkite-agent/builds/libcxx-mbp-local-1/llvm-project/libcxx-ci/build/x86_64-apple-system-backdeployment-10.9/macos-roots/macOS/libc++/10.9/libc++.1.dylib
curdeius updated this revision to Diff 308169.Nov 28 2020, 8:42 AM
  • [abi] Update ABI list for linux (and changelog).
  • Rebase to include changes from D92194
curdeius updated this revision to Diff 308275.Nov 30 2020, 12:27 AM
  • Fix linux ABI list.
  • Re-generate ABI list (after a failed check) and get them as artifacts, so as to ease the development for contributors not having access to necessary platform.
curdeius updated this revision to Diff 308281.Nov 30 2020, 1:08 AM
  • Fix linux ABI list (again).
  • Add forgotten artifact_paths.

@ldionne, I've modified the run-buildbot script (and added *.abilist to artifact_paths in buildkite-pipeline.yml) so as to run generate-cxx-abilist if check-cxx-abilist failed.
The build will fail anyway but the regenerated ABI lists will be present among the artifacts.
That's for people, like me, that don't have access to one of the platforms on which we check ABI lists.
If you prefer, I'll spin off another patch to keep things apart.

curdeius updated this revision to Diff 308291.Nov 30 2020, 2:17 AM
  • Fix macOS ABI list.
ldionne accepted this revision.Dec 1 2020, 3:13 PM

Nice! I love how we're building on top of the CI infrastructure to make our life simpler.

If you commit this before me, I'll rebase D92212 on top.

This revision is now accepted and ready to land.Dec 1 2020, 3:13 PM