The logic for strsignal and strerror is very similar, so I've moved them
both to use a shared utility (MessageMapper) for the basic
functionality.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
The new MsgMapper LGTM but I have comments about other parts of the design.
| libc/src/__support/StringUtil/error_to_string.cpp | ||
|---|---|---|
| 194 | It seems to me like we should not have this overload in the stringutil at all because the actual libc functions need to set errno. Instead, provide a function like: bool fill_unknown_error_msg(int err_num, cpp::span<char> buffer) {
...
// return true if buffer was big enough, else false.
// This will help strerror_r set errno to ERANGE based on the return value.
}So, strerror should be implemented like this: LLVM_LIBC_FUNCTION(... strerror ...) {
auto err_str = get_error_string(num);
if (num)
return const_cast<char *>(num.data());
errno = EINVAL;
// unknown_err_buffer is declared in strerror.cpp.
// In case of strerror_r, it is instantiated from the buffer argument.
fill_unknown_err_msg(num, unknown_err_buffer);
return unknown_err_buffer.data();
} | |
| libc/src/__support/StringUtil/message_mapper.h | ||
| 23 | This is a struct so public is default. | |
| 68 | Why is the const_cast required? | |
| libc/src/__support/StringUtil/signal_to_string.cpp | ||
| 119 | Delete the commented out blocks above? | |
| libc/src/__support/StringUtil/error_to_string.cpp | ||
|---|---|---|
| 194 | For comments on the strerror_r design you should comment on this patch instead: https://reviews.llvm.org/D135227 but overall I think the idea makes sense. | |
| libc/src/__support/StringUtil/error_to_string.cpp | ||
|---|---|---|
| 194 | after further testing, it appears that strerror and strerror_r don't actually set errno in existing implementations. Given that information, I don't think this is necessary. | |
| libc/src/__support/StringUtil/error_to_string.cpp | ||
|---|---|---|
| 194 | I do not know what the existing implementations do, but https://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror.html seems to indicate that errno "may be set" to indicate error. So, existing implementations have probably chosen to not set errno. Couple of things still:
| |
| libc/src/__support/StringUtil/error_to_string.cpp | ||
|---|---|---|
| 194 | The buffer type can't be span because we're not always copying the data into the provided pointer. Most of the time we're returning a static pointer, and span doesn't support changing its underlying pointer. | |
It seems to me like we should not have this overload in the stringutil at all because the actual libc functions need to set errno. Instead, provide a function like:
bool fill_unknown_error_msg(int err_num, cpp::span<char> buffer) { ... // return true if buffer was big enough, else false. // This will help strerror_r set errno to ERANGE based on the return value. }So, strerror should be implemented like this:
LLVM_LIBC_FUNCTION(... strerror ...) { auto err_str = get_error_string(num); if (num) return const_cast<char *>(num.data()); errno = EINVAL; // unknown_err_buffer is declared in strerror.cpp. // In case of strerror_r, it is instantiated from the buffer argument. fill_unknown_err_msg(num, unknown_err_buffer); return unknown_err_buffer.data(); }