This is an archive of the discontinued LLVM Phabricator instance.

[libc] add strerror
ClosedPublic

Authored by michaelrj on Sep 16 2022, 1:55 PM.

Details

Summary

Strerror maps error numbers to strings. Additionally, a utility for
mapping errors to strings was added so that it could be reused for
perror and similar.

Diff Detail

Event Timeline

michaelrj created this revision.Sep 16 2022, 1:55 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 16 2022, 1:55 PM
michaelrj requested review of this revision.Sep 16 2022, 1:55 PM
sivachandra added inline comments.Sep 16 2022, 3:32 PM
libc/src/__support/error_to_string.h
2

Almost everything in this file should live in a .cpp file. The only thing declared in the .h file should something like:

cpp::string_view error_to_string(int error);

IMO, that single function should take care of unknown error numbers also. Do you have a use case where max_buff_size and get_unknown_str are required?

A more fundamental question - is there any reason why this should live in __support at all?

michaelrj added inline comments.Sep 16 2022, 3:49 PM
libc/src/__support/error_to_string.h
2

I did make a .cpp file to contain the functions for the class, but was running into compile errors where the linker couldn't find the implementations, so I moved it into the header for now.

As for the other parts:
The reason max_buff_size and get_unknown_str exist is that when an error without a specified string mapping is passed, we need to generate a new string that says Unknown error [number]. Since this string will be passed back to the caller code, it can't be on the stack, so we need a buffer to hold it. In strerror.cpp I define a global buffer of size max_buff_size to be that buffer. This is specifically allowed by the standard: "The strerror function returns a pointer to the string, the contents of which are localespecific. The array pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the strerror function."

I placed this in support because both strerror which is in strings.h and perror which is in stdio.h use the same functionality. I can move it into strings, but generally I try to avoid including files between headers when possible.

sivachandra added inline comments.Sep 16 2022, 10:14 PM
libc/src/__support/error_to_string.h
2

I did make a .cpp file to contain the functions for the class, but was running into compile errors where the linker couldn't find the implementations, so I moved it into the header for now.

That shouldn't happen - we have lots of examples where in we store tables in .cpp files, especially in the math directory.

As for the other parts:
The reason max_buff_size and get_unknown_str exist is that when an error without a specified string mapping is passed, we need to generate a new string that says Unknown error [number]. Since this string will be passed back to the caller code, it can't be on the stack, so we need a buffer to hold it. In strerror.cpp I define a global buffer of size max_buff_size to be that buffer. This is specifically allowed by the standard: "The strerror function returns a pointer to the string, the contents of which are localespecific. The array pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the strerror function."

My question was more about, why should, say the strerror implementation, provide its own error buffer. Instead, why can't all these details be hidden behind a single error_to_string. Also, since errno is a thread local value, the error string buffer should also be thread local.

I placed this in support because both strerror which is in strings.h and perror which is in stdio.h use the same functionality. I can move it into strings, but generally I try to avoid including files between headers when possible.

SGTM

michaelrj updated this revision to Diff 461334.Sep 19 2022, 1:15 PM

avoid global array of global pointers
moved implementation details into internal namespace

sivachandra accepted this revision.Sep 20 2022, 11:40 AM
sivachandra added inline comments.
libc/src/__support/error_to_string.cpp
28

Add a comment somewhere explaining why this is present at all.

230
This revision is now accepted and ready to land.Sep 20 2022, 11:40 AM
michaelrj updated this revision to Diff 461700.Sep 20 2022, 2:03 PM
michaelrj marked 2 inline comments as done.

move to stringstream for string construction

michaelrj marked 2 inline comments as done.Sep 20 2022, 2:04 PM
sivachandra added inline comments.Sep 20 2022, 2:45 PM
libc/src/__support/error_to_string.cpp
227

There is no point making this method a constexpr and introducing the const_convert (that would in a way be an antipattern) method - the argument err_num is not going to be a constexpr so this function is always evaluated at runtime.

michaelrj updated this revision to Diff 461734.Sep 20 2022, 3:46 PM

remove constexpr from parts that don't need it

michaelrj marked an inline comment as done.Sep 20 2022, 3:47 PM
sivachandra accepted this revision.Sep 20 2022, 3:55 PM
This revision was landed with ongoing or failed builds.Sep 20 2022, 4:23 PM
Closed by commit rG42bcb35c0f29: [libc] add strerror (authored by michaelrj). · Explain Why
This revision was automatically updated to reflect the committed changes.