Development #7
Development #7
Conversation
WalkthroughThis update introduces a kernel panic handling system, including new Changes
Sequence Diagram(s)sequenceDiagram
participant KernelFunction
participant PanicHandler
participant CPU
KernelFunction->>PanicHandler: Panic("Error message")
PanicHandler->>CPU: Disable interrupts (cli)
PanicHandler->>PanicHandler: Clear screen, reset cursor
PanicHandler->>PanicHandler: Print panic header and message
loop Halt forever
PanicHandler->>CPU: hlt
end
Possibly related PRs
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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 (1)
Kernel/Kernel.c (1)
118-119: Good: Added bounds checking with minor improvement suggestion.The bounds checking prevents buffer overflows and limits printing to 80 characters. Consider also updating the column position tracking to match the character limit.
- for (int k = 0; str[k] != '\0' && k < 80; k++) { + for (int k = 0; str[k] != '\0' && k < 80 && (col + k) < 80; k++) {This ensures that printing doesn't exceed the line width when starting from a non-zero column position.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Kernel/Gdt.c(1 hunks)Kernel/Kernel.c(3 hunks)Kernel/Memory.c(2 hunks)Kernel/Panic.c(1 hunks)Kernel/Panic.h(1 hunks)Kernel/Process.c(5 hunks)Kernel/Syscall.c(2 hunks)Kernel/UserMode.c(1 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Kernel/Syscall.c (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
🧬 Code Graph Analysis (4)
Kernel/Memory.c (1)
Kernel/Panic.h (1)
Panic(7-7)
Kernel/Panic.c (2)
Kernel/Panic.h (2)
Panic(7-7)PanicWithCode(8-8)Kernel/Kernel.c (3)
ClearScreen(16-22)PrintKernel(24-50)PrintKernelHex(52-74)
Kernel/Panic.h (1)
Kernel/Panic.c (2)
noreturn(5-20)noreturn(22-39)
Kernel/Process.c (2)
Kernel/Panic.h (1)
Panic(7-7)Kernel/Memory.c (1)
AllocPage(29-41)
🔇 Additional comments (20)
Kernel/Gdt.c (1)
2-2: LGTM: Preparatory inclusion for panic infrastructure.The inclusion of
Panic.haligns with the kernel-wide panic handling system rollout. While no panic calls are made in this file currently, this prepares the GDT module for future defensive programming enhancements.meson.build (1)
54-55: LGTM: Build configuration properly updated.The addition of
Kernel/Panic.cto the build sources correctly integrates the panic handling implementation into the kernel compilation process.Kernel/UserMode.c (2)
5-5: LGTM: Panic header inclusion supports error handling.The inclusion of
Panic.henables the use of panic calls for critical error conditions in user mode transitions.
10-12: Excellent: Replaced silent failure with explicit panic.The change from silent return to explicit panic on stack allocation failure significantly improves error visibility and debugging capabilities. The descriptive panic message clearly indicates the failure condition.
Kernel/Memory.c (4)
5-5: LGTM: Panic header inclusion enables error handling.The inclusion of
Panic.hsupports the defensive programming improvements in memory management functions.
44-46: Excellent: Added NULL pointer validation.The NULL pointer check prevents potential crashes from dereferencing invalid pointers. The descriptive panic message aids in debugging memory management issues.
49-51: Excellent: Added address range validation.The validation ensures that only addresses within the valid memory range are processed. This prevents corruption of kernel memory areas below
memory_start.
54-56: Excellent: Added bounds checking.The bounds check prevents array access violations and ensures that only valid page indices are processed. This prevents potential memory corruption.
Kernel/Kernel.c (5)
13-13: LGTM: Panic header inclusion prepares for error handling.The inclusion of
Panic.haligns with the kernel-wide panic infrastructure, preparing the kernel module for potential future panic usage in critical error conditions.
25-25: LGTM: Added NULL pointer protection.The NULL pointer check prevents crashes when invalid strings are passed to the print function.
31-31: LGTM: Added screen bounds protection.The screen bounds check prevents writing beyond the 25-line screen limit, avoiding potential memory corruption.
38-41: LGTM: Added video memory bounds checking.The bounds check ensures that characters are only written within the valid video memory range, preventing memory corruption.
111-114: LGTM: Added comprehensive input validation.The validation checks for NULL strings and ensures line/column parameters are within valid screen boundaries (0-24 lines, 0-79 columns).
Kernel/Syscall.c (3)
5-11: Good defensive programming with panic check!The addition of the null check for the current process and the panic call is appropriate here. A syscall without a valid current process indicates a critical kernel state inconsistency that warrants a panic.
24-29: Proper input validation for user-provided data.Good addition of buffer validation checks. Returning -1 for invalid user input is the correct approach rather than panicking, as these are expected error conditions from untrusted user space.
38-38: Correct screen boundary enforcement.The check properly prevents the cursor from moving beyond the last line (24) of the 25-line display, keeping output visible on the last line when it would otherwise scroll off-screen.
Kernel/Panic.c (1)
5-20: Well-implemented panic handler.The panic function correctly disables interrupts, provides clear visual feedback, and enters a safe halt state. The implementation matches the
noreturnattribute declaration.Kernel/Process.c (3)
44-68: Excellent defensive checks in process creation.All four panic conditions are appropriate:
- NULL entry point indicates programming error
- Process limit and slot exhaustion are critical resource issues
- Stack allocation failure is an unrecoverable memory error
These replace silent failures with immediate, visible kernel panics for easier debugging.
141-147: Critical validation in interrupt context.Good additions - both conditions would lead to undefined behavior:
- NULL registers would cause immediate segfault when dereferencing
- Invalid current_process index would access out-of-bounds memory
Panicking here provides clear error indication instead of mysterious crashes.
223-225: Important bounds check for process array access.This check prevents potential out-of-bounds access to the processes array, which could lead to memory corruption or crashes. Good defensive programming practice.
| #define ASSERT(condition) \ | ||
| do { \ | ||
| if (!(condition)) { \ | ||
| Panic("Assertion failed: " #condition " at " __FILE__ ":" __LINE__); \ | ||
| } \ | ||
| } while(0) |
There was a problem hiding this comment.
Fix ASSERT macro stringification of LINE.
The __LINE__ macro expands to an integer and cannot be directly concatenated into a string literal. This will cause a compilation error.
Apply this fix to properly stringify the line number:
-#define ASSERT(condition) \
- do { \
- if (!(condition)) { \
- Panic("Assertion failed: " #condition " at " __FILE__ ":" __LINE__); \
- } \
- } while(0)
+#define STRINGIFY(x) #x
+#define TOSTRING(x) STRINGIFY(x)
+#define ASSERT(condition) \
+ do { \
+ if (!(condition)) { \
+ Panic("Assertion failed: " #condition " at " __FILE__ ":" TOSTRING(__LINE__)); \
+ } \
+ } while(0)📝 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.
| #define ASSERT(condition) \ | |
| do { \ | |
| if (!(condition)) { \ | |
| Panic("Assertion failed: " #condition " at " __FILE__ ":" __LINE__); \ | |
| } \ | |
| } while(0) | |
| #define STRINGIFY(x) #x | |
| #define TOSTRING(x) STRINGIFY(x) | |
| #define ASSERT(condition) \ | |
| do { \ | |
| if (!(condition)) { \ | |
| Panic("Assertion failed: " #condition " at " __FILE__ ":" TOSTRING(__LINE__)); \ | |
| } \ | |
| } while (0) |
🤖 Prompt for AI Agents
In Kernel/Panic.h around lines 11 to 16, the ASSERT macro incorrectly
concatenates the __LINE__ macro, which is an integer, directly into a string
literal causing a compilation error. To fix this, define helper macros to
stringify the __LINE__ macro properly and use them to convert the line number to
a string before concatenation in the Panic call within the ASSERT macro.
| void __attribute__((noreturn)) PanicWithCode(const char* message, uint64_t error_code) { | ||
| asm volatile("cli"); | ||
|
|
||
| ClearScreen(); | ||
| CurrentLine = 0; | ||
| CurrentColumn = 0; | ||
|
|
||
| PrintKernel("[----KERNEL PANIC----]\n"); | ||
| PrintKernel(message); | ||
| PrintKernel("\nError Code: "); | ||
| PrintKernelHex(error_code); | ||
| PrintKernel(" -- Not handled"); | ||
| PrintKernel("\n\nSystem halted.\n"); | ||
|
|
||
| while(1) { | ||
| asm volatile("hlt"); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Potential truncation of error code in PrintKernelHex call.
The function passes a uint64_t error_code to PrintKernelHex which accepts an int parameter. This will truncate high 32 bits of the error code on 64-bit systems.
Consider either:
- Updating
PrintKernelHexto acceptuint64_t - Adding a separate
PrintKernelHex64function - Documenting that only 32-bit error codes are supported
🤖 Prompt for AI Agents
In Kernel/Panic.c around lines 22 to 39, the function PanicWithCode passes a
64-bit error_code to PrintKernelHex, which only accepts an int, causing
truncation of the high 32 bits on 64-bit systems. To fix this, update
PrintKernelHex to accept a uint64_t parameter to handle full 64-bit values, or
alternatively create a new PrintKernelHex64 function that supports 64-bit
printing and use it here instead. Ensure the printing function correctly formats
the full 64-bit error code without truncation.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores