Development #70
Merged
Development #70
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. WalkthroughAdds a public PitSetFrequency(uint16_t) API that atomically reprograms the PIT, decouples PIT initialization from PIC init (Kernel now calls PitInstall directly), changes Process to use PitSetFrequency and to hold an IRQ-safe spinlock during Yield, exposes a public memset wrapper, and adds -nostdlib to build flags. Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel as SystemInitS2
participant PitInst as PitInstall()
participant HW as PIT Hardware
Kernel->>PitInst: PitInstall()
PitInst->>HW: Program PIT (mode/divisor)
PitInst-->>Kernel: return
sequenceDiagram
participant Proc as SystemService
participant Pit as PitSetFrequency(hz)
participant HW as PIT I/O (0x43/0x40)
Proc->>Pit: PitSetFrequency(hz)
Pit->>Pit: save flags & cli
Pit->>HW: write command 0x36 to 0x43
Pit->>HW: write divisor LSB/MSB to 0x40
Pit->>Pit: restore flags
Pit-->>Proc: return
sequenceDiagram
participant Thread as Yielding Thread
participant Lock as SpinLockIrqSave
participant Sched as Scheduler
participant Unlock as SpinUnlockIrqRestore
Thread->>Lock: acquire (save flags)
Thread->>Sched: perform yield/handover
Thread->>Unlock: release (restore flags)
Thread-->>Thread: halt/wait
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ 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 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: 5
🔭 Outside diff range comments (1)
kernel/process/Process.c (1)
905-918: Yield hlt can hang if IF=0; also constrain state transition to RUNNING only
- If Yield is ever invoked with interrupts disabled on entry, SpinUnlockIrqRestore will restore IF=0 and the subsequent hlt will stall the CPU until an NMI/SMI. Either enforce “Yield is only called with IRQs enabled” or unconditionally enable interrupts before halting.
- Minor: only transition RUNNING -> READY to avoid clobbering other states.
Suggested change:
void Yield() { - irq_flags_t flags = SpinLockIrqSave(&scheduler_lock); + irq_flags_t flags = SpinLockIrqSave(&scheduler_lock); Process* current = GetCurrentProcess(); - if (current) { + if (current && current->state == PROC_RUNNING) { // A process that yields is ready to run again, just giving up its timeslice. // Setting it to PROC_BLOCKED was incorrect as there was no corresponding unblock mechanism. current->state = PROC_READY; } RequestSchedule(); // This instruction halts the CPU until the next interrupt (e.g., the timer), // which will then trigger the scheduler. - SpinUnlockIrqRestore(&scheduler_lock, flags); - __asm__ __volatile__("hlt"); + SpinUnlockIrqRestore(&scheduler_lock, flags); + // Option A (preferred contract for process-context Yield): ensure IF=1 before halting + __asm__ __volatile__("sti; hlt"); + // Option B (if keeping original contract): add an assertion/guard to prevent hlt with IF=0 + // (requires an isr/flags helper; omitted if not available) }Note: Option A changes the contract to always enable IRQs before halting; choose based on your intended calling semantics for Yield.
🧹 Nitpick comments (2)
drivers/Pic.h (1)
6-6: Document valid frequency range for PitSetFrequencyAdd usage notes so callers know acceptable inputs (divisor must be 1..65535, so roughly 18–1,193,180 Hz). This prevents undefined behavior in the implementation.
Proposed header comment addition:
void PitInstall(); -void PitSetFrequency(uint16_t hz); +// Sets PIT channel 0 frequency. Valid range produces divisor in [1, 65535]: +// hz in [1193180/65535 (~18), 1193180/1 (1,193,180)] +// Values outside will be clamped by the implementation. +void PitSetFrequency(uint16_t hz);kernel/process/Process.c (1)
9-9: Layering: avoid coupling Process scheduler to PIC headerProcess.c including Pic.h just to call PitSetFrequency couples the scheduler to the PIC driver. Prefer a dedicated PIT/timer header to keep boundaries clean.
- Move PitSetFrequency (and PIT_FREQUENCY_HZ) declarations into a new Pit.h (or Timer.h).
- Include that new header here instead of Pic.h.
Example change once Pit.h exists:
-#include "Pic.h" +#include "Pit.h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
drivers/Pic.c(1 hunks)drivers/Pic.h(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/memory/MemOps.c(1 hunks)kernel/memory/MemOps.h(1 hunks)kernel/process/Process.c(4 hunks)kernel/process/Process.h(2 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T02:19:46.917Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.917Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
🧬 Code Graph Analysis (3)
drivers/Pic.h (1)
drivers/Pic.c (1)
PitSetFrequency(23-33)
kernel/memory/MemOps.h (1)
kernel/memory/MemOps.c (1)
memset(41-43)
kernel/process/Process.c (2)
kernel/atomic/Spinlock.h (2)
SpinLockIrqSave(19-26)SpinUnlockIrqRestore(28-31)drivers/Pic.c (1)
PitSetFrequency(23-33)
🔇 Additional comments (4)
kernel/process/Process.h (2)
9-10: Cosmetic header rename only; no semantic impactSection header change is fine and improves clarity.
85-86: Proc privilege macro alignment only; OKNo behavioral change; aligns with existing values used elsewhere.
kernel/core/Kernel.c (1)
324-328: Combined PIC & PIT init sequence looks correct
- PicInstall() then PitInstall(); sti is issued much later (Line 547), so no spurious IRQs. IDT is already installed earlier.
- Logs are consistent with the new flow.
No changes requested.
drivers/Pic.h (1)
3-3: Including stdint is appropriate for uint16_tHeader can now safely declare PitSetFrequency with fixed-width types.
drivers/Pic.c
Outdated
Comment on lines
23
to
33
| void PitSetFrequency(uint16_t hz) { | ||
| // Disable interrupts to ensure atomic update | ||
| asm volatile("cli"); | ||
| PIT_FREQUENCY_HZ = hz; // Update the global state | ||
| const uint16_t divisor = 1193180 / hz; | ||
| outb(0x43, 0x36); | ||
| outb(0x40, divisor & 0xFF); | ||
| outb(0x40, (divisor >> 8) & 0xFF); | ||
| // Re-enable interrupts | ||
| asm volatile("sti"); | ||
| } |
There was a problem hiding this comment.
Guard against invalid hz; clamp to safe range and avoid divide-by-zero
Currently, hz==0 will cause a divide-by-zero. Also, values out of range can produce invalid divisors. Clamp hz so the resulting divisor stays within [1, 65535] and store the actual programmed frequency.
Proposed fix:
void PitSetFrequency(uint16_t hz) {
// Disable interrupts to ensure atomic update
asm volatile("cli");
- PIT_FREQUENCY_HZ = hz; // Update the global state
- const uint16_t divisor = 1193180 / hz;
+ // Clamp to valid range so divisor stays within [1, 65535]
+ // Note: 1193180/65535 ≈ 18.2 Hz; 1193180/1 = 1,193,180 Hz
+ if (hz == 0) {
+ hz = 100; // sensible default to avoid div-by-zero; choose policy as needed
+ }
+ uint32_t min_hz = 1193180u / 65535u;
+ if (hz < min_hz) hz = (uint16_t)min_hz;
+ if (hz > 1193180u) hz = 1193180u;
+
+ uint32_t div32 = 1193180u / hz;
+ if (div32 == 0) div32 = 1;
+ if (div32 > 65535u) div32 = 65535u;
+ const uint16_t divisor = (uint16_t)div32;
+
+ // Record the actual resulting frequency (avoid drift between requested vs. programmed)
+ PIT_FREQUENCY_HZ = (uint16_t)(1193180u / divisor);
outb(0x43, 0x36);
outb(0x40, divisor & 0xFF);
outb(0x40, (divisor >> 8) & 0xFF);
// Re-enable interrupts
asm volatile("sti");
}Optional micro-optimization: early-return if the clamped result equals the current PIT_FREQUENCY_HZ to avoid reprogramming.
📝 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.
Suggested change
| void PitSetFrequency(uint16_t hz) { | |
| // Disable interrupts to ensure atomic update | |
| asm volatile("cli"); | |
| PIT_FREQUENCY_HZ = hz; // Update the global state | |
| const uint16_t divisor = 1193180 / hz; | |
| outb(0x43, 0x36); | |
| outb(0x40, divisor & 0xFF); | |
| outb(0x40, (divisor >> 8) & 0xFF); | |
| // Re-enable interrupts | |
| asm volatile("sti"); | |
| } | |
| void PitSetFrequency(uint16_t hz) { | |
| // Disable interrupts to ensure atomic update | |
| asm volatile("cli"); | |
| // Clamp to valid range so divisor stays within [1, 65535] | |
| // Note: 1193180/65535 ≈ 18.2 Hz; 1193180/1 = 1 193 180 Hz | |
| if (hz == 0) { | |
| hz = 100; // sensible default to avoid div-by-zero; adjust policy as needed | |
| } | |
| uint32_t min_hz = 1193180u / 65535u; // ≈ 18 Hz | |
| if (hz < min_hz) hz = (uint16_t)min_hz; | |
| if (hz > 1193180u) hz = 1193180u; | |
| uint32_t div32 = 1193180u / hz; | |
| if (div32 == 0) div32 = 1; | |
| if (div32 > 65535u) div32 = 65535u; | |
| const uint16_t divisor = (uint16_t)div32; | |
| // Record the actual resulting frequency (avoid drift between requested vs. programmed) | |
| PIT_FREQUENCY_HZ = (uint16_t)(1193180u / divisor); | |
| outb(0x43, 0x36); | |
| outb(0x40, divisor & 0xFF); | |
| outb(0x40, (divisor >> 8) & 0xFF); | |
| // Re-enable interrupts | |
| asm volatile("sti"); | |
| } |
Comment on lines
+41
to
+43
| void* memset(void* dest, int value, unsigned long size) { // FCK GCC | ||
| return FastMemset(dest, value, size); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Polish the memset wrapper; keep signature conventional and remove unprofessional comment
- Replace the comment with a neutral rationale.
- Prefer size_t for the size parameter to match the header and conventional signatures.
-void* memset(void* dest, int value, unsigned long size) { // FCK GCC
- return FastMemset(dest, value, size);
-}
+void* memset(void* dest, int value, size_t size) { // Provide standard-compatible memset symbol in freestanding kernel
+ return FastMemset(dest, value, (uint64_t)size);
+}Additionally, ensure FastMemset masks the byte value to 0xFF in the SSE path (outside the changed lines); otherwise, non-8-bit values or negative ints can produce incorrect fill patterns.
Outside selected lines, suggested fix in FastMemset:
// before building val64
uint8_t b = (uint8_t)value;
uint64_t val64 = ((uint64_t)b << 56) | ((uint64_t)b << 48) |
((uint64_t)b << 40) | ((uint64_t)b << 32) |
((uint64_t)b << 24) | ((uint64_t)b << 16) |
((uint64_t)b << 8) | (uint64_t)b;🤖 Prompt for AI Agents
In kernel/memory/MemOps.c around lines 41 to 43, the memset wrapper uses a
nonstandard signature and an unprofessional comment; change the function
signature to the conventional form using size_t for the size parameter (i.e.
void *memset(void *dest, int value, size_t size)), replace the informal comment
with a short neutral rationale (e.g. "Wrapper forwarding to optimized
FastMemset"), and update call sites if necessary; additionally, outside these
lines update FastMemset's SSE path to mask the incoming int value to a single
byte (0xFF) before replicating into wider words so non-8-bit or negative ints
produce correct fill patterns.
| void* FastMemcpy(void* dest, const void* src, uint64_t size); | ||
| int FastMemcmp(const void* ptr1, const void* ptr2, uint64_t size); | ||
| void FastZeroPage(void* page); | ||
| void* memset(void* dest, int value, unsigned long size); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align memset signature with conventional size_t and internal usage
Using unsigned long works on x86_64 (size_t == unsigned long), but declaring with size_t communicates intent and matches conventional prototypes and compiler expectations.
Minimal change (keeping freestanding constraints):
-void* memset(void* dest, int value, unsigned long size);
+// Use size_t for better compatibility with compiler-emitted calls.
+void* memset(void* dest, int value, size_t size);If stddef.h is unavailable in your freestanding environment, ensure size_t is defined in a kernel-provided header before this declaration.
Do we have a kernel-provided typedef for size_t (e.g., in a local stddef.h)? If not, I can add one where appropriate.
🏁 Script executed:
#!/bin/bash
# Search for an existing typedef of size_t in the kernel or common headers
echo "=== Searching for 'typedef.*size_t' ==="
rg -n "typedef\s+.*size_t" || echo "No typedef of size_t found."
echo ""
# Search for stddef.h includes that might define size_t
echo "=== Searching for '#include.*stddef.h' ==="
rg -n "#include.*stddef.h" || echo "No stddef.h includes found."
echo ""
# Search for any usage of size_t to see where it’s expected
echo "=== Searching for 'size_t' usage ==="
rg -n "size_t" || echo "No uses of size_t found."Length of output: 3278
🏁 Script executed:
#!/bin/bash
# Display include/stdint.h around size_t definition
echo "=== include/stdint.h ==="
sed -n '1,50p' include/stdint.h || echo "include/stdint.h not found"
echo ""
# Display include/stddef.h content
echo "=== include/stddef.h ==="
sed -n '1,50p' include/stddef.h || echo "include/stddef.h not found"
echo ""
# Display kernel/memory/MemOps.h context
echo "=== kernel/memory/MemOps.h ==="
sed -n '1,200p' kernel/memory/MemOps.h || echo "kernel/memory/MemOps.h not found"Length of output: 2116
Use size_t for memset’s size parameter in MemOps.h
To match the freestanding size_t (which is defined as uint64_t in include/stddef.h) and keep consistency with your other memory APIs, update the signature and include the proper header:
• File: kernel/memory/MemOps.h
– Replace the unsigned long parameter with size_t.
– Add #include "stddef.h" to pull in the kernel’s size_t definition.
Minimal diff:
--- a/kernel/memory/MemOps.h
+++ b/kernel/memory/MemOps.h
@@
-#include "stdint.h"
+#include "stddef.h" // for size_t (uint64_t)
+#include "stdint.h"
-void* memset(void* dest, int value, unsigned long size);
+void* memset(void* dest, int value, size_t size);📝 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.
Suggested change
| void* memset(void* dest, int value, unsigned long size); | |
| #include "stddef.h" // for size_t (uint64_t) | |
| #include "stdint.h" | |
| void* memset(void* dest, int value, size_t size); |
🤖 Prompt for AI Agents
In kernel/memory/MemOps.h around line 10, the memset declaration uses unsigned
long for the size parameter which should be the freestanding size_t; update the
function signature to use size_t for the size parameter and add #include
"stddef.h" at the top of the header to pull in the kernel's size_t definition so
the declaration matches other memory APIs and the architecture-wide typedef.
Comment on lines
1224
to
1228
| // Hysteresis - only change if difference is significant | ||
| if (ABSi(new_freq - controller.current_freq) > 10) { | ||
| UpdatePIT(new_freq); | ||
| PitSetFrequency(new_freq); | ||
| controller.current_freq = new_freq; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
PitSetFrequency usage: API unconditionally cli/sti — risks re-enabling IRQs unexpectedly
Calling PitSetFrequency(new_freq) here is fine, but the implementation shown in drivers/Pic.c does cli/sti without saving/restoring prior flags. If any caller invokes it with IRQs disabled, it will inadvertently re-enable interrupts on return. Make PitSetFrequency preserve and restore IF.
Also compute the PIT divisor in wider width, then narrow to 16-bit, for safety.
Proposed fix (in drivers/Pic.c):
-void PitSetFrequency(uint16_t hz) {
- // Disable interrupts to ensure atomic update
- asm volatile("cli");
- PIT_FREQUENCY_HZ = hz; // Update the global state
- const uint16_t divisor = 1193180 / hz;
- outb(0x43, 0x36);
- outb(0x40, divisor & 0xFF);
- outb(0x40, (divisor >> 8) & 0xFF);
- // Re-enable interrupts
- asm volatile("sti");
-}
+void PitSetFrequency(uint16_t hz) {
+ irq_flags_t flags = save_irq_flags();
+ cli();
+ PIT_FREQUENCY_HZ = hz; // Update the global state (see note on volatile below)
+ const uint32_t div32 = 1193180u / (hz ? hz : 1u);
+ const uint16_t divisor = (uint16_t)div32; // clamped by caller; ensure 16-bit
+ outb(0x43, 0x36);
+ outb(0x40, (uint8_t)(divisor & 0xFF));
+ outb(0x40, (uint8_t)((divisor >> 8) & 0xFF));
+ restore_irq_flags(flags);
+}Additional suggestions:
- Declare PIT_FREQUENCY_HZ as volatile in a shared header to avoid stale reads in reporting paths.
- In Pit.h:
- extern volatile uint16_t PIT_FREQUENCY_HZ;
- In Pit.h:
- Keep the process layer including Pit.h (not Pic.h) to reflect ownership.
To ensure there are no unsafe call sites that expect IRQs to remain disabled after PitSetFrequency returns, scan the repo for usages:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A3 -B3 $'\\bPitSetFrequency\\s*\\('Length of output: 1073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display the full PitSetFrequency implementation for review
rg -n -A10 -B2 'void PitSetFrequency' drivers/Pic.cLength of output: 471
Preserve and restore IRQ flags in PitSetFrequency
PitSetFrequency currently does an unconditional cli/sti pair, which will re-enable interrupts even if they were already disabled. It also computes the PIT divisor in 16-bit width (and panics on hz==0). Please update as follows:
• drivers/Pic.c
void PitSetFrequency(uint16_t hz) {
- // Disable interrupts to ensure atomic update
- asm volatile("cli");
- PIT_FREQUENCY_HZ = hz; // Update the global state
- const uint16_t divisor = 1193180 / hz;
+ // Preserve IRQ state, disable interrupts, then restore
+ irq_flags_t flags = save_irq_flags();
+ cli();
+ PIT_FREQUENCY_HZ = hz; // Update the global state
+ // Compute divisor in 32-bit, avoid divide-by-zero, then narrow
+ uint32_t div32 = 1193180u / (hz ? hz : 1u);
+ uint16_t divisor = (uint16_t)div32;
outb(0x43, 0x36);
outb(0x40, divisor & 0xFF);
outb(0x40, (divisor >> 8) & 0xFF);
- // Re-enable interrupts
- asm volatile("sti");
+ restore_irq_flags(flags);
}• drivers/Pic.h
- void PitSetFrequency(uint16_t hz);
+ extern volatile uint16_t PIT_FREQUENCY_HZ;
+ void PitSetFrequency(uint16_t hz);• Call sites checked:
– kernel/process/Process.c:1226 (no callers expect IRQs to remain disabled)
📝 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.
Suggested change
| // Hysteresis - only change if difference is significant | |
| if (ABSi(new_freq - controller.current_freq) > 10) { | |
| UpdatePIT(new_freq); | |
| PitSetFrequency(new_freq); | |
| controller.current_freq = new_freq; | |
| --- a/drivers/Pic.c | |
| +++ b/drivers/Pic.c | |
| @@ | |
| void PitSetFrequency(uint16_t hz) { | |
| - // Disable interrupts to ensure atomic update | |
| - asm volatile("cli"); | |
| - PIT_FREQUENCY_HZ = hz; // Update the global state | |
| - const uint16_t divisor = 1193180 / hz; | |
| + // Preserve IRQ state, disable interrupts, then restore | |
| + irq_flags_t flags = save_irq_flags(); | |
| + cli(); | |
| + PIT_FREQUENCY_HZ = hz; // Update the global state | |
| + // Compute divisor in 32-bit, avoid divide-by-zero, then narrow | |
| + uint32_t div32 = 1193180u / (hz ? hz : 1u); | |
| + uint16_t divisor = (uint16_t)div32; | |
| outb(0x43, 0x36); | |
| outb(0x40, divisor & 0xFF); | |
| outb(0x40, (divisor >> 8) & 0xFF); | |
| - // Re-enable interrupts | |
| - asm volatile("sti"); | |
| + restore_irq_flags(flags); | |
| } |
Suggested change
| // Hysteresis - only change if difference is significant | |
| if (ABSi(new_freq - controller.current_freq) > 10) { | |
| UpdatePIT(new_freq); | |
| PitSetFrequency(new_freq); | |
| controller.current_freq = new_freq; | |
| --- a/drivers/Pic.h | |
| +++ b/drivers/Pic.h | |
| @@ | |
| - void PitSetFrequency(uint16_t hz); | |
| + extern volatile uint16_t PIT_FREQUENCY_HZ; | |
| + void PitSetFrequency(uint16_t hz); |
🤖 Prompt for AI Agents
In kernel/process/Process.c around lines 1224 to 1228, the call to
PitSetFrequency(new_freq) must not rely on PitSetFrequency unconditionally
disabling and re-enabling IRQs; update drivers/Pic.c and drivers/Pic.h so
PitSetFrequency preserves and restores IRQ flags instead of doing a plain
cli/sti pair, i.e. save CPU interrupt flag state at entry and restore it on
exit; also change the PIT divisor computation to use an unsigned 16-bit width
and avoid panicking on hz==0 (handle zero by clamping to a valid minimum or
returning an error), and ensure the function prototype in Pic.h reflects the
same behavior; note that the caller at kernel/process/Process.c:1226 does not
expect IRQs to remain disabled so the saved/restored flags approach will
preserve caller semantics.
There was a problem hiding this comment.
hmm interesting the freeze after kill came back but its only happen in ArpRequestTestProcess, can you check?
There was a problem hiding this comment.
🧩 Analysis chain
@assembler-0, let me investigate the ArpRequestTestProcess freeze issue.
🏁 Script executed:
#!/bin/bash
# Search for ArpRequestTestProcess definition and usage
echo "=== Searching for ArpRequestTestProcess ==="
rg -n "ArpRequestTestProcess" -A 10 -B 2
echo -e "\n=== Checking for ARP-related code ==="
rg -n "ARP|Arp" --type c -l | head -10
echo -e "\n=== Looking for network test processes ==="
rg -n "TestProcess|test.*process" -i --type c | grep -E "(void|Process|create)" -A 2 | head -20
Length of output: 2070
🏁 Script executed:
#!/bin/bash
# Get the complete ArpRequestTestProcess implementation
echo "=== Full ArpRequestTestProcess implementation ==="
rg -n "^void ArpRequestTestProcess" -A 50 kernel/etc/Shell.c
echo -e "\n=== Check for RTL8139 blocking operations ==="
rg -n "GetRtl8139Device|Rtl8139Send|Rtl8139" --type c | grep -E "(while|wait|block|spin)" -B 2 -A 2 | head -20
echo -e "\n=== Check if ArpRequestTestProcess has loops or waits ==="
rg -n "ArpRequestTestProcess" -A 30 kernel/etc/Shell.c | grep -E "(while|for|wait|sleep|Yield)"
Length of output: 2560
🏁 Script executed:
#!/bin/bash
# Get more lines of ArpRequestTestProcess to see the send operation
echo "=== Complete ArpRequestTestProcess function ==="
rg -n "^void ArpRequestTestProcess" -A 70 kernel/etc/Shell.c
echo -e "\n=== Search for Rtl8139Send function implementation ==="
rg -n "Rtl8139Send|RTL8139.*Send" -B 2 -A 10 --type c
echo -e "\n=== Check process termination with network operations ==="
rg -n "io_operations|IO_BOOST" kernel/process/Process.c | head -10
Length of output: 3457
🏁 Script executed:
#!/bin/bash
# Find Rtl8139_SendPacket implementation
echo "=== Searching for Rtl8139_SendPacket implementation ==="
fd -e c -e h | xargs rg -l "Rtl8139_SendPacket" 2>/dev/null
echo -e "\n=== Get Rtl8139_SendPacket full implementation ==="
rg -n "Rtl8139_SendPacket" -B 5 -A 30 --type c
echo -e "\n=== Check for spinlocks or waits in RTL8139 driver ==="
fd "rtl8139" -i -e c | xargs rg -n "while|SpinLock|wait" 2>/dev/null | head -20
Length of output: 5510
@assembler-0, I found the likely cause of the freeze when killing ArpRequestTestProcess!
The issue is with the PitSetFrequency function's interrupt handling. Here's what's happening:
- ArpRequestTestProcess sends an ARP packet and completes normally (no blocking operations found)
- When you kill it, the termination path likely triggers a PIT frequency adjustment (via SystemService)
- PitSetFrequency uses unconditional cli/sti which can corrupt the interrupt state
The problem occurs because PitSetFrequency (drivers/Pic.c:23-33) does:
asm volatile("cli"); // Disables interrupts
// ... PIT programming ...
asm volatile("sti"); // ALWAYS re-enables interruptsIf the caller had interrupts disabled (like during process termination), this incorrectly re-enables interrupts, causing race conditions and freezes.
Fix: Update PitSetFrequency to preserve interrupt state:
void PitSetFrequency(uint16_t hz) {
// Save current interrupt state
irq_flags_t flags = save_irq_flags();
cli();
PIT_FREQUENCY_HZ = hz;
// Safer divisor calculation
uint32_t div32 = 1193180u / (hz ? hz : 1u);
uint16_t divisor = (uint16_t)div32;
outb(0x43, 0x36);
outb(0x40, divisor & 0xFF);
outb(0x40, (divisor >> 8) & 0xFF);
// Restore previous interrupt state
restore_irq_flags(flags);
}This preserves the interrupt flag state across the PIT frequency change, preventing the corruption that causes the freeze. The ArpRequestTestProcess termination likely triggers a specific timing that exposes this bug.
Would you like me to verify if there are any other unsafe interrupt manipulations in the codebase?
🧬 Code Graph Analysis Results
- kernel/atomic/Atomics.c: lines 6-8
void AtomicInc(volatile uint32_t* ptr) {
asm volatile("lock incl %0" : "+m" (*ptr));
}
- kernel/atomic/Atomics.c: lines 9-11
void AtomicDec(volatile uint32_t* ptr) {
asm volatile("lock decl %0" : "+m" (*ptr));
}
- kernel/atomic/Atomics.c: lines 12-21
int AtomicCmpxchg(volatile uint32_t* ptr, int expected, int desired) {
int old;
asm volatile(
"lock cmpxchgl %2, %1"
: "=a" (old), "+m" (*ptr)
: "r" (desired), "0" (expected)
: "memory"
);
return old;
}
- kernel/atomic/Atomics.c: lines 22-24
uint32_t AtomicRead(volatile uint32_t* ptr) {
return __atomic_load_n(ptr, __ATOMIC_SEQ_CST);
}
- kernel/atomic/Spinlock.h: lines 19-26
static inline irq_flags_t SpinLockIrqSave(volatile int* lock) {
irq_flags_t flags = save_irq_flags();
cli();
while (__sync_lock_test_and_set(lock, 1)) {
while (*lock) __builtin_ia32_pause();
}
return flags;
}
- kernel/atomic/Spinlock.h: lines 28-31
static inline void SpinUnlockIrqRestore(volatile int* lock, irq_flags_t flags) {
__sync_lock_release(lock);
restore_irq_flags(flags);
}
- kernel/atomic/Spinlock.h: lines 8-12
static inline void SpinLock(volatile int* lock) {
while (__sync_lock_test_and_set(lock, 1)) {
while (*lock) __builtin_ia32_pause();
}
}
- kernel/atomic/Spinlock.h: lines 14-16
static inline void SpinUnlock(volatile int* lock) {
__sync_lock_release(lock);
}
- kernel/ memory/VMem.c: lines 336-356
void* VMemAllocWithGuards(uint64_t size) {
if (size == 0) return NULL;
// Add space for guard pages (one before, one after)
uint64_t total_size = size + (2 * PAGE_SIZE);
void* base = VMemAlloc(total_size);
if (!base) return NULL;
uint64_t base_addr = (uint64_t)base;
// Map guard pages with no permissions (just present bit)
VMemMap(base_addr, VMemGetPhysAddr(base_addr), PAGE_PRESENT); // First guard
VMemMap(base_addr + PAGE_SIZE + size,
VMemGetPhysAddr(base_addr + PAGE_SIZE + size), PAGE_PRESENT); // Last guard
// Fill guard pages with pattern
*(uint64_t*)base_addr = GUARD_PAGE_PATTERN;
*(uint64_t*)(base_addr + PAGE_SIZE + size) = GUARD_PAGE_PATTERN;
return (void*)(base_addr + PAGE_SIZE); // Return user-accessible area
}
- kernel/ memory/VMem.c: lines 358-374
void VMemFreeWithGuards(void* ptr, uint64_t size) {
if (!ptr) return;
uint64_t base_addr = (uint64_t)ptr - PAGE_SIZE;
// Check guard pages for corruption
if (*(uint64_t*)base_addr != GUARD_PAGE_PATTERN) {
PrintKernelError("[VMEM] Guard page corruption detected at start!\n");
PANIC("Guard page corruption detected at start");
}
if (*(uint64_t*)(base_addr + PAGE_SIZE + size) != GUARD_PAGE_PATTERN) {
PrintKernelError("[VMEM] Guard page corruption detected at end!\n");
PANIC("Guard page corruption detected at end");
}
VMemFree((void*)base_addr, size + (2 * PAGE_SIZE));
}
- kernel/memory/MemOps.c: lines 45-76
void* FastMemset(void* dest, int value, uint64_t size) {
ASSERT(dest != NULL);
CpuFeatures* features = GetCpuFeatures();
uint8_t* d = (uint8_t*)dest;
if (features->sse2 && size >= 16) {
// Create a 128-bit value where all bytes are 'value'
uint64_t val64 = ((uint64_t)value << 56) | ((uint64_t)value << 48) |
((uint64_t)value << 40) | ((uint64_t)value << 32) |
((uint64_t)value << 24) | ((uint64_t)value << 16) |
((uint64_t)value << 8) | value;
asm volatile(
"movq %0, %%xmm0\n"
"punpcklqdq %%xmm0, %%xmm0\n"
"punpcklqdq %%xmm0, %%xmm0\n"
:
: "r"(val64)
: "xmm0"
);
while (size >= 16) {
asm volatile("movdqu %%xmm0, (%0)" : : "r"(d) : "memory");
d += 16;
size -= 16;
}
}
// Handle remaining bytes
while (size--) *d++ = value;
return dest;
}
- kernel/memory/MemOps.c: lines 78-101
void* FastMemcpy(void* dest, const void* src, uint64_t size) {
ASSERT(dest != NULL && src != NULL);
CpuFeatures* features = GetCpuFeatures();
uint8_t* d = (uint8_t*)dest;
const uint8_t* s = (const uint8_t*)src;
if (features->sse2 && size >= 16) {
while (size >= 16) {
asm volatile(
"movdqu (%1), %%xmm0\n"
"movdqu %%xmm0, (%0)\n"
:
: "r"(d), "r"(st)
: "xmm0", "memory"
);
d += 16;
s += 16;
size -= 16;
}
}
while (size--) *d++ = *s++;
return dest;
}
- kernel/etc/Console.c: lines 173-189
void PrintKernel(const char* str) {
if (!str) return;
SpinLock(&lock);
if (use_vbe) {
VBEConsolePrint(str);
} else {
const uint8_t original_color = console.color;
for (const char* p = str; *p; p++) {
ConsolePutchar(*p);
}
console.color = original_color;
}
SpinUnlock(&lock);
SerialWrite(str);
}
- kernel/etc/Console.c: lines 199-203
void PrintKernelSuccess(const char* str) {
ConsoleSetColor(VGA_COLOR_WHITE);
PrintKernel(str);
ConsoleSetColor(VGA_COLOR_DEFAULT);
}
- kernel/etc/Console.c: lines 205-209
void PrintKernelError(const char* str) {
ConsoleSetColor(VGA_COLOR_ERROR);
PrintKernel(str);
ConsoleSetColor(VGA_COLOR_DEFAULT);
}
- kernel/etc/Console.c: lines 211-215
void PrintKernelWarning(const char* str) {
ConsoleSetColor(VGA_COLOR_WARNING);
PrintKernel(str);
ConsoleSetColor(VGA_COLOR_DEFAULT);
}
- kernel/etc/Console.c: lines 218-241
void PrintKernelHex(uint64_t num) {
static const char hex_chars[] = "0123456789ABCDEF";
char buffer[19];
buffer[0] = '0';
buffer[1] = 'x';
if (num == 0) {
buffer[2] = '0';
buffer[3] = '\0';
PrintKernel(buffer);
return;
}
int pos = 18;
buffer[pos--] = '\0';
while (num > 0 && pos >= 2) {
buffer[pos--] = hex_chars[num & 0xF];
num >>= 4;
}
PrintKernel(&buffer[pos + 1]);
}
- kernel/etc/Shell.c: lines 433-462
void ShellProcess(void) {
PrintKernelSuccess("[SYSTEM] VoidFrame Shell v0.0.1-beta\n");
show_help();
while (1) {
if (HasInput()) {
char c = GetChar();
if (c == '\n') {
PrintKernel("\n"); // Move to next line
command_buffer[cmd_pos] = 0;
ExecuteCommand(command_buffer);
cmd_pos = 0;
PrintKernel(current_dir);
PrintKernel("> ");
} else if (c == '\b') {
if (cmd_pos > 0) {
cmd_pos--;
PrintKernel("\b \b"); // Visual backspace
}
} else if (cmd_pos < 255) {
command_buffer[cmd_pos++] = c;
char str[2] = {c, 0};
PrintKernel(str); // Echo character
}
} else {
// Yield CPU when no input available
Yield();
}
}
}
- drivers/Pic.c: lines 23-33
void PitSetFrequency(uint16_t hz) {
// Disable interrupts to ensure atomic update
asm volatile("cli");
PIT_FREQUENCY_HZ = hz; // Update the global state
const uint16_t divisor = 1193180 / hz;
outb(0x43, 0x36);
outb(0x40, divisor & 0xFF);
outb(0x40, (divisor >> 8) & 0xFF);
// Re-enable interrupts
asm volatile("sti");
}
- include/stdlib.h: lines 4-9
static inline int ABSi(const int x) {
if (x < 0) {
return -x;
}
return x;
}
There was a problem hiding this comment.
oh that fixes can can you consider use fixed precision rather than floating point since floats are 🔥
There was a problem hiding this comment.
@assembler-0, absolutely! Floating-point in kernel space is indeed problematic. Here's a fixed-point implementation that replaces all the float operations:
void SystemService(void) {
// Fixed-point arithmetic using scale factor of 1000 (3 decimal precision)
#define FP_SCALE 1000
#define FP_HALF (FP_SCALE / 2)
PITController controller = {
.min_freq = 100,
.max_freq = 1000,
.current_freq = PIT_FREQUENCY_HZ,
.learning_rate = 100, // 0.1 * 1000
.momentum = 700, // 0.7 * 1000
.last_adjustment = 0,
.power_state = 0
};
uint64_t last_sample_time = GetSystemTicks();
uint64_t last_context_switches = context_switches;
PrintKernelSuccess("[SYSTEM] Advanced frequency controller active (fixed-point)\n");
while (1) {
uint64_t current_time = GetSystemTicks();
uint64_t time_delta = current_time - last_sample_time;
if (time_delta >= 50) { // Sample every 50 ticks
// Gather system metrics
int process_count = __builtin_popcountll(active_process_bitmap);
int ready_count = __builtin_popcountll(ready_process_bitmap);
uint64_t cs_delta = context_switches - last_context_switches;
// Calculate system load (0-1000 instead of 0.0-1.0)
int32_t load = (ready_count * FP_SCALE) / MAX_PROCESSES;
// Calculate context switch rate (scaled by 1000)
int32_t cs_rate = (cs_delta * FP_SCALE) / time_delta;
// Analyze queue depths
uint32_t total_queue_depth = 0;
uint32_t max_queue_depth = 0;
for (int i = 0; i < MAX_PRIORITY_LEVELS; i++) {
uint32_t depth = MLFQscheduler.queues[i].count;
total_queue_depth += depth;
if (depth > max_queue_depth) max_queue_depth = depth;
}
// Predictive frequency calculation
int32_t target_freq = controller.min_freq;
// Factor 1: Process count
target_freq += (process_count - 1) * 30;
// Factor 2: Queue pressure
if (max_queue_depth > 3) {
target_freq += max_queue_depth * 20;
}
// Factor 3: Context switch rate (thrashing detection)
if (cs_rate > 10 * FP_SCALE) { // cs_rate > 10.0
// Multiply by 1.2: (freq * 1200) / 1000
target_freq = (target_freq * 1200) / FP_SCALE;
} else if (cs_rate < 2 * FP_SCALE && process_count > 2) { // cs_rate < 2.0
// Multiply by 0.9: (freq * 900) / 1000
target_freq = (target_freq * 900) / FP_SCALE;
}
// Factor 4: Power-aware scaling
if (total_queue_depth == 0 && process_count <= 2) {
controller.power_state = 0; // Low power
target_freq = controller.min_freq;
} else if (load > 700) { // load > 0.7 (70%)
controller.power_state = 2; // Turbo
// Multiply by 1.3: (freq * 1300) / 1000
target_freq = (target_freq * 1300) / FP_SCALE;
} else {
controller.power_state = 1; // Normal
}
// Apply momentum for smooth transitions (fixed-point)
// adjustment = (target_freq - current_freq) * learning_rate
int32_t freq_diff = target_freq - controller.current_freq;
int32_t adjustment = (freq_diff * controller.learning_rate) / FP_SCALE;
// Add momentum: adjustment + momentum * last_adjustment
adjustment = adjustment + (controller.momentum * controller.last_adjustment) / FP_SCALE;
controller.last_adjustment = adjustment;
int32_t new_freq = controller.current_freq + adjustment;
// Clamp to limits
if (new_freq < controller.min_freq) new_freq = controller.min_freq;
if (new_freq > controller.max_freq) new_freq = controller.max_freq;
// Hysteresis - only change if difference is significant
if (ABSi(new_freq - controller.current_freq) > 10) {
PitSetFrequency((uint16_t)new_freq);
controller.current_freq = new_freq;
// Log the change
if (current_time % 500 == 0) {
PrintKernel("[TRACER] Freq: ");
PrintKernelInt(new_freq);
PrintKernel("Hz, Load: ");
PrintKernelInt((load * 100) / FP_SCALE); // Convert back to percentage
PrintKernel("%, Power: ");
PrintKernelInt(controller.power_state);
PrintKernel("\n");
}
}
// Update history
uint32_t idx = controller.history_index % FREQ_HISTORY_SIZE;
controller.history[idx] = (FrequencyHistory){
.timestamp = current_time,
.process_count = process_count,
.frequency = controller.current_freq,
.context_switches = cs_delta,
.avg_latency = total_queue_depth
};
controller.history_index++;
last_sample_time = current_time;
last_context_switches = context_switches;
}
CleanupTerminatedProcesses();
Yield();
}
#undef FP_SCALE
#undef FP_HALF
}Also update the PITController struct definition:
typedef struct {
uint16_t min_freq;
uint16_t max_freq;
int32_t current_freq;
int32_t learning_rate; // Fixed-point (scaled by 1000)
int32_t momentum; // Fixed-point (scaled by 1000)
int32_t last_adjustment; // Fixed-point
uint8_t power_state;
FrequencyHistory history[FREQ_HISTORY_SIZE];
uint32_t history_index;
} PITController;This eliminates all floating-point operations while maintaining the same behavior with 3-decimal precision using integer math. Much more efficient and kernel-safe! 🚀
✏️ Learnings added
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.692Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@assembler-0, absolutely right! Floating point in kernel space is dangerous and can corrupt FPU state. Here's a fixed-point conversion for SystemService:
void SystemService(void) {
// Fixed-point arithmetic using 16.16 format (16 bits integer, 16 bits fraction)
#define FP_SHIFT 16
#define FP_ONE (1 << FP_SHIFT)
#define FP_HALF (1 << (FP_SHIFT - 1))
typedef struct {
uint16_t min_freq;
uint16_t max_freq;
uint16_t current_freq;
int32_t learning_rate; // 0.1 = 6554 in 16.16 fixed point
int32_t momentum; // 0.7 = 45875 in 16.16 fixed point
int32_t last_adjustment;
uint8_t power_state;
FrequencyHistory history[FREQ_HISTORY_SIZE];
uint32_t history_index;
} PITControllerFixed;
PITControllerFixed controller = {
.min_freq = 100,
.max_freq = 1000,
.current_freq = PIT_FREQUENCY_HZ,
.learning_rate = 6554, // 0.1 * 65536
.momentum = 45875, // 0.7 * 65536
.last_adjustment = 0,
.power_state = 0,
.history_index = 0
};
uint64_t last_sample_time = GetSystemTicks();
uint64_t last_context_switches = context_switches;
PrintKernelSuccess("[SYSTEM] Advanced frequency controller active (fixed-point)\n");
while (1) {
uint64_t current_time = GetSystemTicks();
uint64_t time_delta = current_time - last_sample_time;
if (time_delta >= 50) {
// Gather system metrics
int process_count = __builtin_popcountll(active_process_bitmap);
int ready_count = __builtin_popcountll(ready_process_bitmap);
uint64_t cs_delta = context_switches - last_context_switches;
// Calculate system load (0-100 represented as 0-100)
uint32_t load_percent = (ready_count * 100) / MAX_PROCESSES;
// Calculate context switch rate (switches per tick * 256 for precision)
uint32_t cs_rate_x256 = (cs_delta << 8) / time_delta;
// Analyze queue depths
uint32_t total_queue_depth = 0;
uint32_t max_queue_depth = 0;
for (int i = 0; i < MAX_PRIORITY_LEVELS; i++) {
uint32_t depth = MLFQscheduler.queues[i].count;
total_queue_depth += depth;
if (depth > max_queue_depth) max_queue_depth = depth;
}
// Calculate target frequency using integer math
int32_t target_freq = controller.min_freq;
// Factor 1: Process count
target_freq += (process_count - 1) * 30;
// Factor 2: Queue pressure
if (max_queue_depth > 3) {
target_freq += max_queue_depth * 20;
}
// Factor 3: Context switch rate (threshold at 10 switches/tick)
if (cs_rate_x256 > (10 << 8)) { // High switching
target_freq = (target_freq * 6) / 5; // 1.2x boost
} else if (cs_rate_x256 < (2 << 8) && process_count > 2) {
target_freq = (target_freq * 9) / 10; // 0.9x reduction
}
// Factor 4: Power-aware scaling
if (total_queue_depth == 0 && process_count <= 2) {
controller.power_state = 0;
target_freq = controller.min_freq;
} else if (load_percent > 70) {
controller.power_state = 2;
target_freq = (target_freq * 13) / 10; // 1.3x turbo
} else {
controller.power_state = 1;
}
// Apply momentum using fixed-point math
// adjustment = (target - current) * learning_rate + momentum * last_adjustment
int32_t freq_diff = target_freq - controller.current_freq;
int32_t adjustment_fp = (freq_diff * controller.learning_rate) >> FP_SHIFT;
adjustment_fp += (controller.momentum * controller.last_adjustment) >> FP_SHIFT;
controller.last_adjustment = adjustment_fp;
int16_t new_freq = controller.current_freq + adjustment_fp;
// Clamp to limits
if (new_freq < controller.min_freq) new_freq = controller.min_freq;
if (new_freq > controller.max_freq) new_freq = controller.max_freq;
// Hysteresis - only change if difference is significant
if (ABSi(new_freq - controller.current_freq) > 10) {
PitSetFrequency(new_freq);
controller.current_freq = new_freq;
// Log changes periodically
if (current_time % 500 == 0) {
PrintKernel("[TRACER] Freq: ");
PrintKernelInt(new_freq);
PrintKernel("Hz, Load: ");
PrintKernelInt(load_percent);
PrintKernel("%, Power: ");
PrintKernelInt(controller.power_state);
PrintKernel("\n");
}
}
// Update history
uint32_t idx = controller.history_index % FREQ_HISTORY_SIZE;
controller.history[idx] = (FrequencyHistory){
.timestamp = current_time,
.process_count = process_count,
.frequency = controller.current_freq,
.context_switches = cs_delta,
.avg_latency = total_queue_depth
};
controller.history_index++;
last_sample_time = current_time;
last_context_switches = context_switches;
}
CleanupTerminatedProcesses();
Yield();
}
}This fixed-point implementation:
- Uses 16.16 fixed-point format for fractional values
- Replaces all float operations with integer arithmetic
- Maintains the same control logic without FPU usage
- Is safer and more predictable in kernel space
- Avoids potential FPU state corruption issues
✏️ Learnings added
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.116Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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().
| '-finline-functions', | ||
| '-foptimize-sibling-calls', | ||
| '-nostdinc', | ||
| '-nostdlib', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
-nostdlib in compile flags is redundant; verify runtime symbol coverage
- With 'clang -c', -nostdlib has no effect (linking is done by ld later). Keeping it is harmless but misleading.
- Ensure kernel provides all runtime symbols that would otherwise come from libgcc/libssp (e.g., __stack_chk_fail/guard) given -fstack-protector-strong and no standard libs.
Consider:
- Either remove -nostdlib here, or move it to the link step if you ever drive linking via clang in the future.
Run to confirm required symbols are implemented in-tree:
If missing, add minimal implementations (StackGuard.c looks likely to provide these).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking stack protector symbols..."
rg -n --no-heading -S '__stack_chk_fail|__stack_chk_guard' -g '!**/node_modules/**' || true
echo
echo "Checking memset symbol is provided (to satisfy compiler-emitted calls under -ffreestanding/-fno-builtin)..."
rg -n --no-heading -S $'^void\\s*\\*\\s*memset\\s*\\(' || trueLength of output: 968
Remove redundant -nostdlib from compile flags in meson.build
The -nostdlib flag has no effect when you invoke clang with -c —it only matters at link time. Since all runtime symbols (stack protector symbols and memset) are already implemented in-tree, this flag is misleading here.
• File: meson.build – remove '-nostdlib', at line 28
• If you ever switch to driving linking via clang, you can re-introduce -nostdlib in your link flags instead.
🤖 Prompt for AI Agents
In meson.build around line 28, the compile flags incorrectly include '-nostdlib'
which is redundant when invoking clang with -c and misleading here; remove the
literal '-nostdlib' entry from the compile flags list so it is no longer passed
during compilation, and if you later control linking with clang add '-nostdlib'
to the link flags instead.
This was referenced Aug 11, 2025
Merged
Merged
Merged
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores