GPL #107
GPL #107
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Linux-style type and fd_set definitions and ctype header/implementation, updates stdarg guards/macros, introduces an asmlinkage calling-convention and annotates kernel entry/interrupt handlers, rewrites the formatter to a vsprintf-based implementation with simple_strtoul, and extends RTC time with optional century handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Formatter as Format / FormatA / FormatS
participant VS as vsprintf
participant NUM as number()
note right of VS #F3F7E8: New vsprintf-centric formatting flow
Caller->>Formatter: call Format*(buf, fmt, ...)
Formatter->>VS: vsprintf(buf, fmt, va_list)
loop parse format string
VS->>NUM: format integer/ptr (base, flags, width, precision)
NUM-->>VS: digits + sign/prefix
VS-->>VS: copy chars/strings, apply padding
end
VS-->>Formatter: return written length
Formatter-->>Caller: result
sequenceDiagram
autonumber
actor User
participant ST as simple_strtoul
User->>ST: simple_strtoul(cp, &endp, base)
alt base == 0
ST-->>ST: detect "0x"/leading '0' to choose radix
end
ST-->>User: parsed unsigned long + end pointer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (6)
✨ 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: 13
🧹 Nitpick comments (6)
meson.build (1)
127-127: Don't compile sources from the include/ directory.Compiling include/ctype.c from under include/ breaks layering and makes headers non-header-only. Prefer moving the implementation to a src/lib (or kernel/etc) location and keep include/ for headers only.
If/when you relocate the file, update this entry accordingly:
- src_root + '/include/ctype.c', + src_root + '/kernel/lib/ctype.c',include/ctype.c (2)
7-7: Source file under include/Placing a .c in include/ is non-standard and can confuse tooling. Move to lib/ or kernel/lib/ and adjust the build.
Example:
- // meson.build: add('include/ctype.c') + // meson.build: add('lib/ctype.c')
10-34: Table OK; note extended range behaviorASCII 0–127 are classified; 128–255 are zeroed. That’s fine if you intend pure-ASCII, but document it to avoid surprises with extended encodings.
kernel/etc/Format.c (2)
7-10: Remove profane commentKeep attribution but drop profanity to meet project tone and potential CI linters.
-/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */ -/* - * Wirzenius wrote this portably, Torvalds fucked it up :-) - */ +/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. Historical note removed for tone. */
268-275: FormatA still unboundedSame concern as Format(). Route through a bounded implementation once added.
- int result = vsprintf(buffer, format, args); + int result = vsprintf(buffer, format, args); // TODO: switch to vsnprintfI can wire both to a new vsnprintf with minimal churn once we settle on the helper.
include/ctype.h (1)
28-30: isascii/toascii rarely neededThese macros are fine, but consider limiting their exposure or prefixing to avoid namespace clashes with libc if userland code is ever built against this header.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
include/Ltypes.h(1 hunks)include/ctype.c(1 hunks)include/ctype.h(1 hunks)include/stdarg.h(1 hunks)kernel/etc/Format.c(1 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#101
File: kernel/etc/Shell.c:829-837
Timestamp: 2025-08-31T07:06:36.977Z
Learning: assembler-0 is working on optimizing ramfs performance in VoidFrame kernel and wants to implement proper filesystem characteristics like faster lookups, better memory management, and advanced features like copy-on-write and caching.
📚 Learning: 2025-09-01T02:34:45.398Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: kernel/etc/Format.c:384-391
Timestamp: 2025-09-01T02:34:45.398Z
Learning: The user prefers FormatS over FormatA for performance reasons in kernel/etc/Format.c, accepting the thread-safety trade-off when the result is consumed immediately.
Applied to files:
kernel/etc/Format.c
🧬 Code graph analysis (1)
kernel/etc/Format.c (1)
kernel/etc/StringOps.c (1)
StringLength(3-8)
🔇 Additional comments (5)
include/stdarg.h (1)
1-10: LGTM: freestanding stdarg mapped to builtins (+va_copy).Guard rename and mapping to _builtin* are correct for -nostdinc/-ffreestanding.
include/Ltypes.h (1)
1-2: Add correct license header/attribution.This header mirrors early Linux types/fd_set code. If derived, add proper SPDX and attribution to satisfy GPL requirements, or confirm it’s clean-room. Missing license header is risky.
I can draft SPDX/copyright headers consistent with the project’s license.
kernel/etc/Format.c (2)
195-213: String precision/width path OKString path respects width/precision and NULL handling via "". Looks good.
244-247: No unsafe %n usage detected; no changes required
Format.c implements the %n specifier, but no “%n” appears in any format strings and all calls to Format use trusted, literal formats.include/ctype.h (1)
1-3: Add SPDX and ensure GPL alignmentMirror the SPDX tag here as well.
-#ifndef _LINUX_CTYPE_H +#ifndef _LINUX_CTYPE_H +#define SPDX_LICENSE_IDENTIFIER /* SPDX-License-Identifier: GPL-2.0-only */ #define _LINUX_CTYPE_HAlternatively, place SPDX as a comment on the first line and keep guard as-is.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
include/ctype.h (1)
13-14: Remove global_ctmp; make tolower/toupper static inline (thread-safe, reentrant).Current macros rely on a global temp and are not safe under interrupts/SMP.
-extern char _ctmp; +/* no _ctmp needed */ -#define tolower(c) (_ctmp=c,isupper(_ctmp)?_ctmp-('A'-'a'):_ctmp) -#define toupper(c) (_ctmp=c,islower(_ctmp)?_ctmp-('a'-'A'):_ctmp) +static inline int tolower(int c) { + unsigned char ch = (unsigned char)c; + return isupper(ch) ? (int)(ch - ('A' - 'a')) : (int)ch; +} +static inline int toupper(int c) { + unsigned char ch = (unsigned char)c; + return islower(ch) ? (int)(ch + ('A' - 'a')) : (int)ch; +}Note: also delete the
_ctmpdefinition in ctype.c.Also applies to: 31-32
🧹 Nitpick comments (3)
include/ctype.h (2)
1-2: Avoid reserved identifier; use project-specific include guard.Leading underscore + capital is reserved; prefer a neutral guard.
-#ifndef _LINUX_CTYPE_H -#define _LINUX_CTYPE_H +#ifndef VOIDFRAME_CTYPE_H +#define VOIDFRAME_CTYPE_H @@ -#endif +#endif /* VOIDFRAME_CTYPE_H */Also applies to: 34-34
13-13: Make_ctypeconst to live in .rodata and prevent accidental writes.-extern unsigned char _ctype[]; +extern const unsigned char _ctype[];(Remember to update the definition in ctype.c accordingly.)
arch/x86_64/interrupts/Interrupts.c (1)
2-2: Avoid pulling Kernel.h just for asmlinkage; include a tiny linkage header instead.Reduces compile-time deps and layering. After moving the macro, include that small header here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
arch/x86_64/interrupts/Interrupts.c(2 hunks)include/ctype.c(1 hunks)include/ctype.h(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/core/Kernel.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/ctype.c
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#101
File: kernel/etc/Shell.c:829-837
Timestamp: 2025-08-31T07:06:36.977Z
Learning: assembler-0 is working on optimizing ramfs performance in VoidFrame kernel and wants to implement proper filesystem characteristics like faster lookups, better memory management, and advanced features like copy-on-write and caching.
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
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:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-31T14:19:11.338Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.338Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-09-01T01:10:22.088Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.088Z
Learning: VoidFrame's interrupt handlers in arch/x86_64/interrupts/Interrupts.asm only save basic GPRs and segment registers, missing critical registers for deep fault tracing: control registers (CR0/CR2/CR3/CR4), SIMD state (XMM/YMM/ZMM), debug registers (DR0-DR7), and extended FPU state. The CPU initialization code enables SSE/AVX/XSAVE features but interrupt handlers don't preserve this extended state. Enhanced fault tracing requires selective extended state saving for critical exceptions (ISR 6, 8, 13, 14) while keeping lightweight handling for regular interrupts.
Applied to files:
arch/x86_64/interrupts/Interrupts.c
🧬 Code graph analysis (3)
kernel/core/Kernel.h (1)
kernel/core/Kernel.c (1)
ParseMultibootInfo(43-69)
arch/x86_64/interrupts/Interrupts.c (1)
kernel/core/Kernel.c (1)
void(731-750)
kernel/core/Kernel.c (1)
arch/x86_64/interrupts/Interrupts.c (1)
void(15-94)
🔇 Additional comments (3)
include/ctype.h (2)
16-27: Good: casts to unsigned char avoid UB on negative char inputs.
16-27: Ensure ctype macros never receive EOF (-1)
Audit all call sites—especially those using fgetc/getc return values—to confirm they only pass valid unsigned‐char values (e.g. via an explicit cast or EOF check), since these macros no longer special-case EOF.arch/x86_64/interrupts/Interrupts.c (1)
15-15: ISR stub correctly passes Registers in RDI; no changes required*
Verified in Interrupts.asm (lines 42, 111, 180) that each call to InterruptHandler is preceded bymov rdi, rsp, satisfying the asmlinkage convention.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
drivers/RTC/Rtc.c (2)
19-24: NMI remains disabled after each CMOS read. Re-enable it.cmos_read sets bit 7 (NMI disable) and never clears it, potentially leaving NMI off beyond the read window.
Apply this fix to re-enable NMI before returning from cmos_read:
static uint8_t cmos_read(uint8_t reg) { - - // Select the register, making sure NMI is disabled - outb(CMOS_ADDRESS, (1 << 7) | reg); - // Read the data - return inb(CMOS_DATA); + // Select the register with NMI disabled + outb(CMOS_ADDRESS, (1 << 7) | reg); + // Read the data + uint8_t val = inb(CMOS_DATA); + // Re-enable NMI by clearing bit 7 (index value is not critical here) + outb(CMOS_ADDRESS, reg); + return val; }
83-91: Incorrect hour conversion in BCD mode and missing 12/24h handling in binary mode.Hour is not BCD-decoded before arithmetic, and 12h→24h logic should apply regardless of BCD. 20–23 o’clock become wrong (e.g., 0x20→8).
Replace the conversion block with a unified, correct flow:
- if (!(status_b & 0x04)) { // Bit 2 clear means BCD mode - rtc_time->second = bcd_to_bin(rtc_time->second); - rtc_time->minute = bcd_to_bin(rtc_time->minute); - // Handle 12/24 hour clock for the hour value - rtc_time->hour = ((rtc_time->hour & 0x7F) + 12 * ((rtc_time->hour & 0x80) != 0)) % 24; - rtc_time->day = bcd_to_bin(rtc_time->day); - rtc_time->month = bcd_to_bin(rtc_time->month); - rtc_time->year = bcd_to_bin(rtc_time->year); - } + /* Decode BCD fields if needed (Status B bit 2 == 0 => BCD) */ + int bcd_mode = !(status_b & 0x04); + int is_24h = (status_b & 0x02) != 0; + uint8_t raw_h = rtc_time->hour; + + if (bcd_mode) { + rtc_time->second = bcd_to_bin(rtc_time->second); + rtc_time->minute = bcd_to_bin(rtc_time->minute); + rtc_time->day = bcd_to_bin(rtc_time->day); + rtc_time->month = bcd_to_bin(rtc_time->month); + rtc_time->year = bcd_to_bin(rtc_time->year); + } + + /* Hour: decode BCD if needed, then normalize 12/24h */ + uint8_t hour = bcd_mode ? bcd_to_bin(raw_h & 0x7F) : (raw_h & 0x7F); + if (!is_24h) { + int pm = (raw_h & 0x80) != 0; /* PM flag only valid in 12h mode */ + if (pm) { + if (hour != 12) hour = (hour + 12) % 24; /* 1–11 PM -> 13–23; 12 PM stays 12 */ + } else { + if (hour == 12) hour = 0; /* 12 AM -> 00 */ + } + } + rtc_time->hour = hour;
♻️ Duplicate comments (1)
include/ctype.h (1)
1-3: Add SPDX license header for GPL compliance.The file lacks an SPDX tag. Add the correct identifier at the top to reflect the repository license choice.
+/* SPDX-License-Identifier: GPL-2.0-only */ #ifndef _LINUX_CTYPE_H #define _LINUX_CTYPE_H
🧹 Nitpick comments (5)
include/ctype.h (2)
13-13: Consider making the classification table const.Mark the lookup table as const to enforce RO data and allow better placement.
-extern unsigned char _ctype[]; +extern const unsigned char _ctype[];Remember to update the definition in the implementation file accordingly.
1-2: Rename include guard to project-specific (optional).If this isn’t a verbatim import, prefer a project-specific guard to avoid symbol collisions.
-#ifndef _LINUX_CTYPE_H -#define _LINUX_CTYPE_H +#ifndef VOIDFRAME_CTYPE_H +#define VOIDFRAME_CTYPE_Hdrivers/RTC/Rtc.h (1)
6-15: Document century semantics.Clarify whether century is raw CMOS (BCD/binary) or normalized, and when it’s valid (only if CMOS register 0x32 exists).
drivers/RTC/Rtc.c (2)
42-56: Optional: reduce busy-wait impact and guard CMOS access.
- Add a small cpu_relax()/pause or io_delay in the update-in-progress spin to reduce contention.
- Protect port 0x70/0x71 access with a lock to avoid interleaving with other RTC users.
50-51: Optional: gate first-pass century read by config.If VF_CONFIG_RTC_CENTURY is off, avoid touching 0x32 on hardware that doesn’t implement it.
- rtc_time->century = cmos_read(CMOS_REG_CENTURY); +#ifdef VF_CONFIG_RTC_CENTURY + rtc_time->century = cmos_read(CMOS_REG_CENTURY); +#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
drivers/RTC/Rtc.c(3 hunks)drivers/RTC/Rtc.h(1 hunks)include/ctype.h(1 hunks)kernel/core/Kernel.h(1 hunks)kernel/etc/Format.c(1 hunks)meson.build(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- meson.build
- kernel/etc/Format.c
- kernel/core/Kernel.h
🔇 Additional comments (3)
include/ctype.h (2)
15-25: LGTM: safe table indexing.Casting macro args to unsigned char fixes UB on negative char values. Good.
30-37: LGTM: inline tolower/toupper.Thread-safe, single-evaluation, and ASCII-correct.
drivers/RTC/Rtc.h (1)
14-15: Struct ABI change: new field added. Validate callers and serialization.Adding century breaks ABI/packing. Verify all callers, IPC/ABI boundaries, and any persistence formats.
Would you like a script to scan for rtc_time_t sizeof/offset assumptions?
1 participant
definitely didnt borrow linux 1.0 code
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation