This is an archive of the discontinued LLVM Phabricator instance.

[libc] Use string_view for write_to_stderr
ClosedPublic

Authored by gchatelet on Apr 1 2023, 4:36 AM.

Details

Summary

This patch makes use of cpp::string_view instead of const char* for write_to_stderr. This helps sending non null-terminated buffers such as a single character, cpp::string_view or cpp::string.
It also fizes the gpu version that had several bugs (See https://reviews.llvm.org/D145913#4236641).

Diff Detail

Event Timeline

gchatelet created this revision.Apr 1 2023, 4:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 1 2023, 4:36 AM
gchatelet requested review of this revision.Apr 1 2023, 4:36 AM
gchatelet updated this revision to Diff 510212.Apr 1 2023, 4:41 AM
  • Always send null-terminated string so we can use fputs on the server side

Do we want to stop using const char *? We only use this internally currently, but it was easier for me to test when it was a C type. I'll let @sivachandra decide.

libc/src/__support/OSUtil/gpu/io.cpp
22
26

Does libc not use C++ style casts?

Do we want to stop using const char *? We only use this internally currently, but it was easier for me to test when it was a C type. I'll let @sivachandra decide.

cpp::string_view is implicitly constructible from a const char* so the current code still works. Are you calling the function from C code?
Relevant discussion for this patch: https://reviews.llvm.org/D147231#inline-1423241

libc/src/__support/OSUtil/gpu/io.cpp
22

Thx for catching the typo!

gchatelet updated this revision to Diff 510218.Apr 1 2023, 5:20 AM
  • Address comments
gchatelet marked 2 inline comments as done.Apr 1 2023, 5:21 AM

Do we want to stop using const char *? We only use this internally currently, but it was easier for me to test when it was a C type. I'll let @sivachandra decide.

cpp::string_view is implicitly constructible from a const char* so the current code still works. Are you calling the function from C code?
Relevant discussion for this patch: https://reviews.llvm.org/D147231#inline-1423241

I was just calling it using this function for a standalone test, but it's not a huge deal. Alternatively, we could keep the function interface the same and do the conversion internally.

namespace __llvm_libc {
void write_to_stderr(const char *msg);
void quick_exit(int);
} // namespace __llvm_libc
sivachandra accepted this revision.Apr 1 2023, 8:46 AM

OK for linux/io.h.

This revision is now accepted and ready to land.Apr 1 2023, 8:46 AM
jhuber6 accepted this revision.Apr 1 2023, 8:47 AM
gchatelet updated this revision to Diff 510342.Apr 2 2023, 5:51 AM
  • Keep "const char*" version for gpu/io.h
gchatelet added a comment.EditedApr 2 2023, 5:51 AM

Do we want to stop using const char *? We only use this internally currently, but it was easier for me to test when it was a C type. I'll let @sivachandra decide.

cpp::string_view is implicitly constructible from a const char* so the current code still works. Are you calling the function from C code?
Relevant discussion for this patch: https://reviews.llvm.org/D147231#inline-1423241

I was just calling it using this function for a standalone test, but it's not a huge deal.

I can keep the const char* version in the gpu folder on top of the string_view version if that helps.

Alternatively, we could keep the function interface the same and do the conversion internally.

This wouldn't help for single char or non null-terminated strings.

That's fine, we can always change things later.

This revision was automatically updated to reflect the committed changes.