-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc] Add putc, fputc, and fprintf to stdio/baremetal #144567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Also groups the *_write_hook into a new header file which can be extended to easily support other [v][f]printf functions
|
@llvm/pr-subscribers-libc Author: William Huynh (saturn691) ChangesAlso groups the *_write_hook into a new header file which can be extended to easily support other [v][f]printf functions Full diff: https://github.com/llvm/llvm-project/pull/144567.diff 11 Files Affected:
diff --git a/libc/config/baremetal/aarch64/entrypoints.txt b/libc/config/baremetal/aarch64/entrypoints.txt
index a8e653fdd5159..08b143d045463 100644
--- a/libc/config/baremetal/aarch64/entrypoints.txt
+++ b/libc/config/baremetal/aarch64/entrypoints.txt
@@ -124,8 +124,11 @@ set(TARGET_LIBC_ENTRYPOINTS
# stdio.h entrypoints
libc.src.stdio.asprintf
+ libc.src.stdio.fprintf
+ libc.src.stdio.fputc
libc.src.stdio.getchar
libc.src.stdio.printf
+ libc.src.stdio.putc
libc.src.stdio.putchar
libc.src.stdio.puts
libc.src.stdio.remove
diff --git a/libc/config/baremetal/arm/entrypoints.txt b/libc/config/baremetal/arm/entrypoints.txt
index acafef17fa5d1..276f00a04b16a 100644
--- a/libc/config/baremetal/arm/entrypoints.txt
+++ b/libc/config/baremetal/arm/entrypoints.txt
@@ -124,8 +124,11 @@ set(TARGET_LIBC_ENTRYPOINTS
# stdio.h entrypoints
libc.src.stdio.asprintf
+ libc.src.stdio.fprintf
+ libc.src.stdio.fputc
libc.src.stdio.getchar
libc.src.stdio.printf
+ libc.src.stdio.putc
libc.src.stdio.putchar
libc.src.stdio.puts
libc.src.stdio.remove
diff --git a/libc/config/baremetal/riscv/entrypoints.txt b/libc/config/baremetal/riscv/entrypoints.txt
index 023826f12d723..dc173f69ce554 100644
--- a/libc/config/baremetal/riscv/entrypoints.txt
+++ b/libc/config/baremetal/riscv/entrypoints.txt
@@ -124,8 +124,11 @@ set(TARGET_LIBC_ENTRYPOINTS
# stdio.h entrypoints
libc.src.stdio.asprintf
+ libc.src.stdio.fprintf
+ libc.src.stdio.fputc
libc.src.stdio.getchar
libc.src.stdio.printf
+ libc.src.stdio.putc
libc.src.stdio.putchar
libc.src.stdio.puts
libc.src.stdio.remove
diff --git a/libc/src/stdio/baremetal/CMakeLists.txt b/libc/src/stdio/baremetal/CMakeLists.txt
index e879230a9d02c..ba1b0a690844a 100644
--- a/libc/src/stdio/baremetal/CMakeLists.txt
+++ b/libc/src/stdio/baremetal/CMakeLists.txt
@@ -1,3 +1,27 @@
+add_entrypoint_object(
+ fprintf
+ SRCS
+ fprintf.cpp
+ HDRS
+ ../fprintf.h
+ write_utils.h
+ DEPENDS
+ libc.src.__support.OSUtil.osutil
+ libc.src.__support.CPP.string_view
+ libc.src.stdio.printf_core.printf_main
+)
+
+add_entrypoint_object(
+ fputc
+ SRCS
+ fputc.cpp
+ HDRS
+ ../fputc.h
+ write_utils.h
+ DEPENDS
+ libc.src.__support.CPP.string_view
+)
+
add_entrypoint_object(
getchar
SRCS
@@ -20,12 +44,24 @@ add_entrypoint_object(
libc.include.stdio
)
+add_entrypoint_object(
+ putc
+ SRCS
+ putc.cpp
+ HDRS
+ ../putc.h
+ write_utils.h
+ DEPENDS
+ libc.src.__support.CPP.string_view
+)
+
add_entrypoint_object(
printf
SRCS
printf.cpp
HDRS
../printf.h
+ write_utils.h
DEPENDS
libc.src.stdio.printf_core.printf_main
libc.src.stdio.printf_core.writer
@@ -102,3 +138,16 @@ add_entrypoint_object(
libc.src.__support.arg_list
libc.src.__support.OSUtil.osutil
)
+
+add_header_library(
+ baremetal_write_utils
+ HDRS
+ write_utils.h
+ DEPENDS
+ libc.hdr.types.FILE
+ libc.hdr.stdio_macros
+ libc.src.__support.OSUtil.osutil
+ libc.src.__support.CPP.string_view
+ .stdout
+ .stderr
+)
\ No newline at end of file
diff --git a/libc/src/stdio/baremetal/fprintf.cpp b/libc/src/stdio/baremetal/fprintf.cpp
new file mode 100644
index 0000000000000..530c811af5652
--- /dev/null
+++ b/libc/src/stdio/baremetal/fprintf.cpp
@@ -0,0 +1,46 @@
+//===-- Implementation of fprintf for baremetal -----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/fprintf.h"
+
+#include "hdr/types/FILE.h"
+#include "src/__support/arg_list.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/baremetal/write_utils.h"
+#include "src/stdio/printf_core/printf_main.h"
+
+#include <stdarg.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, fprintf,
+ (::FILE *__restrict stream, const char *__restrict format,
+ ...)) {
+ va_list vlist;
+ va_start(vlist, format);
+ internal::ArgList args(vlist); // This holder class allows for easier copying
+ // and pointer semantics, as well as handling
+ // destruction automatically.
+ va_end(vlist);
+ static constexpr size_t BUFF_SIZE = 1024;
+ char buffer[BUFF_SIZE];
+
+ printf_core::WriteBuffer<printf_core::WriteMode::FLUSH_TO_STREAM> wb(
+ buffer, BUFF_SIZE, get_write_hook(stream), nullptr);
+ printf_core::Writer<printf_core::WriteMode::FLUSH_TO_STREAM> writer(wb);
+
+ int retval = printf_core::printf_main(&writer, format, args);
+
+ int flushval = wb.overflow_write("");
+ if (flushval != printf_core::WRITE_OK)
+ retval = flushval;
+
+ return retval;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/baremetal/fputc.cpp b/libc/src/stdio/baremetal/fputc.cpp
new file mode 100644
index 0000000000000..e33bbc663b970
--- /dev/null
+++ b/libc/src/stdio/baremetal/fputc.cpp
@@ -0,0 +1,25 @@
+//===-- Baremetal Implementation of fputc ---------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/fputc.h"
+
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/baremetal/write_utils.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, fputc, (int c, ::FILE *stream)) {
+ char uc = static_cast<char>(c);
+
+ write(stream, cpp::string_view(&uc, 1));
+
+ return 0;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/baremetal/printf.cpp b/libc/src/stdio/baremetal/printf.cpp
index 7253c6549a4e4..5a320cc232456 100644
--- a/libc/src/stdio/baremetal/printf.cpp
+++ b/libc/src/stdio/baremetal/printf.cpp
@@ -7,27 +7,17 @@
//===----------------------------------------------------------------------===//
#include "src/stdio/printf.h"
-#include "src/__support/OSUtil/io.h"
+
#include "src/__support/arg_list.h"
#include "src/__support/macros/config.h"
-#include "src/stdio/printf_core/core_structs.h"
+#include "src/stdio/baremetal/write_utils.h"
#include "src/stdio/printf_core/printf_main.h"
-#include "src/stdio/printf_core/writer.h"
#include <stdarg.h>
#include <stddef.h>
namespace LIBC_NAMESPACE_DECL {
-namespace {
-
-LIBC_INLINE int stdout_write_hook(cpp::string_view new_str, void *) {
- write_to_stdout(new_str);
- return printf_core::WRITE_OK;
-}
-
-} // namespace
-
LLVM_LIBC_FUNCTION(int, printf, (const char *__restrict format, ...)) {
va_list vlist;
va_start(vlist, format);
diff --git a/libc/src/stdio/baremetal/putc.cpp b/libc/src/stdio/baremetal/putc.cpp
new file mode 100644
index 0000000000000..8150ae21eda23
--- /dev/null
+++ b/libc/src/stdio/baremetal/putc.cpp
@@ -0,0 +1,25 @@
+//===-- Baremetal Implementation of putc ----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/putc.h"
+
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/baremetal/write_utils.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, putc, (int c, ::FILE *stream)) {
+ char uc = static_cast<char>(c);
+
+ write(stream, cpp::string_view(&uc, 1));
+
+ return 0;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/baremetal/putchar.cpp b/libc/src/stdio/baremetal/putchar.cpp
index ac21e6e783b01..fcaae40d09397 100644
--- a/libc/src/stdio/baremetal/putchar.cpp
+++ b/libc/src/stdio/baremetal/putchar.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "src/stdio/putchar.h"
+
#include "src/__support/CPP/string_view.h"
#include "src/__support/OSUtil/io.h"
#include "src/__support/macros/config.h"
@@ -14,7 +15,7 @@
namespace LIBC_NAMESPACE_DECL {
LLVM_LIBC_FUNCTION(int, putchar, (int c)) {
- char uc = static_cast<char>(c);
+ char uc = static_cast<unsigned char>(c);
write_to_stdout(cpp::string_view(&uc, 1));
diff --git a/libc/src/stdio/baremetal/vprintf.cpp b/libc/src/stdio/baremetal/vprintf.cpp
index ab02533f14911..38985a9b85209 100644
--- a/libc/src/stdio/baremetal/vprintf.cpp
+++ b/libc/src/stdio/baremetal/vprintf.cpp
@@ -7,9 +7,11 @@
//===----------------------------------------------------------------------===//
#include "src/stdio/vprintf.h"
+
#include "src/__support/OSUtil/io.h"
#include "src/__support/arg_list.h"
#include "src/__support/macros/config.h"
+#include "src/stdio/baremetal/write_utils.h"
#include "src/stdio/printf_core/core_structs.h"
#include "src/stdio/printf_core/printf_main.h"
#include "src/stdio/printf_core/writer.h"
@@ -19,15 +21,6 @@
namespace LIBC_NAMESPACE_DECL {
-namespace {
-
-LIBC_INLINE int stdout_write_hook(cpp::string_view new_str, void *) {
- write_to_stdout(new_str);
- return printf_core::WRITE_OK;
-}
-
-} // namespace
-
LLVM_LIBC_FUNCTION(int, vprintf,
(const char *__restrict format, va_list vlist)) {
internal::ArgList args(vlist); // This holder class allows for easier copying
diff --git a/libc/src/stdio/baremetal/write_utils.h b/libc/src/stdio/baremetal/write_utils.h
new file mode 100644
index 0000000000000..ce493189c472a
--- /dev/null
+++ b/libc/src/stdio/baremetal/write_utils.h
@@ -0,0 +1,48 @@
+//===-- Baremetal helper functions for writing to stdout/stderr -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "hdr/stdio_macros.h" // For stdout/err
+#include "hdr/types/FILE.h"
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/OSUtil/io.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/printf_core/core_structs.h" // For printf_core::WRITE_OK
+
+#include <stddef.h>
+
+namespace LIBC_NAMESPACE_DECL {
+namespace {
+
+LIBC_INLINE int stdout_write_hook(cpp::string_view new_str, void *) {
+ write_to_stdout(new_str);
+ return printf_core::WRITE_OK;
+}
+
+LIBC_INLINE int stderr_write_hook(cpp::string_view new_str, void *) {
+ write_to_stderr(new_str);
+ return printf_core::WRITE_OK;
+}
+
+LIBC_INLINE void write(::FILE *f, cpp::string_view new_str) {
+ if (f == stdout) {
+ write_to_stdout(new_str);
+ } else {
+ write_to_stderr(new_str);
+ }
+}
+
+LIBC_INLINE decltype(&stdout_write_hook) get_write_hook(::FILE *f) {
+ if (f == stdout) {
+ return &stdout_write_hook;
+ }
+
+ return &stderr_write_hook;
+}
+
+} // namespace
+} // namespace LIBC_NAMESPACE_DECL
|
0b4dbbc to
20b1e65
Compare
|
What's the motivation for this change? Do you have a concrete use cases or are you just filling in blanks? To give you some context, we omitted the |
| if (f == stdout) | ||
| return &stdout_write_hook; | ||
|
|
||
| return &stderr_write_hook; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here, if the stream is neither stdout nor stderr, we shouldn't just implicitly fallback to stderr, we should return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fprintf function returns the number of characters transmitted, or a negative value if an output
or encoding error occurred or if the implementation does not support a specified width length
modifier.
Reference: C23, 7.23.6.1 The printf function, paragraph 15
I've chosen to return -1 in the fprintf function (which calls this function), but nullptr here.
| ) | ||
|
|
||
| add_header_library( | ||
| baremetal_write_utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not being used anywhere (see the comments above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using .baremetal_*, this should be resolved
|
Hi Petr, I'm going to take this conversation to Discourse. It would be great if we can some consensus/input before I continue working on this PR. https://discourse.llvm.org/t/rfc-implementation-of-stdio-on-baremetal/86944 |
…f to stdio/baremetal
… fprintf to stdio/baremetal
|
Hi guys, re-requesting review. |
michaelrj-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but Petr should probably approve since this is likely to affect his team's usage.
SanjitRaman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
I am considering closing this PR, please see the attached RFC for more details. https://discourse.llvm.org/t/rfc-implementation-of-stdio-on-baremetal/86944/4 |
These functions are required for some downstream testing, specifically writing to stdout/stderr. This does not introduce any new support for writing to a stream other than stdout/stderr, and will cause undefined behaviour. Part of #146099.