SimpleOS Consolidated - #19
Conversation
The kernel.c and kernel.h contains the complete .c and .h code respectively.
WalkthroughThis PR restructures the build system with cross-compiler awareness, introduces a comprehensive kernel.h header declaring extensive APIs across graphics, synchronization, memory management, task scheduling, and I/O, and updates the kernel entry point and linker script to align with the new stack initialization and naming conventions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
kernel.h (3)
109-111: Minor: Usefalseinstead of0for boolean initialization.For consistency with the
booltype declaration, preferfalseover0.🔎 Suggested fix
static inline void spinlock_init(spinlock_t* lock) { - lock->locked = 0; + lock->locked = false; }
498-502: Consider defining constants forfs_entry_tflags.Using magic numbers
0and1for file types can be error-prone. Consider an enum or defined constants.🔎 Suggested improvement
+typedef enum { + FS_TYPE_FILE = 0, + FS_TYPE_DIRECTORY = 1 +} fs_type_t; + typedef struct { char name[128]; uint32_t size; - uint32_t flags; // 0=File, 1=Directory + fs_type_t type; } fs_entry_t;
477-489:task_tis missing anamefield.The
create_taskfunction accepts achar* nameparameter, but the implementation explicitly discards it—marked as "for debugging, unused for now." If task naming is intended for identification, logging, or debugging, add a name field to the struct to store and utilize it.create_iso.sh (1)
1-28: Consider removing duplicated build logic.This script duplicates the entire Makefile build pipeline. Having two build systems creates maintenance burden and risk of divergence (the linker flag discrepancy above is an example). Consider either:
- Removing
create_iso.shand usingmakedirectly, or- Having the script simply invoke
make all🔎 Simplified script
#!/bin/bash set -e make clean make all echo "Build Complete: simpleos.iso"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)create_iso.sh(1 hunks)kernel.h(1 hunks)kernel_entry.asm(1 hunks)linker.ld(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 85-85: Missing required phony target "test"
(minphony)
🔇 Additional comments (6)
linker.ld (1)
1-28: Linker script structure looks correct.The entry point
_startaligns withkernel_entry.asm, and the 1MB load address is appropriate for protected mode kernels. The section definitions are functional.One consideration: The previous version reportedly had
BLOCK(4K) : ALIGN(4K)directives. While not strictly required for basic functionality, explicit 4KB alignment for.text,.rodata, and.bsssections can improve page-table mapping efficiency and prevent potential issues with memory protection if you later enable NX bits or read-only data pages.kernel.h (1)
1-535: Monolithic header approach noted.Consolidating all kernel declarations into a single header is acceptable for a learning project but may cause longer compile times as the project grows (any change triggers full recompilation). The clear section comments (
// 1. IO.H,// 2. GRAPHICS.H, etc.) suggest these could be split into separate headers if needed later.The overall structure and declarations are well-organized.
kernel_entry.asm (2)
18-20: Stack reservation is correctly ordered.The stack grows downward on x86, so
kernel_stack_topcorrectly labels the address immediately after the 8KB reservation (the highest address in the stack region). Whenespis set to this value, pushes will grow into the reserved space.
8-16: Entry sequence is clean and functional.The logic is correct: set up stack, call kernel, halt if it returns. The assumption that the bootloader has properly configured segments is documented and reasonable for this project.
For extra robustness, some kernels explicitly reload segment selectors (e.g.,
mov ax, 0x10; mov ds, ax; mov es, ax; ...) at entry to ensure a known-good state, but this is optional for a trusted bootloader.Makefile (2)
63-66: Object file order in linking is correct.The linker command places
kernel_entry.ofirst inOBJ_FILESviaASM_SOURCES = kernel_entry.asm interrupt.asm, which ensures the entry point code appears at the expected location. The dependency onlinker.ldensures relinking when the linker script changes.
80-85: Clean target and phony declarations are appropriate.The clean target removes all generated artifacts. The static analysis warning about a missing
testtarget is not applicable to bare-metal kernel development, which typically requires emulator-based testing rather than standard unit tests.
| # Link (Entry point MUST be first) | ||
| i686-w64-mingw32-gcc -Wl,--image-base,0 -Ttext 0x100000 -o kernel.elf -ffreestanding -O2 -nostdlib kernel_entry.o kernel.o interrupt.o -lgcc |
There was a problem hiding this comment.
Build script doesn't use linker.ld; diverges from Makefile.
This script uses -Ttext 0x100000 directly instead of -T linker.ld like the Makefile does. This creates two different build paths that may produce different binaries. Additionally, the -Wl,--image-base,0 flag is unusual for ELF outputs and may not have the intended effect.
🔎 Suggested fix to align with Makefile
# Link (Entry point MUST be first)
-i686-w64-mingw32-gcc -Wl,--image-base,0 -Ttext 0x100000 -o kernel.elf -ffreestanding -O2 -nostdlib kernel_entry.o kernel.o interrupt.o -lgcc
+i686-w64-mingw32-gcc -T linker.ld -ffreestanding -O2 -nostdlib -o kernel.elf kernel_entry.o kernel.o interrupt.o -lgcc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Link (Entry point MUST be first) | |
| i686-w64-mingw32-gcc -Wl,--image-base,0 -Ttext 0x100000 -o kernel.elf -ffreestanding -O2 -nostdlib kernel_entry.o kernel.o interrupt.o -lgcc | |
| # Link (Entry point MUST be first) | |
| i686-w64-mingw32-gcc -T linker.ld -ffreestanding -O2 -nostdlib -o kernel.elf kernel_entry.o kernel.o interrupt.o -lgcc |
🤖 Prompt for AI Agents
In create_iso.sh around lines 12-13, the linker invocation diverges from the
Makefile by using "-Ttext 0x100000" and "-Wl,--image-base,0" instead of the
project's linker script; change the invocation to use "-T linker.ld" (pointing
to the same linker.ld used by the Makefile) and remove the "-Wl,--image-base,0"
unless the image base is intentionally required—if an image base is needed, move
that setting into linker.ld so both build paths use the same linker script and
flags to produce identical binaries.
| #include <stdint.h> | ||
| #include <stddef.h> | ||
| #include <stdbool.h> | ||
| #include <string.h> |
There was a problem hiding this comment.
Verify if the project provides memory function implementations required by GCC in freestanding mode.
A conforming freestanding implementation is required to provide certain library facilities: those in <float.h>, <limits.h>, <stdarg.h>, and <stddef.h>; since AMD1, also those in <iso646.h>; since C99, also those in <stdbool.h> and <stdint.h>; and since C11, also those in <stdalign.h> and <stdnoreturn.h>. Notably, <string.h> is not included.
However, GCC requires the freestanding environment provide memcpy, memmove, memset and memcmp. Even when compiling in freestanding mode, gcc can emit a call to the standard library function memcpy or memset, such as when initializing an array or struct or passing a struct in or out of a function. As a freestanding program does not have access to the standard library, it must supply its own implementations of these functions. Verify that these implementations are provided in the project, either through custom definitions or inclusion of a freestanding-compatible C library.
🤖 Prompt for AI Agents
In kernel.h around lines 4 to 7, the header includes <string.h> even though in a
freestanding environment the standard string functions may not be available; GCC
can still emit calls to memcpy/memmove/memset/memcmp so the project must provide
them. Verify whether the project already supplies freestanding-compatible
implementations (or a freestanding libc) of memcpy/memmove/memset/memcmp; if
not, add portable implementations in the kernel source (or link a freestanding C
library), declare their prototypes in an appropriate header (or keep them extern
in kernel.h) and remove reliance on <string.h> (or guard the include with a
freestanding-detection macro) so compilation does not assume a hosted lib.
Ensure implementations have correct signatures, are optimized/annotated as
needed, and are used by the build/linker.
| # --- Object Files --- | ||
| OBJ_FILES = $(C_SOURCES:.c=.o) $(ASM_SOURCES:.asm=.o) |
There was a problem hiding this comment.
Object file order may place kernel.o before entry point.
OBJ_FILES expands to kernel.o kernel_entry.o interrupt.o, which means kernel.o's .text section may appear before kernel_entry.o's _start in the final binary. When the bootloader jumps to the kernel, it expects the entry point at the beginning.
Either reorder the sources or use a linker script section to explicitly place the entry code first:
🔎 Option 1: Reorder sources
# --- Object Files ---
-OBJ_FILES = $(C_SOURCES:.c=.o) $(ASM_SOURCES:.asm=.o)
+OBJ_FILES = $(ASM_SOURCES:.asm=.o) $(C_SOURCES:.c=.o)🔎 Option 2: Use explicit entry section in linker.ld
.text :
{
*(.text.entry) /* Entry point first */
*(.text)
}Then in kernel_entry.asm:
section .text.entry🤖 Prompt for AI Agents
In Makefile around lines 31-32, OBJ_FILES currently expands in an order that can
place kernel.o before the entry-point object, risking the kernel entry code not
being first in the final image; fix by ensuring the entry object is listed first
or by using a linker script to force the entry section first: either reorder the
source lists so kernel_entry.o appears before kernel.o in OBJ_FILES, or update
the linker.ld to place a dedicated .text.entry (or similar) before other .text
sections and change kernel_entry.asm to emit into that section so the entry code
is guaranteed at the start of the .text segment.
1 participant
The kernel.c and kernel.h contains the complete .c and .h code respectively.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.