This is an archive of the discontinued LLVM Phabricator instance.

[libc] move strerror and strsignal to OS msg maps
ClosedPublic

Authored by michaelrj on Apr 10 2023, 1:12 PM.

Details

Summary

Other OSes may have different mappings from error number to message.
This creates a system to allow new platforms to define their own
mappings.

Diff Detail

Event Timeline

michaelrj created this revision.Apr 10 2023, 1:12 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 10 2023, 1:12 PM
michaelrj requested review of this revision.Apr 10 2023, 1:12 PM
sivachandra added inline comments.Apr 13 2023, 11:42 AM
libc/src/__support/StringUtil/CMakeLists.txt
7

I don't think we should just return when a platform directory is missing. See below.

11

This should be conditional:

if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
  add_subdirectory(${LIBC_TARGET_OS})
endif()
21

This should be added conditionally I think:

list(APPEND error_to_string_deps 
  libc.src.errno.errno
  libc.src.__support.CPP.span
  libc.src.__support.CPP.string_view
  libc.src.__support.CPP.stringstream
  libc.src.__support.integer_to_string
)
if(TARGET libc.src.__support.StringUtil.${LIBC_TARGET_OS}.strerror_map)
  list(APPEND error_to_string_deps  libc.src.__support.StringUtil.${LIBC_TARGET_OS}.strerror_map)
else()
  list(APPEND error_to_string_deps  <target for the default error map - see below>)
endif()
37

Ditto.

libc/src/__support/StringUtil/error_to_string.cpp
20

s/__unix__/__linux__ ?

21

Instead of expecting everything to be listed in a platform specific table, we can have three kinds of tables:

  1. Table of errors specified by the C standard - this will be the fallback table instead of the #error you have below.
  2. Table of POSIX extension errors.
  3. Table of Linux extension errors.

To get the full Linux table for example, you will have to merge all the above three tables. You likely have to use cpp::array for the tables to get this to work at compile-time.

michaelrj updated this revision to Diff 513774.Apr 14 2023, 3:35 PM
michaelrj marked 6 inline comments as done.

Update the string mapping to support concatenating arrays at compile time.
Simplify the cmake.

Mostly LGTM but I suggested some rearrangement of what you already have to improve modularity.

libc/src/__support/StringUtil/error_to_string.cpp
21

I prefer an organization like this:

  1. stdc_error_table.h contains:
inline constexpr ErrorTable<...> STDC_ERRORS = {
  ...
};
  1. posix_error_table.h contains:
inline constexpr ErrorTable<...> POSIX_EXTENSION_ERRORS = {
  ...
};
  1. linux/error_table.h contains:
#include "stdc_errors.h"
#include "posix_errros.h"

namespace __llvm_libc {
inline constexpr ErrorTable<...> LINUX_EXTENSION_ERRORS = {
  ...
};
} // namespace __llvm_libc
  1. error_table.h contains:
#ifdef __linux__
#include "linux/error_table.h"
inline constexpr ErrorTable<...> PLATFORM_ERRORS = STDC_ERRORS + POSIX_EXTENSION_ERRORS + LINUX_EXTENSION_ERRORS;
#else
#include "stdc_error_table.h"
inline constexpr ErrorTable<...> PLATFORM_ERRORS = STDC_ERRORS;
#endif

In this file, all you have to do is include error_table.h and operate on PLATFORM_ERRORS.

libc/src/__support/StringUtil/message_mapper.h
32

Do we need a special class? Can this work:

template <size_t N>
using ErrorTable = cpp::array<MsgMapping, N>;

template <size_t N1, size_t N2>
constexpr ErrorTable<N1 + N2> operator+(const ErrorTable &t1, const ErrorTable &t2) {
  ErrorTable<N1 + N2> res{};
  for (std::size_t i = 0; i < N1; ++i)
    res[i] = t1[i];
  for (std::size_t i = 0; i < N2; ++i)
    res[N1 + i] = t2[i];
  return res;
}
michaelrj updated this revision to Diff 514742.Apr 18 2023, 2:14 PM
michaelrj marked 2 inline comments as done.

subdivide the files more and remove unnecessary custom class.

libc/src/__support/StringUtil/error_to_string.cpp
21

I moved these files into a subdirectory for clarity, but otherwise this is laid out as described.

libc/src/__support/StringUtil/message_mapper.h
32

Interesting, this does work, but when I tried adding operator+ to cpp::array directly it complained about the result not being constexpr.

sivachandra accepted this revision.Apr 18 2023, 3:59 PM
sivachandra added inline comments.
libc/src/__support/StringUtil/CMakeLists.txt
11

We are moving to giving directories also lower case names. So, may be just tables for the nested directory?

libc/src/__support/StringUtil/TableMappings/error_table.h
26 ↗(On Diff #514742)

Lets not add POSIX_ERRORS for the default case.

libc/src/__support/StringUtil/TableMappings/linux/error_table.h
19 ↗(On Diff #514742)

constexpr tables declared in header files should be inline constexpr ....

libc/src/__support/StringUtil/error_to_string.cpp
33

Remove?

This revision is now accepted and ready to land.Apr 18 2023, 3:59 PM
michaelrj updated this revision to Diff 514784.Apr 18 2023, 4:40 PM
michaelrj marked 4 inline comments as done.

address comments before landing

This revision was landed with ongoing or failed builds.Apr 18 2023, 4:42 PM
This revision was automatically updated to reflect the committed changes.