[ATfE] Prototype: LLVM libc file IO support - #1011
Conversation
This is a prototype/RFC for adding file IO support to bare-metal LLVM libc.
LLVM libc changes are intended to land in upstream LLVM - it is easier to show
how the proposed interface is used with ATfE semihosting implementation though.
LLVM libc changes: The embedding API libc/src/__support/OSUtil/baremetal/io.h
is extended with these functions:
- __llvm_libc_stdio_open
- __llvm_libc_stdio_close
- __llvm_libc_stdio_seek
- __llvm_libc_stdio_set_buffer
- __llvm_libc_stdio_flush
which map naturally to, e.g. the API expected by picolibc.
These are used to implement the following additional C library functions
that are enough to implement file IO required by libc++ fstream:
- fopen
- fclose
- fseek
- fseeko
- ftell
- ftello
- setbuf
- setvbuf
- fflush
The embedding API uses opaque pointer as a cookie, the semihosting implementation
then keeps a small struct with either just the semihosting handle for standard
streams or also current position to support seeking for file streams.
Semihosting implementation is unbuffered, however other implementations can
provide it by default or through the buffering management functions
(which are also required by C++ fstream).
Other implementations can use the struct of function pointers pattern for the
cookie to easily implement support for multiple types of file systems, e.g. UART,
flash or FAT.
The RFC is mainly about the embedding API and libc side implementation if it is
reasonable for upstream LLVM libc.
Note: There is a TODO comment whether we need embedding API for ungetc, what we
can do is declare a weak hook:
__llvm_libc_stdio_ungetc(void *cookie, unsigned char ch)
and check in the current ungetc implementation if it is provided:
use it if yes or keep current 1 char buffer otherwise.
|
This pull review modifies files outside of the |
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
Some small nits, but overall looks good to me. Maybe worth @vhscampos to look over the llvmlibc parts.
| namespace { | ||
| // Helper functions implemented here to avoid dependency on libc which is not | ||
| // available in LLVM libc hermetic testing. | ||
| static int _isspace(char ch) { |
There was a problem hiding this comment.
This is in an anonymous namespace already. Does it need static?
There was a problem hiding this comment.
Oh, good spot: this is a left over the previous implementation of argv parsing. Removed.
| reinterpret_cast<size_t>(path), | ||
| mode, | ||
| sizeof(std_stream_name) - 1UL, | ||
| _strlen(path), |
There was a problem hiding this comment.
Could make length a parameter to make the caller pass it in. Would save a call to _strlen when the length can be statically determined, for example :tt.
There was a problem hiding this comment.
Good point - updated.
- Remove _cookie suffix from helper functions. - Remove unnecessary static declaration. - Move call to _strlen() out of semihost_open(). - Avoid negative offsets for advance_file_position().
voltur01
left a comment
voltur01
left a comment
There was a problem hiding this comment.
Thank you for the review! Please see updates.
| namespace { | ||
| // Helper functions implemented here to avoid dependency on libc which is not | ||
| // available in LLVM libc hermetic testing. | ||
| static int _isspace(char ch) { |
There was a problem hiding this comment.
Oh, good spot: this is a left over the previous implementation of argv parsing. Removed.
| reinterpret_cast<size_t>(path), | ||
| mode, | ||
| sizeof(std_stream_name) - 1UL, | ||
| _strlen(path), |
There was a problem hiding this comment.
Good point - updated.
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
Thanks LGTM on my side. I've set approved, but could be worth waiting a day or so to see if there are any other comments from other reviewers.
2 participants
This is a prototype/RFC for adding file IO support to bare-metal LLVM libc. LLVM libc changes are intended to land in upstream LLVM - it is easier to show how the proposed interface is used with ATfE semihosting implementation though.
LLVM libc changes: The embedding API libc/src/__support/OSUtil/baremetal/io.h is extended with these functions:
These are used to implement the following additional C library functions that are enough to implement file IO required by libc++ fstream:
The embedding API uses opaque pointer as a cookie, the semihosting implementation then keeps a small struct with either just the semihosting handle for standard streams or also current position to support seeking for file streams.
Semihosting implementation is unbuffered, however other implementations can provide it by default or through the buffering management functions (which are also required by C++ fstream).
Other implementations can use the struct of function pointers pattern for the cookie to easily implement support for multiple types of file systems, e.g. UART, flash or FAT.
The RFC is mainly about the embedding API and libc side implementation if it is reasonable for upstream LLVM libc.
Note: There is a TODO comment whether we need embedding API for ungetc, what we can do is declare a weak hook:
and check in the current ungetc implementation if it is provided: use it if yes or keep current 1 char buffer otherwise.