starkernel: fix lidt() inline-asm constraint hazard, land -O2
FABRIC.md item 4.5d Finding 4: lidt()'s inline asm used a register-only
("r") constraint on the idtr pointer, never telling GCC the asm
dereferences the pointee. At -O2 this let the compiler treat the
256-entry idt[] population loop and idtr_desc's field writes as dead
stores and eliminate them entirely, loading IDTR from uninitialized
stack instead of the real table -- a #GP on the first APIC timer tick
that happened to land on garbage. Same bug class as the earlier
muldiv64 fix (b43e51a): an inline-asm constraint too weak for what the
asm actually touches, invisible at -O0, live at -O2.
Fixed by switching to a memory operand ("m"(*idtr_desc)), matching how
Linux's own load_idt() is written. aarch64/riscv64 checked for the same
pattern -- neither has it, both install their vector/trap tables
entirely in hand-written .S.
Verified: all three architectures boot clean to ok>, POST Failed: 0,
identical dict-hashes across all three under -O2, zero new warnings
vs an -O0 baseline (amd64 3040/3040, aarch64 3041/3041 serial,
riscv64 3037/3037). -O2/-U_FORTIFY_SOURCE landed permanently in
COMMON_CFLAGS.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
065ab50240
commit
a1c9fc7330
@@ -5525,7 +5525,7 @@ document and committing that amendment as its own item.*
|
||||
>
|
||||
> **Chosen flags: `-O2`.** Added to `COMMON_CFLAGS` in 4.5d; no other new flags.
|
||||
|
||||
- [ ] **4.5d — Apply the chosen flags to `Makefile.starkernel`; build all three
|
||||
- [x] **4.5d — Apply the chosen flags to `Makefile.starkernel`; build all three
|
||||
architectures.** Depends on 4.5b (hazard fixed) and 4.5c (flags decided). Code change:
|
||||
add the flags to `COMMON_CFLAGS` (shared across all three `ARCH_CFLAGS` blocks, so this is
|
||||
one change, not three). Optimization can surface warnings `-O0` never triggers (e.g.
|
||||
@@ -5750,8 +5750,64 @@ document and committing that amendment as its own item.*
|
||||
> the `idt[]` array and IDTR directly across the whole pre-fault window) or a fundamentally
|
||||
> different narrowing strategy — a bigger tooling step than the tracing done so far, and one
|
||||
> this session did not attempt. 4.5d cannot complete until Finding 4 is resolved.
|
||||
>
|
||||
> **Finding 4 root-caused, 2026-08-11 — not corruption, mis-construction; static analysis,
|
||||
> no GDB session needed.** Before single-stepping, checked whether `idt[]` had a suspicious
|
||||
> `-O2`-layout neighbor (per Findings 1–3's pattern) via `nm`/`readelf` on a fresh `-O2`
|
||||
> build — and found something more direct: `arch_interrupts_init()` itself, disassembled
|
||||
> from the `-O2` object file, is only 30 bytes (`lidt` off an uninitialized 6-byte stack
|
||||
> slot → mask both PICs → `ret`). The entire 256-entry `idt[]`-population loop
|
||||
> (`set_idt_entry()` calls, `interrupts.c:461-468`) and the `idtr_desc.limit`/`.base`
|
||||
> assignments (`interrupts.c:470-472`) are gone from the compiled output — dead-store
|
||||
> eliminated. Root cause: `lidt()`'s inline asm (`interrupts.c:163-165`,
|
||||
> `__asm__ volatile ("lidt (%0)" :: "r"(idtr_desc))`) uses an `"r"` (register) constraint,
|
||||
> telling GCC only that the asm reads the *pointer value* — nothing tells it the asm
|
||||
> dereferences the pointee, so nothing anchors the writes to `idt[]` or `idtr_desc`'s fields
|
||||
> as observed. `volatile` blocks reordering/removal of the asm statement itself but does
|
||||
> nothing for dead stores feeding into it. At `-O0` this is invisible (nothing is eliminated
|
||||
> without optimization); at `-O2` GCC removes the whole population loop as unobservable,
|
||||
> and IDTR gets loaded with stack garbage instead of the real table — explaining both the
|
||||
> `#GP` on the first APIC timer tick that happened to hit a garbage descriptor slot, and the
|
||||
> earlier `limit=0`-flavored trace evidence. Same bug class and same fix shape as Finding 3
|
||||
> (`muldiv64`): an inline-asm operand constraint too weak for what the asm actually touches.
|
||||
> Precedented fix (this is exactly how Linux's own `load_idt()` is written): change the
|
||||
> constraint to `"m"(*idtr_desc)` so GCC knows the asm dereferences the struct. **Not
|
||||
> applied — reporting only, per Captain Bob's Law.** `-O2`/`-U_FORTIFY_SOURCE` build used
|
||||
> only to produce the diagnostic object file, then reverted; tree confirmed clean, back at
|
||||
> committed `-O0`. 4.5d still cannot complete until this fix is written and verified.
|
||||
>
|
||||
> **Fix applied and verified, 2026-08-11 — all three architectures.** `lidt()`
|
||||
> (`interrupts.c:163-165`) changed to `__asm__ volatile ("lidt %0" :: "m"(*idtr_desc));`.
|
||||
> amd64: disassembly of `arch_interrupts_init()` at `-O2` now shows the full (GCC-vectorized)
|
||||
> 256-entry population loop instead of the 30-byte stub; full acceptance boot reaches `ok>`
|
||||
> clean, past the prior Hermes-registration fault point, POST `Failed: 0`, Mama/Hermes parity
|
||||
> and dict-hash records present. Warning diff against a fresh `-O0` baseline: byte-for-byte
|
||||
> identical (3040 warnings both sides, zero new).
|
||||
>
|
||||
> **Checked aarch64/riscv64 for the same bug class first** (before building/booting): neither
|
||||
> has an equivalent. Both install their vector/trap table entirely in hand-written `.S`
|
||||
> (`isr.S`: `msr vbar_el1, x0` on aarch64, `csrw stvec, t0` on riscv64), not via a C loop
|
||||
> populating an array that a separate C function then hands to inline asm through a
|
||||
> pointer-only constraint. Their C-side inline asm is limited to scalar CSR/system-register
|
||||
> reads (`mrs`/`csrr`) with correctly-matched `"=r"` constraints — not the same shape as the
|
||||
> `lidt` bug, nothing to fix.
|
||||
>
|
||||
> **Both then built and booted clean at `-O2` anyway**, no changes needed: aarch64 and
|
||||
> riscv64 both reach `ok>`, POST `Failed: 0`, and — notably — identical dict-hashes to the
|
||||
> amd64 `-O2` run (`0x211a35043331d472` Mama / `0x97502db38aec4d04` Hermes), confirming
|
||||
> cross-arch parity holds under optimization, not just per-arch self-consistency. Warning
|
||||
> diffs: riscv64 identical to `-O0` (3037/3037) on first pass. aarch64's first pass showed a
|
||||
> spurious +289-warning discrepancy in `-Wmissing-field-initializers` counts (already a
|
||||
> documented, somewhat unstable warning class per this file's Code Standards section) that
|
||||
> turned out to be parallel-make (`-j`) stderr interleaving corrupting the log, not a real
|
||||
> `-O2` regression — confirmed by rerunning both `-O0` and `-O2` serially (`-j1`): identical
|
||||
> 3041/3041, zero new warnings.
|
||||
>
|
||||
> **Landed, 2026-08-11.** Captain Bob approved: `-O2`/`-U_FORTIFY_SOURCE` applied to
|
||||
> `COMMON_CFLAGS` in `Makefile.starkernel` permanently (not reverted this time), fix and
|
||||
> this writeup committed together.
|
||||
|
||||
- [ ] **4.5e — Three-arch acceptance boot with optimization enabled.** Depends on 4.5d. This
|
||||
- [x] **4.5e — Three-arch acceptance boot with optimization enabled.** Depends on 4.5d. This
|
||||
is the actual gate, per CLAUDE.md's non-negotiable acceptance criteria — a kernel that has
|
||||
only ever been built and accepted at `-O0` has no track record at any other optimization
|
||||
level, so this is a full fresh verification, not a formality.
|
||||
@@ -5759,6 +5815,13 @@ document and committing that amendment as its own item.*
|
||||
parity check still passes, logs captured in `logs/` per CLAUDE.md.
|
||||
*Refs:* 4.5, CLAUDE.md acceptance criteria.
|
||||
|
||||
> **Done, 2026-08-11.** All three architectures verified in the same session that landed the
|
||||
> fix (see 4.5d's log): amd64 (`logs/20260811-164545/amd64/`), aarch64
|
||||
> (`logs/20260811-164924/aarch64/`), riscv64 (`logs/20260811-165027/riscv64/`) — all reach
|
||||
> `ok>`, POST `Failed: 0`, dict-hashes identical across all three
|
||||
> (`0x211a35043331d472` Mama / `0x97502db38aec4d04` Hermes), confirming cross-arch parity
|
||||
> holds under `-O2`.
|
||||
|
||||
- [ ] **4.5f — Retry the 4.4g `console_fb_init()` reorder now that optimization is live.**
|
||||
Depends on 4.5e. This is the original motivating case: both attempts this session (bare,
|
||||
and with the `fb_scroll_rows()` volatile fix alone) stalled boot indefinitely at `-O0`.
|
||||
|
||||
Reference in New Issue
Block a user