Add feature "kernel" to C backend#133
Conversation
leo-ard
left a comment
leo-ard
left a comment
There was a problem hiding this comment.
Really good, I love the PR! I have a few comments here, mostly nitpicking or questions I have on the code, maybe we could meet in the lab to discuss further?
|
|
||
| FILE *fdopen(int fd, const char *modes) { | ||
| if (fd == 0 || fd == 1 || fd == 2) { | ||
| return malloc(0x10); |
There was a problem hiding this comment.
It seams odd to return a pointer to uninitialized memory. Looking at the code, it seems like you just want to return any pointer (as file descriptors don't seem to be used at all). But it would be safer to just initialize the structure to mock data
There was a problem hiding this comment.
Actually, I checked a bit more and FILE is an opaque to user code, meaning that in this case, it is valid to use malloc to just return a valid pointer. However, it would be better to do : malloc(sizeof(FILE)) as FILE might change in the future.
| local->next->next = NULL; | ||
| local = local->next; |
There was a problem hiding this comment.
This repeats with the other branch of the if (see "here"). You could extract it out in the englobing if.
| local->next->next = NULL; | ||
| local = local->next; |
| if (position == 80 * 25) { | ||
| uint16_t *vga = (uint16_t *)0xb8000; | ||
| for (int i = 80; i < 80 * 25; i++) | ||
| vga[i - 80] = vga[i]; | ||
| for (int i = 80 * 24; i < 80 * 25; i++) | ||
| vga_write('c', 0x00, i); | ||
| position -= 80; |
There was a problem hiding this comment.
It would probably be better to put the 80 and 25 inside preprocessor variables/C variables if we ever want to change the size of the screen
| fragment->next = local->next; | ||
| fragment->size = local->size - size; | ||
| local->size = size; | ||
| return (void *)(local) + sizeof(local->size); |
There was a problem hiding this comment.
return is the same as in the other branch, maybe put it in the englobing if?
| _string_output_buf = str; | ||
| _string_output_buf_size = size; | ||
| _string_output_len = 0; |
There was a problem hiding this comment.
Is there a reason these variables are global?
| _string_output_buf_size = size; | ||
| _string_output_len = 0; | ||
| result = vfprintf(_standard_files + 3, format, ap); | ||
| _string_output_buf[_string_output_len] = 0; /* null terminate string */ |
There was a problem hiding this comment.
This seems odd, as _string_output_len is always 0, thus always writing the null terminated string at the beginning of the string.
| cp "$1" $dir/repl.c | ||
| cd $dir | ||
| make -f KMakefile kernel.elf | ||
| rm repl.c repl.o |
There was a problem hiding this comment.
Is mk-kernel specialized for the repl only? Ideally, it should work to run any programs using ribbit. Right now, I get the following error:
> ./rsc.exe -t c -l empty -f+ grub-kernel ./tests/00-ribbit/01-putchar.scm -x putchar
/Users/leonard/udem-dlteam/ribbit/src/host/c/ /Users/leonard/udem-dlteam/ribbit/src/host/c/mk-kernel
clang -m32 -nostdlib -fno-builtin -fno-stack-protector -nostartfiles -nodefaultlibs -c -Oz repl.c -o repl.o
clang: warning: argument unused during compilation: '-nostartfiles' [-Wunused-command-line-argument]
repl.c:676:12: error: use of undeclared identifier 'stdout'
676 | fflush(stdout);
| ^
1 error generated.
make: *** [repl.o] Error 1
rm: repl.o: No such file or directory
mv: /Users/leonard/udem-dlteam/ribbit/src/host/c//kernel.elf: No such file or directory
*** ERROR IN generate-code -- Cannot generating executable, returned status 256
2 participants
Description
the following PR adds a feature named kernel and accessible via -f+ kernel to the C backend of ribbit scheme
It allows to compile scheme source code to a GRUB-bootable kernel.
Only basic stdin/stdout IO is available at the time of writing.