fix(codegen): guard print_r's container walk against the null-container sentinel (#647) - #649
Conversation
…er sentinel emit_print_r_array had no null branch at all -- not even the zero-pointer check var_dump carried -- so a missed element read wrote "Array\n" and handed the null-container sentinel to the walker as a live container, segfaulting mid-output. The guard uses the shared sentinels helper, recognizing both the zero pointer and the sentinel, and jumps past BOTH the "Array\n" write and the walker call. Skipping every write is what makes all three call modes correct at once: echo mode prints nothing and still returns true, and the capture modes leave the buffer empty so __rt_pr_finish yields "". Note this is deliberately not var_dump's behaviour: PHP prints nothing for print_r(null), where var_dump(null) prints NULL, so the branch target differs from the one added for issue illegalstudio#581.
Greptile SummaryThis PR fixes issue #647 by adding the null-container guard that was missing entirely from
Confidence Score: 5/5Safe to merge; the change is a targeted one-function guard addition with no impact on existing code paths and thorough test coverage. The guard is inserted in exactly the right place (before the header write and walker call), uses the shared sentinel helper that every other null-container site in the codebase uses, and the skip label is placed after the walker call so both writes are skipped together. All three call modes (echo, capture, runtime-flag) are covered by behavioural tests, and an assembly-ordering test confirms the guard precedes the walker on both supported architectures. The negative test verifies that genuine null locals and live arrays continue to render correctly. No existing behaviour is disturbed. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/codegen/lower_inst/builtins/debug.rs | Adds null-container guard to emit_print_r_array using the shared emit_branch_if_null_container helper; skip label is placed correctly after the walker call, matching PHP semantics for both walkers (indexed and hash). Docblock updated to explain the fix. |
| tests/codegen/io/printing.rs | Adds 8 regression tests: 6 behavioural crash tests, 1 negative guard for pre-existing shapes, and 1 assembly-ordering test that verifies the sentinel comparison precedes the walker call on both AArch64 and x86_64. |
| CHANGELOG.md | Adds one [Unreleased] bullet describing the fix, consistent with the project changelog policy. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["emit_print_r_array(ctx, walker)"] --> B["result_reg = int_result_reg\nskip_label = next_label\nscratch_reg = secondary_scratch_reg"]
B --> C["emit_branch_if_null_container\n(zero ptr OR NULL_SENTINEL)"]
C -->|null / sentinel| D["skip_label:\n(no output — PHP empty)"]
C -->|live container| E["emit_push_reg(result_reg)\nemit_write_literal 'Array\\n'\nemit_pop_reg(result_reg)"]
E --> F{"arch?"}
F -->|AArch64| G["mov x1, #0\n(base indent = 0)"]
F -->|x86_64| H["mov rdi, rax\nmov esi, 0"]
G --> I["emit_call_label(walker)\n__rt_print_r_indexed or\n__rt_print_r_hash"]
H --> I
I --> D
Reviews (1): Last reviewed commit: "fix(codegen): guard print_r's container ..." | Re-trigger Greptile
|
CI note — the three All three targets fail on the same unrelated test: It is an None of those is in For what it is worth locally, on this branch: |
Fixes #647. Based on current
main9f74f5300.Sibling of #646 (same file, same sentinel family) but a different function and different PHP semantics — see below. The two are independent; see the conflict note at the end.
Root cause
emit_print_r_array(src/codegen/lower_inst/builtins/debug.rs) had no null branch at all — not even the zero-pointer check thatemit_var_dump_arraycarried. So a missed element read wroteArray\nand passed the null-container sentinel straight to the walker as a live container.Pre-fix assembly for the repro:
No
cbz, no comparison — so unlike #581 the crash happens inside the walker rather than at an inline header load.Fix, and why it is not #646's fix
The guard uses the shared
sentinels::emit_branch_if_null_container(zero or sentinel) and jumps to a label placed after the walker call, skipping both theArray\nwrite and the walk.It deliberately does not reuse the branch target added in #646, because PHP's semantics differ and I checked them before implementing:
print_r(null)prints nothing, wherevar_dump(null)printsNULL. Skipping every write is also what makes all three call modes correct with one branch: echo mode prints nothing and still returnstrue(the immediate is loaded later, inlower_print_r), and the capture modes leave the buffer empty so__rt_pr_finishyields"".Tests
Eight regressions in
tests/codegen/io/printing.rs— the natural home, next to the 20+ existingtest_print_r_*cases. Six are behavioural and fail on the pre-fix build withprogram crashed; one guards the shapes that already worked; one asserts the emitted guard directly.The assembly test was checked with an explicit negative control: reverting the source makes it fail on the host and under
ELEPHC_TEST_TARGET=linux-x86_64andlinux-aarch64, withmissing print_r null-container skip branch. So x86_64 is genuinely verified, not assumed.Sibling shapes, all matching PHP after the fix: hash receiver with an array value, hash receiver with a hash value (the
__rt_print_r_hashwalker — it crashed too), a miss through?? null, the return mode (print_r($x, true)), and the runtime-flag mode (print_r($x, $flag)). Return mode was the least obvious: it crashed without printingArray, because the text went into the capture buffer first, so its test assertslen=0rather than merely the absence of a crash.A miss inside a boxed
mixedwas already correct before this change — the boxing-boundary normalization from #585/#591 covers it — and is left alone rather than claimed.Verification (macOS ARM64,
78a6a6590)print_r(incl. the 8 new)ELEPHC_IR_OPT=offELEPHC_TEST_TARGET=linux-x86_64ELEPHC_TEST_TARGET=linux-aarch64output_buffering(print_r writes through ob)var_dumpsentinelarray_miss(#526)null_container(#556/#592)autovivify(#600/#592)--heap-debugleak summary: clean, exit 0cargo build/git diff --checkNarrow filters only, per the project's test policy; CI owns the full matrix. Linux not executed locally (no Docker), but the x86_64 lowering is covered by the assembly test above.
Conflict note with #646
Checked with
git merge-treerather than assumed:debug.rsmerges cleanly — fix(codegen): guard var_dump's container walk against the null-container sentinel (#581) #646 touchesemit_var_dump_array(~line 535), this touchesemit_print_r_array(~line 296), roughly 230 lines apart.tests/codegen/io/printing.rs, fix(codegen): guard var_dump's container walk against the null-container sentinel (#581) #646's intests/codegen/regressions/arrays.rs.CHANGELOG.mdconflicts, since both add a bullet under## [Unreleased]. Routine rebase resolution: keep both lines. Merge order does not matter.