[libc] Add printf error handling #162876
[libc] Add printf error handling #162876
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mleleszi
force-pushed
the
libc-printf-error-handling
branch
from
c306ed5 to
71554f9
Compare
|
@michaelrj-google Can you please take a look? |
|
@llvm/pr-subscribers-libc Author: Marcell Leleszi (mleleszi) Changes
Internal changes:
Patch is 42.23 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/162876.diff 30 Files Affected:
diff --git a/libc/src/stdio/asprintf.cpp b/libc/src/stdio/asprintf.cpp
index f8cfb74ce48ea..13c1ee68ef0f2 100644
--- a/libc/src/stdio/asprintf.cpp
+++ b/libc/src/stdio/asprintf.cpp
@@ -22,8 +22,17 @@ LLVM_LIBC_FUNCTION(int, asprintf,
// and pointer semantics, as well as handling
// destruction automatically.
va_end(vlist);
- int ret = printf_core::vasprintf_internal(buffer, format, args);
- return ret;
+ auto ret_val = printf_core::vasprintf_internal(buffer, format, args);
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/baremetal/printf.cpp b/libc/src/stdio/baremetal/printf.cpp
index 7253c6549a4e4..a2e308ed54a57 100644
--- a/libc/src/stdio/baremetal/printf.cpp
+++ b/libc/src/stdio/baremetal/printf.cpp
@@ -42,13 +42,24 @@ LLVM_LIBC_FUNCTION(int, printf, (const char *__restrict format, ...)) {
buffer, BUFF_SIZE, &stdout_write_hook, nullptr);
printf_core::Writer<printf_core::WriteMode::FLUSH_TO_STREAM> writer(wb);
- int retval = printf_core::printf_main(&writer, format, args);
+ auto retval = printf_core::printf_main(&writer, format, args);
+ if (retval.has_error()) {
+ libc_errno = retval.error;
+ return -1;
+ }
int flushval = wb.overflow_write("");
- if (flushval != printf_core::WRITE_OK)
- retval = flushval;
+ if (flushval != printf_core::WRITE_OK) {
+ libc_errno = -flushval;
+ return -1;
+ }
- return retval;
+ if (retval.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(retval.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/baremetal/vprintf.cpp b/libc/src/stdio/baremetal/vprintf.cpp
index ab02533f14911..003d7d70707fb 100644
--- a/libc/src/stdio/baremetal/vprintf.cpp
+++ b/libc/src/stdio/baremetal/vprintf.cpp
@@ -40,13 +40,24 @@ LLVM_LIBC_FUNCTION(int, vprintf,
buffer, BUFF_SIZE, &stdout_write_hook, nullptr);
printf_core::Writer<printf_core::WriteMode::FLUSH_TO_STREAM> writer(wb);
- int retval = printf_core::printf_main(&writer, format, args);
+ auto retval = printf_core::printf_main(&writer, format, args);
+ if (retval.has_error()) {
+ libc_errno = retval.error;
+ return -1;
+ }
int flushval = wb.overflow_write("");
- if (flushval != printf_core::WRITE_OK)
- retval = flushval;
+ if (flushval != printf_core::WRITE_OK) {
+ libc_errno = -flushval;
+ return -1;
+ }
- return retval;
+ if (retval.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(retval.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/generic/fprintf.cpp b/libc/src/stdio/generic/fprintf.cpp
index 087aeadfc52c5..632264f944f44 100644
--- a/libc/src/stdio/generic/fprintf.cpp
+++ b/libc/src/stdio/generic/fprintf.cpp
@@ -27,8 +27,17 @@ LLVM_LIBC_FUNCTION(int, fprintf,
// and pointer semantics, as well as handling
// destruction automatically.
va_end(vlist);
- int ret_val = printf_core::vfprintf_internal(stream, format, args);
- return ret_val;
+ auto ret_val = printf_core::vfprintf_internal(stream, format, args);
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/generic/printf.cpp b/libc/src/stdio/generic/printf.cpp
index bb7c7c86f843f..482a65cd5a2fa 100644
--- a/libc/src/stdio/generic/printf.cpp
+++ b/libc/src/stdio/generic/printf.cpp
@@ -31,9 +31,18 @@ LLVM_LIBC_FUNCTION(int, printf, (const char *__restrict format, ...)) {
// and pointer semantics, as well as handling
// destruction automatically.
va_end(vlist);
- int ret_val = printf_core::vfprintf_internal(
+ auto ret_val = printf_core::vfprintf_internal(
reinterpret_cast<::FILE *>(PRINTF_STDOUT), format, args);
- return ret_val;
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/generic/vfprintf.cpp b/libc/src/stdio/generic/vfprintf.cpp
index 01f4265f118a6..6b39011123084 100644
--- a/libc/src/stdio/generic/vfprintf.cpp
+++ b/libc/src/stdio/generic/vfprintf.cpp
@@ -24,8 +24,17 @@ LLVM_LIBC_FUNCTION(int, vfprintf,
internal::ArgList args(vlist); // This holder class allows for easier copying
// and pointer semantics, as well as handling
// destruction automatically.
- int ret_val = printf_core::vfprintf_internal(stream, format, args);
- return ret_val;
+ auto ret_val = printf_core::vfprintf_internal(stream, format, args);
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/generic/vprintf.cpp b/libc/src/stdio/generic/vprintf.cpp
index 08d71515646ed..09ba2ce0d32a9 100644
--- a/libc/src/stdio/generic/vprintf.cpp
+++ b/libc/src/stdio/generic/vprintf.cpp
@@ -29,9 +29,18 @@ LLVM_LIBC_FUNCTION(int, vprintf,
internal::ArgList args(vlist); // This holder class allows for easier copying
// and pointer semantics, as well as handling
// destruction automatically.
- int ret_val = printf_core::vfprintf_internal(
+ auto ret_val = printf_core::vfprintf_internal(
reinterpret_cast<::FILE *>(PRINTF_STDOUT), format, args);
- return ret_val;
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/printf_core/core_structs.h b/libc/src/stdio/printf_core/core_structs.h
index e27f77b6b594a..3cb76ed32776f 100644
--- a/libc/src/stdio/printf_core/core_structs.h
+++ b/libc/src/stdio/printf_core/core_structs.h
@@ -22,6 +22,18 @@
namespace LIBC_NAMESPACE_DECL {
namespace printf_core {
+struct PrintfResult {
+ size_t value;
+ int error;
+
+ constexpr PrintfResult(size_t val) : value(val), error(0) {}
+ constexpr PrintfResult(size_t val, int error) : value(val), error(error) {}
+
+ constexpr bool has_error() { return error != 0; }
+
+ constexpr operator size_t() { return value; }
+};
+
// These length modifiers match the length modifiers in the format string, which
// is why they are formatted differently from the rest of the file.
enum class LengthModifier { hh, h, l, ll, j, z, t, L, w, wf, none };
@@ -132,14 +144,6 @@ template <typename T> LIBC_INLINE constexpr TypeDesc type_desc_from_type() {
// This is the value to be returned by conversions when no error has occurred.
constexpr int WRITE_OK = 0;
-// These are the printf return values for when an error has occurred. They are
-// all negative, and should be distinct.
-constexpr int FILE_WRITE_ERROR = -1;
-constexpr int FILE_STATUS_ERROR = -2;
-constexpr int NULLPTR_WRITE_ERROR = -3;
-constexpr int INT_CONVERSION_ERROR = -4;
-constexpr int FIXED_POINT_CONVERSION_ERROR = -5;
-constexpr int ALLOCATION_ERROR = -6;
} // namespace printf_core
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/printf_core/fixed_converter.h b/libc/src/stdio/printf_core/fixed_converter.h
index e8a3314967562..77384b1891174 100644
--- a/libc/src/stdio/printf_core/fixed_converter.h
+++ b/libc/src/stdio/printf_core/fixed_converter.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_FIXED_CONVERTER_H
#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_FIXED_CONVERTER_H
+#include "hdr/errno_macros.h"
#include "include/llvm-libc-macros/stdfix-macros.h"
#include "src/__support/CPP/string_view.h"
#include "src/__support/ctype_utils.h"
@@ -59,7 +60,7 @@ LIBC_INLINE constexpr uint32_t const_ten_exp(uint32_t exponent) {
READ_FX_BITS(unsigned LENGTH_MODIFIER accum); \
} else { \
LIBC_ASSERT(false && "Invalid conversion name passed to convert_fixed"); \
- return FIXED_POINT_CONVERSION_ERROR; \
+ return -EINVAL; \
} \
} while (false)
diff --git a/libc/src/stdio/printf_core/int_converter.h b/libc/src/stdio/printf_core/int_converter.h
index 11234c32ce997..554436c9091a8 100644
--- a/libc/src/stdio/printf_core/int_converter.h
+++ b/libc/src/stdio/printf_core/int_converter.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_INT_CONVERTER_H
#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_INT_CONVERTER_H
+#include "hdr/errno_macros.h"
#include "src/__support/CPP/span.h"
#include "src/__support/CPP/string_view.h"
#include "src/__support/ctype_utils.h"
@@ -91,7 +92,7 @@ LIBC_INLINE int convert_int(Writer<write_mode> *writer,
cpp::array<char, details::num_buf_size()> buf;
auto str = details::num_to_strview(num, buf, to_conv.conv_name);
if (!str)
- return INT_CONVERSION_ERROR;
+ return -ERANGE;
size_t digits_written = str->size();
diff --git a/libc/src/stdio/printf_core/printf_main.h b/libc/src/stdio/printf_core/printf_main.h
index 57f29858d5298..f187b43a9cd8f 100644
--- a/libc/src/stdio/printf_core/printf_main.h
+++ b/libc/src/stdio/printf_core/printf_main.h
@@ -22,8 +22,8 @@ namespace LIBC_NAMESPACE_DECL {
namespace printf_core {
template <WriteMode write_mode>
-int printf_main(Writer<write_mode> *writer, const char *__restrict str,
- internal::ArgList &args) {
+PrintfResult printf_main(Writer<write_mode> *writer, const char *__restrict str,
+ internal::ArgList &args) {
Parser<internal::ArgList> parser(str, args);
int result = 0;
for (FormatSection cur_section = parser.get_next_section();
@@ -33,9 +33,8 @@ int printf_main(Writer<write_mode> *writer, const char *__restrict str,
result = convert(writer, cur_section);
else
result = writer->write(cur_section.raw_string);
-
if (result < 0)
- return result;
+ return {0, -result};
}
return writer->get_chars_written();
diff --git a/libc/src/stdio/printf_core/vasprintf_internal.h b/libc/src/stdio/printf_core/vasprintf_internal.h
index 283d8df2810fb..f84f60bec2fc6 100644
--- a/libc/src/stdio/printf_core/vasprintf_internal.h
+++ b/libc/src/stdio/printf_core/vasprintf_internal.h
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "hdr/errno_macros.h"
#include "hdr/func/free.h"
#include "hdr/func/malloc.h"
#include "hdr/func/realloc.h"
@@ -29,7 +30,7 @@ LIBC_INLINE int resize_overflow_hook(cpp::string_view new_str, void *target) {
if (new_buff == nullptr) {
if (wb->buff != wb->init_buff)
free(wb->buff);
- return printf_core::ALLOCATION_ERROR;
+ return -ENOMEM;
}
if (isBuffOnStack)
inline_memcpy(new_buff, wb->buff, wb->buff_cur);
@@ -42,27 +43,28 @@ LIBC_INLINE int resize_overflow_hook(cpp::string_view new_str, void *target) {
constexpr size_t DEFAULT_BUFFER_SIZE = 200;
-LIBC_INLINE int vasprintf_internal(char **ret, const char *__restrict format,
- internal::ArgList args) {
+LIBC_INLINE PrintfResult vasprintf_internal(char **ret,
+ const char *__restrict format,
+ internal::ArgList args) {
char init_buff_on_stack[DEFAULT_BUFFER_SIZE];
printf_core::WriteBuffer<Mode<WriteMode::RESIZE_AND_FILL_BUFF>::value> wb(
init_buff_on_stack, DEFAULT_BUFFER_SIZE, resize_overflow_hook);
printf_core::Writer writer(wb);
auto ret_val = printf_core::printf_main(&writer, format, args);
- if (ret_val < 0) {
+ if (ret_val.has_error()) {
*ret = nullptr;
- return -1;
+ return ret_val;
}
if (wb.buff == init_buff_on_stack) {
- *ret = static_cast<char *>(malloc(ret_val + 1));
+ *ret = static_cast<char *>(malloc(ret_val.value + 1));
if (ret == nullptr)
- return printf_core::ALLOCATION_ERROR;
- inline_memcpy(*ret, wb.buff, ret_val);
+ return {0, ENOMEM};
+ inline_memcpy(*ret, wb.buff, ret_val.value);
} else {
*ret = wb.buff;
}
- (*ret)[ret_val] = '\0';
+ (*ret)[ret_val.value] = '\0';
return ret_val;
}
} // namespace printf_core
diff --git a/libc/src/stdio/printf_core/vfprintf_internal.h b/libc/src/stdio/printf_core/vfprintf_internal.h
index 630de9d9d43dd..c50cacf2709a7 100644
--- a/libc/src/stdio/printf_core/vfprintf_internal.h
+++ b/libc/src/stdio/printf_core/vfprintf_internal.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_VFPRINTF_INTERNAL_H
#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_VFPRINTF_INTERNAL_H
+#include "hdr/errno_macros.h"
#include "src/__support/File/file.h"
#include "src/__support/arg_list.h"
#include "src/__support/macros/attributes.h" // For LIBC_INLINE
@@ -63,23 +64,27 @@ LIBC_INLINE int file_write_hook(cpp::string_view new_str, void *fp) {
size_t written = internal::fwrite_unlocked(new_str.data(), sizeof(char),
new_str.size(), target_file);
if (written != new_str.size() || internal::ferror_unlocked(target_file))
- return FILE_WRITE_ERROR;
+ return -EIO;
return WRITE_OK;
}
-LIBC_INLINE int vfprintf_internal(::FILE *__restrict stream,
- const char *__restrict format,
- internal::ArgList &args) {
+LIBC_INLINE PrintfResult vfprintf_internal(::FILE *__restrict stream,
+ const char *__restrict format,
+ internal::ArgList &args) {
constexpr size_t BUFF_SIZE = 1024;
char buffer[BUFF_SIZE];
printf_core::WriteBuffer<Mode<WriteMode::FLUSH_TO_STREAM>::value> wb(
buffer, BUFF_SIZE, &file_write_hook, reinterpret_cast<void *>(stream));
Writer writer(wb);
internal::flockfile(stream);
- int retval = printf_main(&writer, format, args);
+ auto retval = printf_main(&writer, format, args);
+ if (retval.has_error()) {
+ internal::funlockfile(stream);
+ return retval;
+ }
int flushval = wb.overflow_write("");
if (flushval != WRITE_OK)
- retval = flushval;
+ retval.error = -flushval;
internal::funlockfile(stream);
return retval;
}
diff --git a/libc/src/stdio/printf_core/write_int_converter.h b/libc/src/stdio/printf_core/write_int_converter.h
index efcff278bd284..b424278c66185 100644
--- a/libc/src/stdio/printf_core/write_int_converter.h
+++ b/libc/src/stdio/printf_core/write_int_converter.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITE_INT_CONVERTER_H
#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITE_INT_CONVERTER_H
+#include "hdr/errno_macros.h"
#include "src/__support/macros/config.h"
#include "src/stdio/printf_core/core_structs.h"
#include "src/stdio/printf_core/writer.h"
@@ -26,14 +27,14 @@ LIBC_INLINE int convert_write_int(Writer<write_mode> *writer,
#ifndef LIBC_COPT_PRINTF_NO_NULLPTR_CHECKS
// This is an additional check added by LLVM-libc.
if (to_conv.conv_val_ptr == nullptr)
- return NULLPTR_WRITE_ERROR;
+ return -EINVAL;
#endif // LIBC_COPT_PRINTF_NO_NULLPTR_CHECKS
- int written = writer->get_chars_written();
+ size_t written = writer->get_chars_written();
switch (to_conv.length_modifier) {
case LengthModifier::none:
- *reinterpret_cast<int *>(to_conv.conv_val_ptr) = written;
+ *reinterpret_cast<int *>(to_conv.conv_val_ptr) = static_cast<int>(written);
break;
case LengthModifier::l:
*reinterpret_cast<long *>(to_conv.conv_val_ptr) = written;
diff --git a/libc/src/stdio/printf_core/writer.h b/libc/src/stdio/printf_core/writer.h
index 1d4734a51b9b8..9de108ece510f 100644
--- a/libc/src/stdio/printf_core/writer.h
+++ b/libc/src/stdio/printf_core/writer.h
@@ -127,7 +127,7 @@ template <WriteMode write_mode> struct WriteBuffer {
template <WriteMode write_mode> class Writer final {
WriteBuffer<write_mode> &wb;
- int chars_written = 0;
+ size_t chars_written = 0;
LIBC_INLINE int pad(char new_char, size_t length) {
// First, fill as much of the buffer as possible with the padding char.
@@ -161,7 +161,7 @@ template <WriteMode write_mode> class Writer final {
// Takes a string, copies it into the buffer if there is space, else passes it
// to the overflow mechanism to be handled separately.
LIBC_INLINE int write(cpp::string_view new_string) {
- chars_written += static_cast<int>(new_string.size());
+ chars_written += new_string.size();
if (LIBC_LIKELY(wb.buff_cur + new_string.size() <= wb.buff_len)) {
inline_memcpy(wb.buff + wb.buff_cur, new_string.data(),
new_string.size());
@@ -175,7 +175,7 @@ template <WriteMode write_mode> class Writer final {
// if there is space, else calls pad which will loop and call the overflow
// mechanism on a secondary buffer.
LIBC_INLINE int write(char new_char, size_t length) {
- chars_written += static_cast<int>(length);
+ chars_written += length;
if (LIBC_LIKELY(wb.buff_cur + length <= wb.buff_len)) {
inline_memset(wb.buff + wb.buff_cur, static_cast<unsigned char>(new_char),
@@ -199,7 +199,7 @@ template <WriteMode write_mode> class Writer final {
return wb.overflow_write(char_string_view);
}
- LIBC_INLINE int get_chars_written() { return chars_written; }
+ LIBC_INLINE size_t get_chars_written() { return chars_written; }
};
// Class-template auto deduction helpers.
diff --git a/libc/src/stdio/snprintf.cpp b/libc/src/stdio/snprintf.cpp
index c8940862f711f..6691da1debb5a 100644
--- a/libc/src/stdio/snprintf.cpp
+++ b/libc/src/stdio/snprintf.cpp
@@ -32,10 +32,20 @@ LLVM_LIBC_FUNCTION(int, snprintf,
wb(buffer, (buffsz > 0 ? buffsz - 1 : 0));
printf_core::Writer writer(wb);
- int ret_val = printf_core::printf_main(&writer, format, args);
+ auto ret_val = printf_core::printf_main(&writer, format, args);
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
if (buffsz > 0) // if the buffsz is 0 the buffer may be a null pointer.
wb.buff[wb.buff_cur] = '\0';
- return ret_val;
+
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/sprintf.cpp b/libc/src/stdio/sprintf.cpp
index 7be97d3591aaf..10db9feb80121 100644
--- a/libc/src/stdio/sprintf.cpp
+++ b/libc/src/stdio/sprintf.cpp
@@ -33,9 +33,19 @@ LLVM_LIBC_FUNCTION(int, sprintf,
wb(buffer, cpp::numeric_limits<size_t>::max());
printf_core::Writer writer(wb);
- int ret_val = printf_core::printf_main(&writer, format, args);
+ auto ret_val = printf_core::printf_main(&writer, format, args);
+ if (ret_val.has_error()) {
+ libc_errno = ret_val.error;
+ return -1;
+ }
wb.buff[wb.buff_cur] = '\0';
- return ret_val;
+
+ if (ret_val.value > cpp::numeric_limits<int>::max()) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+
+ return static_cast<int>(ret_val.value);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/vasprintf.cpp b/libc/src/stdio/vasprintf.cpp
index 4a44d4a0f8842..11174727e5d64 100644
--- a/libc/src/stdio/vasprintf.cpp
+++ b/libc/src/stdio/vasprintf.cpp
@@ -18,7 +18,16 @@ LLVM_LIBC_FUNCTION(int, vasprintf,
internal::ArgList args(vlist); // This holder class allo...
[truncated]
|
| constexpr int NULLPTR_WRITE_ERROR = -3; | ||
| constexpr int INT_CONVERSION_ERROR = -4; | ||
| constexpr int FIXED_POINT_CONVERSION_ERROR = -5; | ||
| constexpr int ALLOCATION_ERROR = -6; |
There was a problem hiding this comment.
I'd prefer to use our own internal error numbers until we get to the entrypoint because the list of errno macros that are available depends on the platform. Specifically the C standard only defines 3: EDOM, ERANGE, and EILSEQ. Other error values like ENOMEM and EINVAL are actually from posix and might not be available on all platforms.
There was a problem hiding this comment.
In that case, we still need to have access to the actual errors the syscalls returned for IO errors. What I had in mind is to store the error codes in the libc::File class (instead of just an error flag), and access that in the public entry points, while mapping the other internal errors.
But I'm not sure this either will satisfy the requirement of not handling errnos internally...but I can see the libc::File class already working with errno macros.
I had a go at implementing this, can you take a look? No need to do full review it's WIP, just if it's an OK direction or not.
If we cannot do this, the only other ways I see at the moment is either to map every FILE_WRITE_ERROR to EIO, or have our own errors defined for basically every possible errno with a one to one mapping and map back and forth, but I'm not sure I like either of those.
There was a problem hiding this comment.
The printf formatting engine (basically everything inside /printf_core) is designed to be platform independent, it doesn't actually reference FILE at all except in the Writer class. For LLVM-libc we want to make sure that these pieces are modular so we can support printf on platforms that don't support posix.
The generic FILE needs a rewrite, at least in part to fix the issue you've pointed out about it using errno macros directly.
The error translation function is a good idea, but it can be simplified. It doesn't need to translate every error, just ones that came from within the formatting engine. Currently if the Writer returns a negative number it will be propagated back up to the caller. Additionally, the return value of each write call is either WRITE_OK or the result of calling overflow_write. This means that if you make file_write_hook, the overflow_write implementation for vfprintf, return the negative result of calling ferror_unlocked it will be the return of printf_main as well. That means that the translation function only needs to negate those, not know what the actual value means. For the other errors, I'd recommend changing them to be at least -1000 so they don't overlap with any normal errno value.
There was a problem hiding this comment.
Got it, thanks. I've refactored the file writes to return the FileIOResult, not just the size_t written and now the file write hooks returns negative errnos and non overlapping custom errors as well which are then mapped in public funcs
There was a problem hiding this comment.
the translation function looks good to me now, but it needs to be in its own file to keep the posix specific errors isolated from the OS independent code.
There was a problem hiding this comment.
Where would you have this? Within printf_core in OS specific directories which are conditionally added? Or somewhere more general
There was a problem hiding this comment.
printf_core OS specific subfolders seems like the best option
There was a problem hiding this comment.
I've added specific implementation for linux and a generic one for baremetal and the other platforms. Also added one for darwin, which is the same as the linux one...is that OK? We should be able to provide a POSIX api on MacOS, right? If so, we should probably define EOVERFLOW in our generic error macros?
mleleszi
force-pushed
the
libc-printf-error-handling
branch
from
6abc6e7 to
8b44e2f
Compare
mleleszi
force-pushed
the
libc-printf-error-handling
branch
from
8b44e2f to
6d05774
Compare
mleleszi
force-pushed
the
libc-printf-error-handling
branch
from
4b58abe to
f8cb702
Compare
| return ::fwrite_unlocked(ptr, size, nmemb, f); | ||
| LIBC_INLINE FileIOResult fwrite_unlocked(const void *ptr, size_t size, | ||
| size_t nmemb, ::FILE *f) { | ||
| return {::fwrite_unlocked(ptr, size, nmemb, f), errno}; |
There was a problem hiding this comment.
@michaelrj-google Is it ok to use the global errno here when using the system implementation? Can't see another way of propagating errors in that case
There was a problem hiding this comment.
it should be libc_errno. Also instead of including "hdr/errno_macros.h" at the top add an include for #include "src/__support/libc_errno.h" inside of the ifdef
There was a problem hiding this comment.
Shouldn't we use the system errno in this case? If we are using system file libc_errno won't be set but the system one
There was a problem hiding this comment.
I'm pretty sure that in overlay mode libc_errno is aliased to the system errno for library builds, but is kept separate for tests.
There was a problem hiding this comment.
Ah, so that's why the overlay tests were failing with libc_errno...but if libc_errno is not set in tests, we won't be able to test this case in overlay mode...should I just not check errno in these tests in overlay mode?
| return ::fwrite_unlocked(ptr, size, nmemb, f); | ||
| LIBC_INLINE FileIOResult fwrite_unlocked(const void *ptr, size_t size, | ||
| size_t nmemb, ::FILE *f) { | ||
| return {::fwrite_unlocked(ptr, size, nmemb, f), errno}; |
There was a problem hiding this comment.
it should be libc_errno. Also instead of including "hdr/errno_macros.h" at the top add an include for #include "src/__support/libc_errno.h" inside of the ifdef
| constexpr int NULLPTR_WRITE_ERROR = -3; | ||
| constexpr int INT_CONVERSION_ERROR = -4; | ||
| constexpr int FIXED_POINT_CONVERSION_ERROR = -5; | ||
| constexpr int ALLOCATION_ERROR = -6; |
There was a problem hiding this comment.
the translation function looks good to me now, but it needs to be in its own file to keep the posix specific errors isolated from the OS independent code.
libc/src/stdio/baremetal/printf.cpp
Outdated
|
|
||
| return retval; | ||
| if (retval.value() > cpp::numeric_limits<int>::max()) { | ||
| libc_errno = EOVERFLOW; |
There was a problem hiding this comment.
EOVERFLOW is posix specific, so we don't want to use that for baremetal. For now I'd recommend just setting it to ERANGE since that's C standard. CC: @petrhosek and @saturn691 for their opinions on how baremetal printf error handling should work.
There was a problem hiding this comment.
Got it, mapped it to ERANGE for baremetal
| @@ -43,6 +43,7 @@ | |||
| #define EPIPE 32 | |||
| #define EDOM 33 | |||
| #define ERANGE 34 | |||
| #define EOVERFLOW 75 | |||
There was a problem hiding this comment.
this shouldn't need to be added
| constexpr int NULLPTR_WRITE_ERROR = -3; | ||
| constexpr int INT_CONVERSION_ERROR = -4; | ||
| constexpr int FIXED_POINT_CONVERSION_ERROR = -5; | ||
| constexpr int ALLOCATION_ERROR = -6; |
There was a problem hiding this comment.
printf_core OS specific subfolders seems like the best option
| return ::fwrite_unlocked(ptr, size, nmemb, f); | ||
| LIBC_INLINE FileIOResult fwrite_unlocked(const void *ptr, size_t size, | ||
| size_t nmemb, ::FILE *f) { | ||
| return {::fwrite_unlocked(ptr, size, nmemb, f), errno}; |
There was a problem hiding this comment.
I'm pretty sure that in overlay mode libc_errno is aliased to the system errno for library builds, but is kept separate for tests.
mleleszi
force-pushed
the
libc-printf-error-handling
branch
from
5ffc956 to
5f3b0af
Compare
3 participants
#159474