-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[libc] Allow using sscanf() and vsscanf() on baremetal targets. #130527
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
In addition to the normal "cookie" definitions a baremetal user needs to provide to use printf(), a user also needs to add "FILE *stdin;" somehere in their source. I have not updated the default baremetal entrypoints, so a user will currently need to supply their own to add sscanf() and vsscanf(). This might also work for regular sscanf(), but I have not yet tried it. This updates LLVMCCompileOptionRules.cmake to add a macro the libc build can use to determine if it is doing a baremetal build. This also updates the baremetal getchar() function because it looked like it would return an uninitialized value if __llvm_libc_stdio_read() returned 0. The baremetal build does not provide ungetc(), so the baremetal Reader class tracks the would-be ungot character itself. From what I could see in the code, tracking only one character is sufficient.
@llvm/pr-subscribers-libc Author: Jesse D (jdeguire) ChangesIn addition to the normal "cookie" definitions a baremetal user needs to provide to use printf(), a user also needs to add This updates LLVMCCompileOptionRules.cmake to add a macro the libc build can use to determine if it is doing a baremetal build. This also updates the baremetal getchar() function because it looked like it would return an uninitialized value if __llvm_libc_stdio_read() returned 0. The baremetal build does not provide ungetc(), so the baremetal Reader class tracks the would-be ungot character itself. From what I could see in the code, tracking only one character is sufficient. Full diff: https://github.com/llvm/llvm-project/pull/130527.diff 4 Files Affected:
diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 0facb0b9be0c1..270a74a6949c5 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -119,6 +119,9 @@ function(_get_common_compile_options output_var flags)
list(APPEND compile_options "-fpie")
if(LLVM_LIBC_FULL_BUILD)
+ if(LIBC_TARGET_OS_IS_BAREMETAL)
+ list(APPEND compile_options "-DLIBC_TARGET_OS_IS_BAREMETAL")
+ endif()
# Only add -ffreestanding flag in non-GPU full build mode.
if(NOT LIBC_TARGET_OS_IS_GPU)
list(APPEND compile_options "-ffreestanding")
diff --git a/libc/src/stdio/baremetal/getchar.cpp b/libc/src/stdio/baremetal/getchar.cpp
index 0a78a1ff8adf1..8fb7bc32537e7 100644
--- a/libc/src/stdio/baremetal/getchar.cpp
+++ b/libc/src/stdio/baremetal/getchar.cpp
@@ -17,7 +17,7 @@ namespace LIBC_NAMESPACE_DECL {
LLVM_LIBC_FUNCTION(int, getchar, ()) {
char buf[1];
auto result = read_from_stdin(buf, sizeof(buf));
- if (result < 0)
+ if (result <= 0)
return EOF;
return buf[0];
}
diff --git a/libc/src/stdio/scanf_core/CMakeLists.txt b/libc/src/stdio/scanf_core/CMakeLists.txt
index 014413ccaa8da..79ae87bf3dd75 100644
--- a/libc/src/stdio/scanf_core/CMakeLists.txt
+++ b/libc/src/stdio/scanf_core/CMakeLists.txt
@@ -16,6 +16,10 @@ if(LIBC_TARGET_OS_IS_GPU)
libc.src.stdio.ungetc
libc.src.stdio.ferror
)
+elseif(LIBC_TARGET_OS_IS_BAREMETAL)
+ list(APPEND file_deps
+ libc.src.stdio.getchar
+ )
elseif(LLVM_LIBC_FULL_BUILD)
list(APPEND file_deps
libc.src.__support.File.file
@@ -54,13 +58,6 @@ add_header_library(
libc.src.__support.CPP.string_view
)
-if(NOT(TARGET libc.src.__support.File.file) AND LLVM_LIBC_FULL_BUILD AND
- (NOT LIBC_TARGET_OS_IS_GPU))
- # Not all platforms have a file implementation. If file is unvailable, and a
- # full build is requested, then we must skip all file based scanf sections.
- return()
-endif()
-
add_object_library(
scanf_main
SRCS
@@ -117,6 +114,13 @@ add_object_library(
${use_system_file}
)
+if(NOT(TARGET libc.src.__support.File.file) AND LLVM_LIBC_FULL_BUILD AND
+ (NOT LIBC_TARGET_OS_IS_GPU))
+ # Not all platforms have a file implementation. If file is unvailable, and a
+ # full build is requested, then we must skip all file based scanf sections.
+ return()
+endif()
+
#TODO: condense the file-related code as possible.
add_header_library(
vfscanf_internal
diff --git a/libc/src/stdio/scanf_core/reader.h b/libc/src/stdio/scanf_core/reader.h
index 1f8ec9695a314..281dabe7faf01 100644
--- a/libc/src/stdio/scanf_core/reader.h
+++ b/libc/src/stdio/scanf_core/reader.h
@@ -11,13 +11,16 @@
#include "hdr/types/FILE.h"
-#ifndef LIBC_COPT_STDIO_USE_SYSTEM_FILE
+#if !defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE) && !defined(LIBC_TARGET_OS_IS_BAREMETAL)
#include "src/__support/File/file.h"
#endif
#if defined(LIBC_TARGET_ARCH_IS_GPU)
#include "src/stdio/getc.h"
#include "src/stdio/ungetc.h"
+#elif defined(LIBC_TARGET_OS_IS_BAREMETAL)
+#include "src/stdio/getchar.h"
+#include "hdr/stdio_macros.h" // for EOF.
#endif
#include "src/__support/macros/attributes.h" // For LIBC_INLINE
@@ -47,6 +50,28 @@ LIBC_INLINE void ungetc(int c, void *f) {
LIBC_NAMESPACE::ungetc(c, reinterpret_cast<::FILE *>(f));
}
+#elif defined(LIBC_TARGET_OS_IS_BAREMETAL)
+// The baremetal build does not currently support file operations, but it does
+// declare pointers to the stanard FILE streams. The user just needs to declare
+// "FILE *stdin;" somwhere. That is not much to ask since a user already has to
+// define cookie structures for stdio.
+extern "C" FILE *stdin;
+
+LIBC_INLINE int getc(void *f) {
+ if (f == stdin) {
+ int c = getchar();
+ if (EOF == c)
+ c = '\0';
+
+ return c;
+ }
+
+ return '\0';
+}
+
+// Baremetal does not currently provide an ungetc(), so the Reader will need to
+// handle that for now.
+
#elif !defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)
LIBC_INLINE int getc(void *f) {
@@ -89,6 +114,10 @@ class Reader {
ReadBuffer *rb;
void *input_stream = nullptr;
size_t cur_chars_read = 0;
+#if defined(LIBC_TARGET_OS_IS_BAREMETAL)
+ // Baremetal does not provide an ungetc(), so track that ourselves for now.
+ int unget_char = EOF;
+#endif
public:
// TODO: Set buff_len with a proper constant
@@ -107,6 +136,13 @@ class Reader {
++(rb->buff_cur);
return output;
}
+#if defined(LIBC_TARGET_OS_IS_BAREMETAL)
+ if (EOF != unget_char) {
+ char output = (char)unget_char;
+ unget_char = EOF;
+ return output;
+ }
+#endif
// This should reset the buffer if applicable.
return static_cast<char>(reader_internal::getc(input_stream));
}
@@ -123,7 +159,11 @@ class Reader {
--(rb->buff_cur);
return;
}
+#if defined(LIBC_TARGET_OS_IS_BAREMETAL)
+ unget_char = c;
+#else
reader_internal::ungetc(static_cast<int>(c), input_stream);
+#endif
}
LIBC_INLINE size_t chars_read() { return cur_chars_read; }
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Just as a note, I was able to test this on my own baremetal ARM configuration using a simple test file here. This test is very specific to my configuration and I'm not sure if I can make something more generic that would need to be included with libc. |
Thank you for the PR, I'm working on a similar change to support |
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.
Thanks for this patch, and I'm really excited to learn about your project using our code!
I think the design might need some tweaks to fit our overall design but I'm very interested in making printf and scanf work better for baremetal.
if(LIBC_TARGET_OS_IS_BAREMETAL) | ||
list(APPEND compile_options "-DLIBC_TARGET_OS_IS_BAREMETAL") | ||
endif() |
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.
When possible we prefer to have compile options based on capabilities instead of targets. In this specific case we'd probably want something similar to LIBC_COPT_STDIO_USE_SYSTEM_FILE
, which is an existing flag that changes printf/scanf behavior to use another libc's FILE* functions. It's also defined closer to where it's used, in the stdio CMake file, which I think would also be good here.
#elif defined(LIBC_TARGET_OS_IS_BAREMETAL) | ||
#include "hdr/stdio_macros.h" // for EOF. | ||
#include "src/stdio/getchar.h" |
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.
We try to avoid having libc functions depend on each other where possible. The GPU build is a special case (that I'd like to clean up at some point) but for baremetal I think just calling read_from_stdin
should work fine instead of going through the public entrypoint. That way you also don't have to declare stdin
in the middle of an unrelated file.
#if defined(LIBC_TARGET_OS_IS_BAREMETAL) | ||
// Baremetal does not provide an ungetc(), so track that ourselves for now. | ||
int unget_char = EOF; | ||
#endif |
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 char from ungetc can be ignored since we never modify it, so you can skip most of the extra handling there.
LIBC_INLINE int getc(void *f) { | ||
if (f == stdin) { | ||
int c = getchar(); | ||
if (EOF == c) | ||
c = '\0'; | ||
|
||
return c; | ||
} | ||
|
||
return '\0'; | ||
} |
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.
Ideally this code would go in baremetal/scanf.cpp
(which I realize doesn't exist yet but it probably should). This gives me an idea for how to solve this problem in a more general way, I'll see if I can get a basic proof of concept patch up.
Oh, excellent! In that case, I'm totally fine with closing this PR or whatever I need to do to end it. I look forward to using your scanf() stuff! I didn't want to blindly call Are there plans to eventually add FILE support to baremetal libc? It's not critical, of course, but it could be useful to let us baremetal folks translate between standard file IO functions and vendor-supplied ones. Think having fprintf() call into FileX or something like that. The current "ccokie" types for stdio might work--I wonder if I can do Also, to build libc and libc++ together I have to manually add a few file IO declarations to LLVM-libc's stdio.h (Exhibit A then in my app add a few very trivial implementations (Exhibit B). This does what I need for now, so I'm all good here. |
Thanks! I'm really just trying to use Clang, libc, and libc++ to build for Microchip PIC32 ARM microcontrollers. I appreciate that you guys are supporting baremetal environments while providing a modern development experience with up-to-date C and C++ standards support. I have a build script here to create a whole toolchain. Like I mentioned in my previous reply, I can use both libc and libc++ together (at least in what little testing I've done so far) with just a couple of extra steps to add a few trivial file IO functions. Maybe I can try documenting what I've done--Peter Smith's FOSDEM 2025 presentation was very helpful. Anyway, I'd better stop getting this PR thread off track. It looks like @petrhosek is already making progress on scanf() support, so I'm okay closing this PR and waiting for his support to land. Your comments are still helpful, so I'll look them over and keep them in mind should I make a future PR for libc. Thanks again! |
Proper scanf() support on baremetal was added with PR #131043, so I'm happy to close this PR and use that! |
Glad the problem has been solved! |
In addition to the normal "cookie" definitions a baremetal user needs to provide to use printf(), a user also needs to add
FILE *stdin;
somewhere in their source. I have not updated the default baremetal entrypoints, so a user will currently need to supply their own to add sscanf() and vsscanf(). This might also work for regular sscanf(), but I have not yet tried it.This updates LLVMCCompileOptionRules.cmake to add a macro the libc build can use to determine if it is doing a baremetal build. This also updates the baremetal getchar() function because it looked like it would return an uninitialized value if __llvm_libc_stdio_read() returned 0. The baremetal build does not provide ungetc(), so the baremetal Reader class tracks the would-be ungot character itself. From what I could see in the code, tracking only one character is sufficient.