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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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);
----------------
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>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135322