mirror of
https://github.com/fluffos/fluffos
synced 2026-08-12 18:26:06 -04:00
1099b48236
Three related changes, plus the use-after-free the third one exposed.
1. read_source_line() walked the file with fgetc() to reach a target line,
and render_diagnostic() calls it once per level of a macro-expansion chain
-- so rendering one diagnostic cost O(levels x filesize) in one-byte stdio
calls. Profiling the LPC testsuite measured 45,430,932 fgetc() calls
reached from render_diagnostic, 12.1% of the entire run, for only 153
rendered diagnostics. It now reads through an 8K stack buffer and finds
line breaks with memchr.
Deliberately no heap buffer and no arena for that scan: it runs DURING a
compile (report_compile_diagnostic, from yyerror/yywarn) and well AFTER
one -- lpcshell renders stored diagnostics once the arena has been reset,
and the compiler GTests call it with no compile in flight at all. A stack
buffer is correct in all three contexts.
Output is unchanged, and checked rather than assumed: on the case that
exercises this hardest (compiler/deep_macro_nesting.lpc, 64 expansion
notes) all 396 lines of rendered diagnostics are byte-identical, and the
file's runtime drops from 1713 ms to 828 ms.
2. The compiler reset the scratchpad at the END of every compile -- freeing
its own output before the caller had read it, which is exactly why anything
a consumer reads afterwards could not live on the arena. Inverted:
compile_file()/compile_file_fd() take a ScratchArena*, allocate every
transient there, and leave it exactly as found. A caller that supplies none
gets a shared default arena, which IS the compiler's to recycle, so that
one is reset on the way IN -- the previous compile's transients stay
readable until the next compile starts. Arena state moves from file-static
globals into ScratchArena::Impl behind a plain RAII handle, and
scratchpad.{h,cc} moves to base/internal/ since ownership now sits outside
the compiler.
The default arena is process-lifetime deliberately. A fresh arena per
compile reads tidier but discards the retained chunk cache every time (that
cache is what drives a long-lived driver to zero chunk mallocs in the
steady state) and leaves scratch_stats()/scratchpad_status() describing an
arena that never took part in a compile. bench_compile caught it: "0
retained chunks, 0 resets" after 2000 compiles where master reports 1 and
2034 -- the "chunk mallocs delta MUST be 0" invariant had gone vacuous
rather than failing.
Because a second arena can no longer borrow the static base block, chunk 0
is sometimes a heap chunk, and base_is_static tells teardown whether to
free it. Every path that drops chunk 0 now keeps that flag honest via
release_base_claim(); without it the tiny-chunk test knob leaked its base
chunk on both calls (476 B and 1 MB, LeakSanitizer-confirmed).
3. lpcshell reaches the compiler through load_object_from_source(), which
now threads an optional ScratchArena* -- a single narrow entry point, so
the 24 general load_object() callers are untouched. lpcshell's Session owns
one, reset at the TOP of each Eval() rather than the end, since by then the
previous evaluation's diagnostics have been printed.
That unblocked the last piece: Diagnostic's variable-length fields are now
ScratchString/ScratchVector, as are the pending_* containers staging into
them. Two boundaries stay heap on purpose -- the lexer's provenance
accessors outlive any compile, and compiler_next_load_reason is set before
compile_file runs -- so both are copied onto the arena at capture.
That conversion carries a lifetime contract, learned twice the hard way: a
stale ScratchString is harmless to destroy, but a stale ScratchVector is
not (its destructor walks its own arena buffer to destroy elements), and
releasing must drop the BUFFER, not just clear() -- a cleared container
keeps capacity, so the next push_back wrote into memory the arena had since
handed to the lexer's Flex buffers, surfacing as a segfault in
yypop_buffer_state nowhere near the diagnostics.
Yucong Sun then found and fixed the remaining hole in that contract: an
arena could die while compiler_diags still referenced it, a
heap-use-after-free that the guard above did not cover. His fix adds the
missing teardown coupling plus regression coverage (test_compiler.cc, an
lpcshell .lpcs case) and the CMake wiring for it.
Validated on RelWithDebInfo and Debug+ASan/UBSan/LSan: 339/339 GTest and
the LPC testsuite clean on each, bench_compile steady-state matching
master, and an lpcshell run rendering a full clang-style error with snippet
and caret AFTER the compile returned -- the case this whole change exists
to make legal.
Claude-Session: https://claude.ai/code/session_01MN9sz4nvGgBZw9oZiei3XR
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|---|---|---|
| .. | ||
| errors.lpcs | ||
| eval.lpcs | ||
| warning_at_exit.lpcs | ||