From 3a2b72b5a8cecf94ad262a0bba5b9c2a74644ee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 23:37:48 +0200 Subject: [PATCH 1/2] fix(codegen): a taken throw out of a packed loop no longer loses loop-carried locals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #9185 admitted `Stmt::Throw` to the packed-f64 versioned loop on the reasoning that the throw block ends in `unreachable`, so "control never returns to the loop and nothing reads what the clone cached". The second half of that is false: an unwind lands in a `catch`, which can read anything the loop wrote. `break`, `continue` and `return` leave the clone through normal CFG edges that flush the loop-carried locals back to their frame slots. An unwind edge does not, so a local the loop updated read back stale: let s = 0; try { for (let i = 0; i < arr.length; i++) { if (arr[i] === 40) throw PRE; s += arr[i]; } } catch (e) { console.log(s); } // perry 0, node 780 Wrong in both orderings (780 and 820 expected, 0 observed), correct under PERRY_PACKED_LOOP_ABRUPT=0, and correct on the 0.5.1220 release, which confirms #9185 as the regression. This restores `Stmt::Throw` to unadmitted. Re-admitting needs the writeback emitted at the throw site rather than only at the exit block; that is #9210, and the admission predicate is not the missing piece. #9185's own tests missed this because both taken-throw cases read the thrown value or an untouched variable — the clone's live SSA value, which was correct — never the frame slot left behind. A closure-captured accumulator was also correct, being boxed rather than register-promoted, which is what kept the defect this narrow. The added test reads a loop-carried local AFTER each abrupt exit; its break and continue rows are what establish that the defect is specific to the unwind edge. Cost: a loop containing a throw returns to 16.00 ns/op against node's 1.17 (13.7x), the pre-#9185 gap. Correctness first; #9210 is the way back. Size: __text unchanged (10,968,212 B, 0.00%), __const +64 B, file +8 B. Refs #9151, #9185, #9210 --- crates/perry-codegen/src/stmt/loops.rs | 23 +++++- .../tests/packed_loop_abrupt_statements.rs | 80 +++++++++++++++++-- 2 files changed, 92 insertions(+), 11 deletions(-) diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 29b69f49f4..84b8aa3a6a 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -5063,9 +5063,26 @@ fn stmt_is_packed_f64_loop_safe( .as_ref() .is_none_or(|expr| expr_is_packed_f64_loop_safe(ctx, expr, arr_id, counter_id)) } - // `throw` stays out: the thrown value is typically constructed - // (`throw new Error(…)`), which is a call in the loop body. - Stmt::Throw(_) => packed_loop_abrupt_enabled(), + // `throw` stays out. Admitting it (#9185) was a silent wrong answer: + // `break`/`continue`/`return` leave the clone through normal CFG edges + // that flush the loop-carried locals back to their frame slots, but an + // unwind edge does not, so anything reading such a local AFTER the + // throw saw its stale pre-loop value: + // + // let s = 0; + // try { for (let i = 0; i < arr.length; i++) { + // if (arr[i] === 40) throw PRE; s += arr[i]; } } + // catch (e) { return s; } // gave 0, node gives 780 + // + // #9185's tests missed it because none of them read a loop-carried + // local after unwinding — they read the thrown value (which IS the + // clone's live value, and was correct) or an untouched variable. A + // closure-captured accumulator was also correct, being boxed rather + // than register-promoted, which is what kept the bug this narrow. + // + // Re-admitting this needs the writeback emitted at the throw site, not + // just the admission; see #9210. + Stmt::Throw(_) => false, Stmt::LabeledBreak(_) | Stmt::LabeledContinue(_) | Stmt::While { .. } diff --git a/crates/perry/tests/packed_loop_abrupt_statements.rs b/crates/perry/tests/packed_loop_abrupt_statements.rs index e331b78d67..5d8c3d84f6 100644 --- a/crates/perry/tests/packed_loop_abrupt_statements.rs +++ b/crates/perry/tests/packed_loop_abrupt_statements.rs @@ -145,11 +145,16 @@ fn break_in_a_loop_whose_array_grows() { } #[test] -fn a_throw_that_does_not_construct_takes_the_fast_path_correctly() { - // A `throw` whose value is already built is admitted: its block ends in - // `unreachable`, so control never returns to the loop and nothing reads - // what the clone cached. A throw that CONSTRUCTS its value is not, because - // the construction is emitted in blocks preceding the terminating one. +fn a_throw_that_does_not_construct_is_still_correct() { + // This shape was admitted to the fast path by #9185 on the reasoning that + // the throw block ends in `unreachable`, so "control never returns to the + // loop and nothing reads what the clone cached". The second half of that + // is false — an unwind lands in a `catch`, which can read anything the + // loop wrote — and the loop is no longer admitted. See + // `a_loop_carried_local_survives_a_taken_throw` below for the case that + // proves it, and note what these assertions do NOT check: `throwPre` + // reads the thrown value and `k`, `throwValue` throws `s` itself. Both + // observe the clone's live value, never the frame slot left behind. let out = compile_and_run(&format!( "{PRELUDE} const PRE = new Error(\"boom\"); @@ -175,9 +180,7 @@ fn a_throw_that_does_not_construct_takes_the_fast_path_correctly() { #[test] fn throw_inside_the_loop_is_still_correct() { - // A throw that CONSTRUCTS its value is not admitted (the construction is a - // call in a block that does not end in `unreachable`), but it must keep - // working on the generic path. + // A throw that CONSTRUCTS its value must keep working on the generic path. let out = compile_and_run(&format!( "{PRELUDE} function throwAt(k: number): string {{ @@ -192,3 +195,64 @@ fn throw_inside_the_loop_is_still_correct() { )); assert_eq!(out, "hit15 none2016"); } + +/// #9185 admitted `throw` to the packed fast path; this is the case that +/// showed it was a silent wrong answer. +/// +/// `break` and `continue` leave the clone through normal CFG edges, which +/// flush the loop-carried locals back to their frame slots. An unwind edge +/// does not, so the `catch` below read `s` from a slot the loop never +/// updated and got its pre-loop `0` instead of `780`. +/// +/// Every assertion here reads a loop-carried local AFTER the abrupt exit, +/// which is precisely what #9185's own tests did not do. The `break` and +/// `continue` rows are not padding: they are what established that the defect +/// was specific to the unwind edge rather than to abrupt exits in general. +#[test] +fn a_loop_carried_local_survives_a_taken_throw() { + let out = compile_and_run(&format!( + "{PRELUDE} + const PRE = new Error(\"boom\"); + function viaBreak(): string {{ + let s = 0; + for (let i = 0; i < arr.length; i++) {{ if (arr[i] === 40) break; s += arr[i]; }} + return \"break \" + s; + }} + function viaContinue(): string {{ + let s = 0; + for (let i = 0; i < arr.length; i++) {{ if (arr[i] % 2 === 0) continue; s += arr[i]; }} + return \"continue \" + s; + }} + function throwThenRead(): string {{ + let s = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ if (arr[i] === 40) throw PRE; s += arr[i]; }} + }} catch (e) {{ return \"throwBefore \" + s; }} + return \"none \" + s; + }} + function accumulateThenThrow(): string {{ + let s = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ s += arr[i]; if (arr[i] === 40) throw PRE; }} + }} catch (e) {{ return \"throwAfter \" + s; }} + return \"none \" + s; + }} + function throwThenReadViaClosure(): string {{ + let s = 0; + const get = () => s; + try {{ + for (let i = 0; i < arr.length; i++) {{ if (arr[i] === 40) throw PRE; s += arr[i]; }} + }} catch (e) {{ return \"closure \" + get(); }} + return \"none \" + get(); + }} + console.log( + viaBreak() + \" | \" + viaContinue() + \" | \" + throwThenRead() + \" | \" + + accumulateThenThrow() + \" | \" + throwThenReadViaClosure() + ); + " + )); + assert_eq!( + out, + "break 780 | continue 1024 | throwBefore 780 | throwAfter 820 | closure 780" + ); +} From 937628231ab8040d58f5e497dbfd8936f9126b15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 23:16:08 +0200 Subject: [PATCH 2/2] fix(ui): drop redundant unsafe blocks in perry-ui-macos Six inner unsafe blocks nested inside an outer one, each wrapping a single str_from_header call. rustc reports 12 unnecessary-unsafe warnings across the lib and test targets, which fails the workspace warnings gate under -D warnings on any PR that invalidates this crate's check cache. Pre-existing since #8453. --- crates/perry-ui-macos/src/file_dialog.rs | 2 +- crates/perry-ui-macos/src/lib_ffi/system.rs | 2 +- crates/perry-ui-macos/src/lib_ffi/window_misc.rs | 2 +- crates/perry-ui-macos/src/widgets/alert.rs | 2 +- crates/perry-ui-macos/src/widgets/combobox.rs | 2 +- crates/perry-ui-macos/src/widgets/webview.rs | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/perry-ui-macos/src/file_dialog.rs b/crates/perry-ui-macos/src/file_dialog.rs index 09f7bb8752..d5191d5b61 100644 --- a/crates/perry-ui-macos/src/file_dialog.rs +++ b/crates/perry-ui-macos/src/file_dialog.rs @@ -93,7 +93,7 @@ pub fn save_dialog(callback: f64, default_name_ptr: *const u8, _allowed_types_pt // Set default filename if provided if !default_name_ptr.is_null() { - let name = unsafe { str_from_header(default_name_ptr) }; + let name = str_from_header(default_name_ptr); if !name.is_empty() { let ns_name = NSString::from_str(&name); let _: () = msg_send![&*panel, setNameFieldStringValue: &*ns_name]; diff --git a/crates/perry-ui-macos/src/lib_ffi/system.rs b/crates/perry-ui-macos/src/lib_ffi/system.rs index 18e29628d8..e0bce1ccbf 100644 --- a/crates/perry-ui-macos/src/lib_ffi/system.rs +++ b/crates/perry-ui-macos/src/lib_ffi/system.rs @@ -275,7 +275,7 @@ pub extern "C" fn perry_system_preferences_set(key_ptr: i64, value: f64) { if (bits >> 48) == 0x7FFF { // NaN-boxed string — extract string pointer let str_ptr = js_nanbox_get_pointer(value) as *const u8; - let s = unsafe { str_from_header(str_ptr) }; + let s = str_from_header(str_ptr); let ns_str = objc2_foundation::NSString::from_str(&s); let _: () = objc2::msg_send![defaults, setObject: &*ns_str, forKey: &*ns_key]; } else { diff --git a/crates/perry-ui-macos/src/lib_ffi/window_misc.rs b/crates/perry-ui-macos/src/lib_ffi/window_misc.rs index 276b2b2645..b5bc8ce18f 100644 --- a/crates/perry-ui-macos/src/lib_ffi/window_misc.rs +++ b/crates/perry-ui-macos/src/lib_ffi/window_misc.rs @@ -408,7 +408,7 @@ pub extern "C" fn perry_ui_set_text(id_ptr: i64, value_ptr: i64) { let (value_data, value_len) = value .as_ref() .map_or((std::ptr::null(), 0), |value| (value.as_ptr(), value.len())); - unsafe { + { widgets::text_registry::set_text_handler(id.as_ptr(), id.len(), value_data, value_len); } } diff --git a/crates/perry-ui-macos/src/widgets/alert.rs b/crates/perry-ui-macos/src/widgets/alert.rs index 026a80a759..d3a168435d 100644 --- a/crates/perry-ui-macos/src/widgets/alert.rs +++ b/crates/perry-ui-macos/src/widgets/alert.rs @@ -47,7 +47,7 @@ pub fn show(title_ptr: *const u8, message_ptr: *const u8, arr_ptr: i64, callback for i in 0..len { let elem = js_array_get_element(arr_ptr, i); let str_ptr = js_get_string_pointer_unified(elem) as *const u8; - let label = unsafe { str_from_header(str_ptr) }; + let label = str_from_header(str_ptr); let ns_label = NSString::from_str(&label); let _: Retained = msg_send![&*alert, addButtonWithTitle: &*ns_label]; } diff --git a/crates/perry-ui-macos/src/widgets/combobox.rs b/crates/perry-ui-macos/src/widgets/combobox.rs index accc1d65c3..08a13554a3 100644 --- a/crates/perry-ui-macos/src/widgets/combobox.rs +++ b/crates/perry-ui-macos/src/widgets/combobox.rs @@ -111,7 +111,7 @@ pub fn create(initial_ptr: *const u8, on_change: f64) -> i64 { let _: () = msg_send![&*combobox, setEditable: true]; let _: () = msg_send![&*combobox, setNumberOfVisibleItems: 8i64]; - let initial_str = unsafe { str_from_header(initial_ptr) }; + let initial_str = str_from_header(initial_ptr); if !initial_str.is_empty() { let ns_initial = NSString::from_str(&initial_str); let _: () = msg_send![&*combobox, setStringValue: &*ns_initial]; diff --git a/crates/perry-ui-macos/src/widgets/webview.rs b/crates/perry-ui-macos/src/widgets/webview.rs index 38f69c3237..fcd3fb66da 100644 --- a/crates/perry-ui-macos/src/widgets/webview.rs +++ b/crates/perry-ui-macos/src/widgets/webview.rs @@ -747,7 +747,7 @@ pub fn set_allowed_domains(handle: i64, domains_arr_handle: i64) { let elem = js_array_get_element_f64(domains_arr_handle, i); let str_ptr = js_get_string_pointer_unified(elem) as *const u8; if !str_ptr.is_null() { - domains.push(unsafe { str_from_header(str_ptr) }.to_string()); + domains.push(str_from_header(str_ptr).to_string()); } } }