Development #80
Development #80
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRewrites ELF loading into a guarded, validated loader with new API/options; adds VfsGetFileSize and FAT12 serial diagnostics; inserts a runtime CheckResourceLeaks() called from the main process loop; removes an image-conversion script; and replaces README feature checklist with project-structure docs plus BUILD and ROADMAP files. Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as Kernel Shell
participant ELF as ELF Loader
participant VMem as Guarded VM Alloc
participant Proc as Process Manager
Shell->>ELF: CreateProcessFromElf(path, &opts)
ELF->>ELF: ValidateElfFile(header, size)
ELF->>VMem: VMemAllocWithGuards(total_process_size)
ELF->>ELF: Copy PT_LOAD segments -> allocated region
ELF->>VMem: VMemFreeWithGuards on error
ELF->>Proc: Register process, return PID
Proc-->>Shell: PID (uint32_t)
sequenceDiagram
participant AstraLoop as Astra main loop
participant StackGuard as CheckResourceLeaks()
participant Memory as GetDetailedMemoryStats()
participant Console as PrintKernelWarning()
AstraLoop->>StackGuard: call CheckResourceLeaks()
StackGuard->>Memory: sample allocation/free counters
alt suspicious net growth detected (every 100 calls)
StackGuard->>Console: PrintKernelWarning("Potential memory leak detected: net +N")
end
AstraLoop->>AstraLoop: CleanupTerminatedProcesses()
AstraLoop->>AstraLoop: Yield()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
docs/BUILD.md (2)
15-24: Fix heading level to comply with markdownlint (MD001).Use H2 for “Quickstart” to increment one level from H1.
-### Quickstart +## Quickstart
1-24: Optional: Add a BIOS/UEFI hybrid ISO note (align with project capabilities).Per prior work, the project supports hybrid BIOS/UEFI boot. Consider adding a short “BIOS/UEFI” section with the exact command/target (if any) to produce a dual-bootable ISO and any extra dependencies.
I can draft a “Hybrid ISO” section tailored to your current Meson targets if you want.
docs/ROADMAP.md (2)
124-129: Roadmap item likely already achieved: “Bootable ISO with GRUB2 EFI support”.Previous work indicates completed UEFI boot with hybrid BIOS/UEFI ISO creation. Either check this off or add nuance (e.g., “done for kernel demo; pending for v1.0 release criteria”).
I can submit a follow-up patch toggling this item and adding a note on the current status if you confirm.
1-129: Optional: Link checklist items to source or issues.For maintainability, consider linking some items to the relevant directories (e.g., ELF loader, scheduler) or GitHub issues/PRs for traceability.
I can generate a pass that adds links where file paths are clear from the README structure.
README.md (8)
12-13: Fix heading level (MD001).Use H2 for “Project Structure” to increment correctly from H1.
-### Project Structure +## Project Structure
13-13: Specify a language for the fenced code block (MD040) and improve rendering.These are file paths; “text” is an appropriate language hint.
-``` +```text
15-15: Typo: “Architechture” → “Architecture”.Also hyphenate for adjective use: “Architecture-specific”.
-├── arch/x86_64/ # Architechture specific code +├── arch/x86_64/ # Architecture-specific code
54-55: Duplicate entry: Serial.c appears twice.Remove the duplicate or replace one with the corresponding header if that was the intent.
-│ ├── Serial.c -│ ├── Serial.c +│ ├── Serial.c
80-81: Typo: “Spilock.h” → “Spinlock.h”.-│ │ └── Spilock.h +│ │ └── Spinlock.h
85-86: Duplicate entry: Panic.c appears twice.Remove the duplicate or add the header if that’s what was meant.
-│ │ ├── Panic.c -│ │ ├── Panic.c +│ │ ├── Panic.c
88-90: Inconsistent ELF loader naming/casing.Standardize to ELFLoader.* for consistency.
-│ │ ├── ELFloader.c -│ │ └── ElFloader.h +│ │ ├── ELFLoader.c +│ │ └── ELFLoader.h
13-126: Avoid documentation drift: auto-generate the tree.Manually maintained trees easily get stale (as seen with typos/duplicates above). Consider generating this section via a script (e.g., tree + filters) in CI and embedding it, or regenerating on release.
I can provide a script/target that builds a sanitized directory tree for inclusion in README.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)docs/BUILD.md(1 hunks)docs/ROADMAP.md(1 hunks)scripts/converter.sh(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/converter.sh
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T03:19:52.798Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.798Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
docs/BUILD.mdREADME.mddocs/ROADMAP.md
📚 Learning: 2025-08-12T03:19:52.798Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.798Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
docs/BUILD.mdREADME.mddocs/ROADMAP.md
📚 Learning: 2025-08-12T01:43:39.812Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T01:43:39.812Z
Learning: User prefers to defer syscall implementation complexity until next year and is considering making a ring 0 kernel instead of continuing with user-mode syscall approach in the VoidFrame kernel project.
Applied to files:
README.md
🪛 markdownlint-cli2 (0.17.2)
docs/BUILD.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
docs/BUILD.md (1)
21-23: Validate Meson/Ninja targets referenced.“ninja img” and “ninja mkfs” imply custom targets. Ensure these exist or document them briefly (what they do and expected outputs). If they’re distro/toolchain-sensitive, note prerequisites (e.g., xorriso/mtools for ISO creation).
Would you like me to scan the build system to enumerate and document available Meson/Ninja targets?
docs/ROADMAP.md (1)
71-76: Align “IPC / Syscalls” with the “syscall-less microkernel” positioning.README now brands the kernel as “syscall-less,” while this section lists “Syscall dispatch system” as complete and further syscall-related items as pending. Clarify whether “syscall-less” is a near-term direction (e.g., message-passing only in ring 0) or historical; update the checklist labels accordingly.
Would you like me to propose alternative wording that emphasizes message-passing primitives and de-emphasizes syscalls?
README.md (2)
27-29: Likely incorrect filenames under idt/.These entries reference GDT files in the IDT directory. If this is a typo, fix to Idt.*.
-│ │ ├── Gdt.h -│ │ └── Gdt.c +│ │ ├── Idt.h +│ │ └── Idt.cIf GDT files truly reside there, consider moving or explaining the layout to avoid confusion.
1-4: Clarify “syscall-less microkernel” claim vs. roadmap.The README title says “syscall-less,” but ROADMAP lists syscall dispatch as complete and more syscall items pending. Either:
- clarify “syscall-less” as a current design choice (message passing only in ring 0), or
- adjust roadmap to match the positioning.
I can harmonize the phrasing across README and ROADMAP once you confirm the intended direction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (3)
12-12: Fix heading level to satisfy markdownlint (MD001).Change the "Project Structure" heading to h2 (##) since the previous heading is h1.
-### Project Structure +## Project Structure
13-13: Specify a language for the fenced code block (MD040).Add a language hint to the tree block to silence markdownlint and improve rendering.
-``` +```text
15-15: Fix typos and duplicates in the tree listing for accuracy.These small corrections improve onboarding and reduce confusion when navigating the repo.
-├── arch/x86_64/ # Architechture specific code +├── arch/x86_64/ # Architecture-specific code-│ │ ├── Gdt.h -│ │ └── Gdt.c +│ │ ├── Idt.h +│ │ └── Idt.c-│ ├── Serial.c -│ ├── Serial.c +│ ├── Serial.c +│ ├── Serial.h-│ │ └── Spilock.h +│ │ └── Spinlock.h-│ │ ├── Panic.c -│ │ ├── Panic.c +│ │ ├── Panic.c +│ │ ├── Panic.h-│ │ └── ElFloader.h +│ │ └── ELFloader.hAlso applies to: 27-28, 54-55, 80-80, 84-86, 89-90
kernel/memory/StackGuard.h (1)
5-6: Optional: avoid wide header coupling.Including Memory.h and Console.h in a widely-included header can increase compile-time and risk circular deps. Consider moving CheckResourceLeaks() to StackGuard.c (or Diagnostics.c) and expose a prototype here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)docs/BUILD.md(1 hunks)kernel/memory/StackGuard.h(2 hunks)kernel/process/Process.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/BUILD.md
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-13T08:43:54.720Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#80
File: README.md:7-8
Timestamp: 2025-08-13T08:43:54.720Z
Learning: User assembler-0 is working on VoidFrame kernel with a tight deadline (2-3 days before summer ends) and specifically requested runtime safety checks similar to __stack_chk_fail but for deadlocks and other kernel issues. The kernel already has extensive safety mechanisms including stack canaries, guard pages, heap integrity checks, and process security validation.
Applied to files:
README.md
📚 Learning: 2025-08-12T01:43:39.812Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T01:43:39.812Z
Learning: User prefers to defer syscall implementation complexity until next year and is considering making a ring 0 kernel instead of continuing with user-mode syscall approach in the VoidFrame kernel project.
Applied to files:
README.md
📚 Learning: 2025-08-12T02:13:46.239Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.239Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
README.md
📚 Learning: 2025-08-10T06:22:28.788Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.788Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
README.md
📚 Learning: 2025-08-12T03:19:52.798Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.798Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
README.md
📚 Learning: 2025-08-12T03:19:52.798Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.798Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
README.md
🧬 Code Graph Analysis (2)
kernel/process/Process.c (1)
kernel/memory/StackGuard.h (1)
CheckResourceLeaks(15-28)
kernel/memory/StackGuard.h (2)
kernel/memory/Memory.c (1)
GetDetailedMemoryStats(319-369)kernel/etc/Console.c (1)
PrintKernelWarning(211-215)
🪛 markdownlint-cli2 (0.17.2)
README.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
README.md (1)
7-8: Good fix: docs links now point to the right place.Both Roadmap and Build links correctly reference docs/ROADMAP.md and docs/BUILD.md.
kernel/process/Process.c (1)
13-13: Include addition looks good.Importing StackGuard.h here is appropriate for leak checks without leaking memory headers into unrelated modules.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores