This is an archive of the discontinued LLVM Phabricator instance.

[libc] Make ErrnoSetterMatcher handle logging floating point values.
ClosedPublic

Authored by sivachandra on May 22 2023, 11:29 AM.

Details

Summary

Along the way, couple of additional things have been done:

  1. Move ErrnoSetterMatcher.h to test/UnitTest as all other matchers live there now.
  2. ErrnoSetterMatcher ignores matching errno on GPUs.

Diff Detail

Event Timeline

sivachandra created this revision.May 22 2023, 11:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 22 2023, 11:29 AM
sivachandra requested review of this revision.May 22 2023, 11:29 AM
sivachandra edited the summary of this revision. (Show Details)May 25 2023, 12:26 AM
sivachandra added a reviewer: jhuber6.

Applied locally, here's some errors while building,

In file included from /home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:10:
In file included from /home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/platform_errors.h:15:
In file included from /home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/minimal_platform_errors.h:12:
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/stdc_errors.h:22:16: error: use of undeclared identifier 'EILSEQ'
    MsgMapping(EILSEQ, "Invalid or incomplete multibyte or wide character"),
               ^
In file included from /home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:10:
In file included from /home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/platform_errors.h:15:
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/minimal_platform_errors.h:16:23: error: constexpr variable 'PLATFORM_ERRORS' must be initialized by a constant expression
inline constexpr auto PLATFORM_ERRORS = STDC_ERRORS;
                      ^                 ~~~~~~~~~~~
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/minimal_platform_errors.h:16:41: note: initializer of 'STDC_ERRORS' is unknown
inline constexpr auto PLATFORM_ERRORS = STDC_ERRORS;
                                        ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/minimal_platform_errors.h:16:41: note: in call to 'array(STDC_ERRORS)'
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/stdc_errors.h:18:36: note: declared here
inline constexpr const MsgTable<4> STDC_ERRORS = {
                                   ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:36:18: error: constexpr variable 'TOTAL_STR_LEN' must be initialized by a constant expression
constexpr size_t TOTAL_STR_LEN = total_str_len(PLATFORM_ERRORS);
                 ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/message_mapper.h:35:32: note: initializer of 'PLATFORM_ERRORS' is not a constant expression
  for (size_t i = 0; i < table.size(); ++i) {
                               ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:36:34: note: in call to 'total_str_len(PLATFORM_ERRORS)'
constexpr size_t TOTAL_STR_LEN = total_str_len(PLATFORM_ERRORS);
                                 ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/minimal_platform_errors.h:16:23: note: declared here
inline constexpr auto PLATFORM_ERRORS = STDC_ERRORS;
                      ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:44:18: error: constexpr variable 'ERR_ARRAY_SIZE' must be initialized by a constant expression
constexpr size_t ERR_ARRAY_SIZE = max_key_val(PLATFORM_ERRORS) + 1;
                 ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/message_mapper.h:44:32: note: initializer of 'PLATFORM_ERRORS' is not a constant expression
  for (size_t i = 0; i < table.size(); ++i) {
                               ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:44:35: note: in call to 'max_key_val(PLATFORM_ERRORS)'
constexpr size_t ERR_ARRAY_SIZE = max_key_val(PLATFORM_ERRORS) + 1;
                                  ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/tables/minimal_platform_errors.h:16:23: note: declared here
inline constexpr auto PLATFORM_ERRORS = STDC_ERRORS;
                      ^
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:46:25: error: non-type template argument is not a constant expression
constexpr MessageMapper<ERR_ARRAY_SIZE, TOTAL_STR_LEN>
                        ^~~~~~~~~~~~~~
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:46:25: note: initializer of 'ERR_ARRAY_SIZE' is not a constant expression
/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:44:18: note: declared here
constexpr size_t ERR_ARRAY_SIZE = max_key_val(PLATFORM_ERRORS) + 1;
                 ^
5 errors generated.

Add EILSEQ to the list of generic error number macros.

New failure due to thread_local

/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:34:1: error: thread-local storage is not supported for the current target
thread_local char error_buffer[ERR_BUFFER_SIZE];
^

I think all of those errors have a single root cause that EILSEQ was missing from the list of generic error numbers. I have added it now. Can you give it another try?

I think all of those errors have a single root cause that EILSEQ was missing from the list of generic error numbers. I have added it now. Can you give it another try?

Yes, I think we just need to make the thread_local error buffer a regular integer on platforms without errno. (Same reason we can't support it in the first place).

I think all of those errors have a single root cause that EILSEQ was missing from the list of generic error numbers. I have added it now. Can you give it another try?

Yes, I think we just need to make the thread_local error buffer a regular integer on platforms without errno. (Same reason we can't support it in the first place).

Few things:

  1. This patch is likely titled incorrectly titled because I uploaded when I wanted to quickly show how we can ignore testing for errno. I totally happy to remove that part (re-title this patch anyway).
  2. If you want to consider GPU to be a platform without errno, that is up to you. But, This question goes beyond errno. Is the GPU platform in general a platform without thread local variable support? If yes, we need to say that somewhere and give guidance on how to detect C library function errors.
  3. Is the libc project groups and patches the right forum to discuss/decide about GPU errno?
sivachandra retitled this revision from [libc] Make ErrnoSetterMatcher ignore matching errno on GPU. to [libc] Make ErrnoSetterMatcher handle logging floating point values..May 25 2023, 10:50 AM
sivachandra edited the summary of this revision. (Show Details)
sivachandra added reviewers: lntue, michaelrj.
sivachandra edited the summary of this revision. (Show Details)May 25 2023, 10:52 AM

I think all of those errors have a single root cause that EILSEQ was missing from the list of generic error numbers. I have added it now. Can you give it another try?

Yes, I think we just need to make the thread_local error buffer a regular integer on platforms without errno. (Same reason we can't support it in the first place).

Few things:

  1. This patch is likely titled incorrectly titled because I uploaded when I wanted to quickly show how we can ignore testing for errno. I totally happy to remove that part (re-title this patch anyway).

Okay, this prevented running the existing tests on the GPU so it should at least be worked around for this to land.

  1. If you want to consider GPU to be a platform without errno, that is up to you. But, This question goes beyond errno. Is the GPU platform in general a platform without thread local variable support? If yes, we need to say that somewhere and give guidance on how to detect C library function errors.

That's the simplest solution right now. The GPU does have thread local memory, it comes from the stack. It's very limited but it does exist. The problem is that this stack variable would require each executing kernel on the GPU to initialize the variables. There's basically a few ways to get around this if we really wanted to support it.

  1. Preallocate global memory in the GPU loader such that every software thread has a slot.
  2. Statically allocate 1024 integers in block-local memory and index out of it.
  3. Perform an alloca instruction from the bottom of the stack and initialize it
  4. Set up true codegen for the GPU that basically does 3 but automatically. This would require a lot of fun with sections like how TLS is handled on Linux to implement properly.
  5. Ignore it completely.
  6. Make it a global and have data races
  1. Is the libc project groups and patches the right forum to discuss/decide about GPU errno?

I'm not sure, there hasn't exactly been an outcry from the GPU community for errno support. Speaking frankly most users would only *really* care about malloc, free, and `printf. Most everything else can be seen as supplemental.

I think all of those errors have a single root cause that EILSEQ was missing from the list of generic error numbers. I have added it now. Can you give it another try?

Yes, I think we just need to make the thread_local error buffer a regular integer on platforms without errno. (Same reason we can't support it in the first place).

Few things:

  1. This patch is likely titled incorrectly titled because I uploaded when I wanted to quickly show how we can ignore testing for errno. I totally happy to remove that part (re-title this patch anyway).

Okay, this prevented running the existing tests on the GPU so it should at least be worked around for this to land.

How is the latest version of this patch?

  1. If you want to consider GPU to be a platform without errno, that is up to you. But, This question goes beyond errno. Is the GPU platform in general a platform without thread local variable support? If yes, we need to say that somewhere and give guidance on how to detect C library function errors.

That's the simplest solution right now. The GPU does have thread local memory, it comes from the stack. It's very limited but it does exist. The problem is that this stack variable would require each executing kernel on the GPU to initialize the variables. There's basically a few ways to get around this if we really wanted to support it.

  1. Preallocate global memory in the GPU loader such that every software thread has a slot.
  2. Statically allocate 1024 integers in block-local memory and index out of it.
  3. Perform an alloca instruction from the bottom of the stack and initialize it
  4. Set up true codegen for the GPU that basically does 3 but automatically. This would require a lot of fun with sections like how TLS is handled on Linux to implement properly.
  5. Ignore it completely.
  6. Make it a global and have data races
  1. Is the libc project groups and patches the right forum to discuss/decide about GPU errno?

I'm not sure, there hasn't exactly been an outcry from the GPU community for errno support. Speaking frankly most users would only *really* care about malloc, free, and `printf. Most everything else can be seen as supplemental.

I will leave it up to you to drive this discussion in the appropriate forums. Even things like, should there be an errno.h in the GPU libc with relevant error macros exposed? For the current work in the libc around this, I only view them as "workarounds" which will help make progress with making more of the libc available on the GPUs while the fundamental questions are being sorted out.

Did you update the patch? It's still got the same error:

/home/jhuber/Documents/llvm/llvm-project/libc/src/__support/StringUtil/error_to_string.cpp:34:1: error: thread-local storage is not supported for the current target
thread_local char error_buffer[ERR_BUFFER_SIZE];
^
1 error generated.
jhuber6 accepted this revision.May 25 2023, 1:19 PM

Works now, thanks!

This revision is now accepted and ready to land.May 25 2023, 1:19 PM