-
Notifications
You must be signed in to change notification settings - Fork 1
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
Read stdin initialized file #8
base: dramfs
Are you sure you want to change the base?
Conversation
@@ -6,35 +6,40 @@ | |||
/* Read from a file. */ | |||
ssize_t _read(int fd, void *ptr, size_t len) | |||
{ | |||
if(dramfs_check_fd(fd) < 0) { | |||
return -1; | |||
if (dramfs_check_fd(fd) < 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.
This seems like a semantic change. If check_fd < 0, we used to return -1. What's the difference in behavior?
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.
If no stdin was created in the lfs.c file(we're building as baremetal), check_fd is going to return -1, but we might want to read the host getchar.
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.
I'm think I'm missing something here. The MKLFS tool doesn't always define the STDIN/STDOUT to exist in lfs.c?
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.
MKLFS always will create an stdin in lfs.c. If we're using BAREMETAL perch then we're not using any lfs.c files so there's no stdin file descriptor.
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.
Okay, so:
- if we do have an LFS.c, there is no change
- If we do not have an LFS.c:
- previously, we would return -1
- now, we attempt to do host_getchar regardless?
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.
Not exactly. If there is an lfs.c, we read the stdin file before doing the host_getchar. If not, we would only do host_getchar. Before this change, we only did host_getchar if there was a stdin file and we didn't actually read the file.
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.
Yeah I'm just talking about this if statement right now. If we don't call dramfs_init, will this go to host read or fail? Can we call dramfs_init without an LFS? I'm trying to nail down does this change fully enable users to use stdin/stdout without messing with lfs or will something down the line fail because of not having LFS or compiling with BAREMETAL
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.
If we don't call dramfs_init this will go to host_getchar. We cannot call the non-dummy dramfs_init without an lfs.c, for this reqason there's a default lfs.c with just the stdin/out/err in the repo. This if statement convers the BAREMETAL case. The other if statment after lfs_file_read covers the default lfs.c case.
I think the normal way to handle this is to redefine STDIN from the 0 input stream to the FD of the file. Is there a reason not to do this? |
The FD of the file is already 0. Here we're just making sure to read that file before doing the dramfs_getchar. |
|
||
return (ssize_t) i; | ||
lfs_file_t *fptr = dramfs_get_file(fd); | ||
ssize_t size = (ssize_t) lfs_file_read(&dramfs_fs, fptr, ptr, (lfs_size_t) len); |
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.
lfs_file_read will always return 0 for stdin and have no side effects? (an actual read?)
Yeah I think that is non-standard. Normally stdout/stdin/stderr are fixed. Then an explicit syscall remaps calls to that fd to the file instead of stdout/in. This can be taken care of in the start code for the spec benchmarks in question. http://www.cs.loyola.edu/~jglenn/702/S2005/Examples/dup2.html
Basically this implementation is doing an implicit dup2 on nonzero size and then switching to stdin once the file is empty. My concern is that implicit behavior like this can be tricky to track down. When we fall off the end of the file, we would just start reading from stdin instead, which could cause hangs or unexpected problems. Is there ever a case when we would want to read from both stdin and a stdin.txt? I don't think so, so it makes more sense to me have to make an explicit choice to read from one or the other. It can't be a compile time option because this is a linked library, so an explicit syscall seems like the best option |
The file is just a way to pre-fill the stdin buffer with data during build. If the program needs more data from stdin it will hang until it's provided by the host MMIO like any other program. I don't see how's this a behavior change. Also, we don't want to add hardcoded syscalls in the code to dup2 whatever input file is given to every program. The code is not reading from an FD!=0 but from the actual stdin. |
We create an initialized stdin file for certain benchmarks to feed them their input without manually using the terminal. This change reads that file before looking for new characters from the host MMIO.