This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Refactor char_traits
ClosedPublic

Authored by philnik on Dec 7 2022, 9:49 AM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Commits
rG72173469dd02: [libc++] Refactor char_traits
Summary

This allows us to reuse workarounds for compilers that don't provide the builtins or constexpr support.

Diff Detail

Event Timeline

philnik created this revision.Dec 7 2022, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 9:49 AM
philnik requested review of this revision.Dec 7 2022, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 9:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

In general I like the approach, some remarks. I really like to see the CI once it's running again.

libcxx/include/__string/char_traits.h
362

Why is this function and length not changed? Do the builtins not work with them?

libcxx/include/cstring
103

Are these used except in the traits?
I really like to see some private tests for these functions.

philnik marked an inline comment as done.Dec 7 2022, 11:52 AM
philnik added inline comments.
libcxx/include/__string/char_traits.h
362

Yes, __builtin_char_memchr only works with char: https://godbolt.org/z/YvzMdzc1x

libcxx/include/cstring
103

Not currently, but I'm working on a patch that uses __constexpr_memcmp to optimize std{,::ranges}::equal().

philnik updated this revision to Diff 481195.Dec 8 2022, 1:40 AM
philnik marked 2 inline comments as done.

Add tests

philnik updated this revision to Diff 481246.Dec 8 2022, 4:47 AM

Try to fix CI

philnik updated this revision to Diff 481278.Dec 8 2022, 7:57 AM

Next try

ldionne accepted this revision.Dec 8 2022, 8:56 AM
ldionne added inline comments.
libcxx/include/__string/char_traits.h
227

I would make this explicit, and I'd use int instead of int_type since int is what memchr takes.

This revision is now accepted and ready to land.Dec 8 2022, 8:56 AM
Mordante accepted this revision.Dec 8 2022, 9:29 AM

LGTM modulo some nits.

libcxx/include/__string/char_traits.h
215

This is odd, I did leave a message here, but it's gone :-/

How about removing this test? I tested in godbolt and Clang and GCC for -O0 and -O2 this generated no code.
(If wanted I can recreate the example in godbolt.)

libcxx/test/libcxx/strings/c.strings/constexpr.cwchar.compile.pass.cpp
10
philnik updated this revision to Diff 481433.Dec 8 2022, 1:26 PM
philnik marked 3 inline comments as done.
  • Address comments
  • Try to fix CI
libcxx/include/__string/char_traits.h
215

Maybe? I've tried and for me it generates code: https://godbolt.org/z/54v99TMo9. Though it's arguably not worth the extra four instructions and branch prediction slot to optimize for comparing nothing. I mainly kept it because I thought it was UB to call memcmp with null pointers, but I can't find any evidence of that, so removing I'm it.

philnik updated this revision to Diff 481444.Dec 8 2022, 2:14 PM

Next try

philnik updated this revision to Diff 481446.Dec 8 2022, 2:21 PM

Fix static_assert

philnik updated this revision to Diff 481448.Dec 8 2022, 2:27 PM

Remove static_assert again, since it also fires when not actually calling __constexpr_memcmp during constant evaluation

philnik updated this revision to Diff 481488.Dec 8 2022, 5:58 PM

More fixes

philnik updated this revision to Diff 481589.Dec 9 2022, 3:09 AM

Try to fix CI

This revision was automatically updated to reflect the committed changes.
haowei added a subscriber: haowei.Dec 9 2022, 11:24 AM

We are seeing build breakages in Fuchsia's builders after this change was landed and this change is in the blamelist and looks directly related:

[37031/289776](267) CXX kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata.llvm-profdata.cc.o
FAILED: kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata.llvm-profdata.cc.o
../../../recipe_cleanup/clangehe1kz7e/bin/clang++ -MD -MF kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata.llvm-profdata.cc.o.d -o kernel.efi_x64/obj/src/lib/llvm-profdata/libllvm-profdata...
In file included from ../../src/lib/llvm-profdata/llvm-profdata.cc:5:
In file included from ../../src/lib/llvm-profdata/include/lib/llvm-profdata/llvm-profdata.h:11:
In file included from ../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/string_view:220:
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/__string/char_traits.h:286:17: error: no member named '__constexpr_wmemcmp' in namespace 'std'; did you mean '__constexpr_memcmp'?
    return std::__constexpr_wmemcmp(__s1, __s2, __n);
           ~~~~~^
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/cstring:119:1: note: '__constexpr_memcmp' declared here
__constexpr_memcmp(const _Tp* __lhs, const _Tp* __rhs, size_t __count) {
^
In file included from ../../src/lib/llvm-profdata/llvm-profdata.cc:5:
In file included from ../../src/lib/llvm-profdata/include/lib/llvm-profdata/llvm-profdata.h:11:
In file included from ../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/string_view:220:
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/__string/char_traits.h:290:17: error: no member named '__constexpr_wcslen' in namespace 'std'
    return std::__constexpr_wcslen(__s);
           ~~~~~^
../../../recipe_cleanup/clangehe1kz7e/include/c++/v1/__string/char_traits.h:297:17: error: no member named '__constexpr_wmemchr' in namespace 'std'
    return std::__constexpr_wmemchr(__s, __a, __n);
           ~~~~~^
3 errors generated.
clang++: error: failing because '-gen-reproducer' is used
Fuchsia clang version 16.0.0 (https://llvm.googlesource.com/llvm-project fe9e442c57f3b50ec7ad5f01e191745aa3156b4e)
Target: x86_64-unknown-windows-msvc
Thread model: posix
InstalledDir: ../../../recipe_cleanup/clangehe1kz7e/bin
clang++: note: diagnostic msg:
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: clang-crashreports/llvm-profdata-960ad3.cpp
clang++: note: diagnostic msg: clang-crashreports/llvm-profdata-960ad3.sh
clang++: note: diagnostic msg:

********************

Could you take a look?
If it takes a while to fix, could you revert the change first?

Looks like it is Fuchsia's own issue as it overrides libcxx headers in kernel (https://fuchsia.googlesource.com/fuchsia/+/658bbc3cef7f340c058dde67c3d5dd2be4084bdd/zircon/kernel/lib/ktl/include/cwchar). You don't need to take any action. Sorry for the trouble.

Looks like it is Fuchsia's own issue as it overrides libcxx headers in kernel (https://fuchsia.googlesource.com/fuchsia/+/658bbc3cef7f340c058dde67c3d5dd2be4084bdd/zircon/kernel/lib/ktl/include/cwchar). You don't need to take any action. Sorry for the trouble.

I would check if that workaround is still needed. You should be able to configure the library with LIBCXX_ENABLE_WIDE_CHARACTERS=OFF at CMake time to make sure that the library doesn't rely on this header.