diff --git a/changelog.d/6840-typed-feedback-entry-return-ordering.md b/changelog.d/6840-typed-feedback-entry-return-ordering.md new file mode 100644 index 0000000000..c137b3c33c --- /dev/null +++ b/changelog.d/6840-typed-feedback-entry-return-ordering.md @@ -0,0 +1,20 @@ +### Fixed + +- **`typed_feedback_trace_dump_runs_before_entry_return` has been red on `main`.** + Codegen was never wrong: `add_pre_return_void_call` splices + `js_typed_feedback_maybe_dump_trace()` in front of every `ret` in `main`, and + it still does. The test was measuring the wrong thing. It compared + `ir.rfind("call void @js_typed_feedback_maybe_dump_trace()")` against + `ir.rfind("ret i32 0")` over the whole module text. `main` has more than one + return: the host-return early exit returns a literal `i32 0`, the event-loop + exit returns the pending exit code. So the two `rfind`s landed on unrelated + sites — the last dump sat after the first return, and the assertion failed + while the property it names held. + + The test now slices the IR to `main`'s body and checks every return site: each + `ret` must be immediately preceded by the dump call. Removing the + `add_pre_return_void_call` in `crates/perry-codegen/src/codegen/entry.rs` + makes it fail, which the old shape could not guarantee. + + The PR workflow now uses diff-based selection in `e2e-scoped`, so changing + this integration test pulls its suite into the per-PR run. diff --git a/crates/perry-codegen/src/loop_purity.rs b/crates/perry-codegen/src/loop_purity.rs index aeace93d48..364d34cb3b 100644 --- a/crates/perry-codegen/src/loop_purity.rs +++ b/crates/perry-codegen/src/loop_purity.rs @@ -121,9 +121,7 @@ fn expr_alloc_free(e: &Expr) -> bool { // Element READS never allocate — they return an existing element / a // number. Recurse so the object and index are themselves alloc-free. Expr::IndexGet { object, index } => expr_alloc_free(object) && expr_alloc_free(index), - Expr::BufferIndexGet { buffer, index } => { - expr_alloc_free(buffer) && expr_alloc_free(index) - } + Expr::BufferIndexGet { buffer, index } => expr_alloc_free(buffer) && expr_alloc_free(index), Expr::Uint8ArrayGet { array, index } => expr_alloc_free(array) && expr_alloc_free(index), // `arr[i]++` / `--`: read-modify-write of an existing numeric slot, no // growth, no allocation. diff --git a/crates/perry-codegen/tests/typed_feedback.rs b/crates/perry-codegen/tests/typed_feedback.rs index 3f648f30fe..4834296494 100644 --- a/crates/perry-codegen/tests/typed_feedback.rs +++ b/crates/perry-codegen/tests/typed_feedback.rs @@ -218,27 +218,44 @@ fn typed_feedback_trace_dump_runs_before_entry_return() { )); assert!(ir.contains("declare void @js_typed_feedback_maybe_dump_trace()")); - // The entry epilogue now has TWO exit paths: the host-return path - // (`ret i32 0`) and the event-loop exit path, which returns the dynamic - // `js_process_pending_exit_code` result (`ret i32 %rN`). The old - // rfind(dump) < rfind("ret i32 0") comparison broke the day the second - // path appeared — its dump call sits after the literal `ret i32 0`. The - // real invariant is stronger: EVERY entry return must be immediately - // preceded by the trace dump, so no exit path can skip the dump. - let mut ret_count = 0; - let mut search_from = 0; - while let Some(rel) = ir[search_from..].find("ret i32") { - let ret_pos = search_from + rel; - ret_count += 1; - let preceding = &ir[ret_pos.saturating_sub(200)..ret_pos]; - assert!( - preceding.contains("call void @js_typed_feedback_maybe_dump_trace()"), - "entry return at byte {ret_pos} is not preceded by the typed-feedback \ - trace dump:\n...{preceding}" - ); - search_from = ret_pos + 1; + + // Scope the ordering check to `main`'s body, and check every return site. + // `main` has more than one `ret` (the host-return early exit returns + // `i32 0`, the event-loop exit returns the pending exit code), and later + // functions in the module carry their own returns. A positional + // `rfind(dump) < rfind("ret i32 0")` over the whole module text therefore + // compares two unrelated sites and proves nothing about the ordering. + let body = entry_fn_body(&ir); + let mut returns = 0; + let mut prev = ""; + for line in body.lines() { + let trimmed = line.trim(); + if trimmed.starts_with("ret ") { + returns += 1; + assert_eq!( + prev, "call void @js_typed_feedback_maybe_dump_trace()", + "every `main` return must dump the typed-feedback trace first, \ + else a PERRY_TYPED_FEEDBACK trace is truncated at exit; \ + found `{trimmed}` preceded by `{prev}`" + ); + } + prev = trimmed; } - assert!(ret_count >= 1, "entry should return i32"); + assert!(returns > 0, "expected at least one return in `main`"); +} + +/// Body of the entry `main` function, without its `define`/`}` lines. +fn entry_fn_body(ir: &str) -> &str { + let header = "define i32 @main() {\n"; + let start = ir + .find(header) + .expect("entry module should define `i32 @main()`") + + header.len(); + let rest = &ir[start..]; + let end = rest + .find("\n}\n") + .expect("`main` should be terminated by a closing brace"); + &rest[..end] } #[test]