[libc-commits] [PATCH] D135322: [libc] add strsignal and refactor message mapping

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/StringUtil/error_to_string.cpp:194
+
+cpp::string_view get_error_string(int err_num, cpp::string_view buffer) {
+  auto opt_str = internal::error_mapper.get_str(err_num);
----------------
sivachandra wrote:
> michaelrj wrote:
> > michaelrj wrote:
> > > sivachandra wrote:
> > > > 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();
> > > > }
> > > > ```
> > > 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.
> > 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.
> 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:
> 
> 1. Our internal API should be clean about indicating errors sufficiently. So, irrespective of setting `errno`, the API should convey error conditions.
> 2. Hidden in my previous comment is a point about changing the buffer type to `cpp::span<char>`.
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. 
Additionally, with the way we currently have the function set up it doesn't have true error states. Regardless of the arguments it will always pass back a valid string (unless the pointer isn't valid, in which case it will segfault). I feel that given the situations strerror is likely to be used in, attempting to pass to the client some sort of error state is almost certainly going to be ignored.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135322/new/

https://reviews.llvm.org/D135322