diff --git a/majit/majit-backend-wasm/src/codegen.rs b/majit/majit-backend-wasm/src/codegen.rs index 13f94c73894..43e0a19d9e2 100644 --- a/majit/majit-backend-wasm/src/codegen.rs +++ b/majit/majit-backend-wasm/src/codegen.rs @@ -5626,7 +5626,7 @@ fn emit_guard_spill( // ── Binary ops ── -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug)] enum BinOp { I64Add, I64Sub, @@ -5795,6 +5795,53 @@ fn emit_umulhi_to_local( /// Overflow binary op: stores the wrapping result in pos and the signed /// overflow flag in the dedicated scratch local. +/// The overflow condition for `a + c` / `a - c` against a constant `c`, as +/// `(limit, greater_than)`: the operation overflows exactly when `a > limit` +/// (`greater_than`) or when `a < limit`. `None` means it cannot overflow. +/// +/// The general form needs both operands and the result to compare sign bits. +/// Against a constant the same predicate is one comparison against a bound +/// folded here, which also drops the dependency on the result. Each bound is +/// taken from the opposite extreme, so none of them can itself overflow: +/// `MAX - c` only for `c > 0`, `MIN - c` only for `c < 0`, and so on. +fn ovf_const_bound(binop: BinOp, c: i64) -> Option<(i64, bool)> { + use std::cmp::Ordering; + match binop { + BinOp::I64Add => match c.cmp(&0) { + Ordering::Greater => Some((i64::MAX - c, true)), + Ordering::Less => Some((i64::MIN - c, false)), + Ordering::Equal => None, + }, + BinOp::I64Sub => match c.cmp(&0) { + Ordering::Greater => Some((i64::MIN + c, false)), + Ordering::Less => Some((i64::MAX + c, true)), + Ordering::Equal => None, + }, + _ => None, + } +} + +/// The variable operand and constant operand of an add/sub whose overflow can +/// take the [`ovf_const_bound`] test. Addition is commutative, so either side +/// may supply the constant; for subtraction only the subtrahend does, since +/// `c - a` has a different bound shape and keeps the general form. +fn ovf_const_operand( + constants: &indexmap::IndexMap, + op: &Op, + binop: BinOp, +) -> Option<(OpRef, i64)> { + let (a, b) = (op.arg(0).to_opref(), op.arg(1).to_opref()); + match binop { + BinOp::I64Add if a.is_constant() && !b.is_constant() => { + Some((b, resolve_const_bits(constants, a))) + } + BinOp::I64Add | BinOp::I64Sub if !a.is_constant() && b.is_constant() => { + Some((a, resolve_const_bits(constants, b))) + } + _ => None, + } +} + fn emit_ovf_binop( sink: &mut InstructionSink<'_>, constants: &indexmap::IndexMap, @@ -5825,6 +5872,28 @@ fn emit_ovf_binop( apply_binop(sink, binop); sink.local_set(result_local); + if let Some((var, c)) = ovf_const_operand(constants, op, binop) { + match ovf_const_bound(binop, c) { + Some((limit, greater_than)) => { + emit_resolve(sink, constants, value_types, var); + sink.i64_const(limit); + if greater_than { + sink.i64_gt_s(); + } else { + sink.i64_lt_s(); + } + sink.i64_extend_i32_u(); + } + // Adding or subtracting zero: the flag stays live so the paired + // guard still finds it, and folds against a constant zero. + None => { + sink.i64_const(0); + } + } + sink.local_set(ovf_flag_local); + return true; + } + match binop { BinOp::I64Add => { // ((a ^ result) & (b ^ result)) >>s 63 @@ -6157,4 +6226,38 @@ mod tests { assert_eq!(frame.call_args_ofs, 416); assert_eq!(frame.frame_bytes, 544); } + + /// The constant-operand bound must answer exactly what the wrapping + /// arithmetic does, including at the extremes where the bound itself is + /// closest to overflowing (`c` = `MIN` makes `MAX + c` and `MIN - c` the + /// interesting cases). + #[test] + fn ovf_const_bound_agrees_with_checked_arithmetic() { + let edges = [ + i64::MIN, + i64::MIN + 1, + -3, + -1, + 0, + 1, + 3, + i64::MAX - 1, + i64::MAX, + ]; + for &c in &edges { + for &a in &edges { + for (binop, expected) in [ + (BinOp::I64Add, a.checked_add(c).is_none()), + (BinOp::I64Sub, a.checked_sub(c).is_none()), + ] { + let got = match ovf_const_bound(binop, c) { + None => false, + Some((limit, true)) => a > limit, + Some((limit, false)) => a < limit, + }; + assert_eq!(got, expected, "{binop:?}: a={a} c={c}"); + } + } + } + } } diff --git a/majit/majit-backend-wasm/tests/codegen_test.rs b/majit/majit-backend-wasm/tests/codegen_test.rs index e2d7d174acb..ed6e2aacfb4 100644 --- a/majit/majit-backend-wasm/tests/codegen_test.rs +++ b/majit/majit-backend-wasm/tests/codegen_test.rs @@ -767,6 +767,109 @@ fn execute_ovf_trace(opcode: OpCode, a: i64, b: i64) -> (i64, i64) { execute_ovf_trace_with_guard(opcode, OpCode::GuardNoOverflow, a, b) } +/// Same trace as [`execute_ovf_trace`], but the constant `c` is an operand +/// rather than a second input argument, so the overflow check takes the +/// folded-bound form instead of the sign-comparison one. `const_first` puts +/// it on the left, which only addition accepts. +fn execute_ovf_trace_const(opcode: OpCode, a: i64, c: i64, const_first: bool) -> (i64, i64) { + let inputargs = vec![InputArg::from_type(Type::Int, 0)]; + let guard = Op::new(OpCode::GuardNoOverflow, &[]); + guard.setfailargs(smallvec![rb(OpRef::input_arg_int(0))]); + let finish = Op::new(OpCode::Finish, &[rb(OpRef::int_op(1))]); + finish.setfailargs(smallvec![rb(OpRef::int_op(1))]); + let args = if const_first { + [OpRef::const_int(c), OpRef::input_arg_int(0)] + } else { + [OpRef::input_arg_int(0), OpRef::const_int(c)] + }; + let ops = vec![make_op(opcode, &args, OpRef::int_op(1)), guard, finish]; + let (bytes, _) = build_module_default(&inputargs, &ops, &indexmap::IndexMap::new()); + + let engine = Engine::default(); + let module = Module::new(&engine, &bytes).expect("generated trace should compile"); + let mut store = Store::new(&engine, ()); + let memory = + Memory::new(&mut store, MemoryType::new(1, None)).expect("test memory should allocate"); + memory + .write( + &mut store, + codegen::FRAME_SLOT_BASE as usize, + &a.to_le_bytes(), + ) + .unwrap(); + let mut linker = Linker::new(&engine); + linker.define("env", "memory", memory).unwrap(); + let instance = linker + .instantiate_and_start(&mut store, &module) + .expect("generated trace should instantiate"); + instance + .get_typed_func::(&store, "trace") + .unwrap() + .call(&mut store, 0) + .expect("generated trace should execute"); + + let mut fail_index = [0; 8]; + let mut result = [0; 8]; + memory.read(&store, 0, &mut fail_index).unwrap(); + memory + .read(&store, codegen::FRAME_SLOT_BASE as usize, &mut result) + .unwrap(); + (i64::from_le_bytes(fail_index), i64::from_le_bytes(result)) +} + +/// A constant operand takes the folded-bound overflow check, so it needs the +/// same verdicts as the general form at the extremes — including the two +/// bounds that sit closest to overflowing themselves, `a - i64::MIN` and +/// `a + i64::MIN`. +#[test] +fn test_ovf_against_a_constant_matches_the_general_form() { + // (opcode, a, c, overflows) + let cases = [ + (OpCode::IntAddOvf, 10, 20, false), + (OpCode::IntAddOvf, 5, 0, false), + (OpCode::IntAddOvf, i64::MAX, 1, true), + (OpCode::IntAddOvf, i64::MAX - 1, 1, false), + (OpCode::IntAddOvf, 10, -20, false), + (OpCode::IntAddOvf, i64::MIN, -1, true), + (OpCode::IntAddOvf, -1, i64::MIN, true), + (OpCode::IntAddOvf, 0, i64::MIN, false), + (OpCode::IntSubOvf, 100, 58, false), + (OpCode::IntSubOvf, 5, 0, false), + (OpCode::IntSubOvf, i64::MIN, 1, true), + (OpCode::IntSubOvf, i64::MIN + 1, 1, false), + (OpCode::IntSubOvf, 10, -5, false), + (OpCode::IntSubOvf, i64::MAX, -1, true), + (OpCode::IntSubOvf, 0, i64::MIN, true), + (OpCode::IntSubOvf, -1, i64::MIN, false), + ]; + for (opcode, a, c, overflows) in cases { + let (fail_index, result) = execute_ovf_trace_const(opcode, a, c, false); + if overflows { + assert_eq!(fail_index, 0, "{opcode:?}: {a} op {c} should guard-exit"); + } else { + let expected = match opcode { + OpCode::IntAddOvf => a.wrapping_add(c), + _ => a.wrapping_sub(c), + }; + assert_eq!( + (fail_index, result), + (1, expected), + "{opcode:?}: {a} op {c}" + ); + } + } + + // Addition is commutative, so the constant is also accepted on the left. + assert_eq!( + execute_ovf_trace_const(OpCode::IntAddOvf, 10, 20, true), + (1, 30) + ); + assert_eq!( + execute_ovf_trace_const(OpCode::IntAddOvf, i64::MAX, 1, true).0, + 0 + ); +} + #[test] fn test_int_add_ovf_guards_overflow() { for (a, b, expected) in [(10, 20, 30), (i64::MIN, 1, i64::MIN + 1)] { diff --git a/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats b/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats index 795616df41a..3de3abe9c96 100644 --- a/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats +++ b/pyre/bench/synth/recursive_call_frame_relocation.wasm.jitstats @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=637 +guard_failures=636 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 diff --git a/pyre/bench/synth/selfrec_bridge_nontail_promote.wasm.jitstats b/pyre/bench/synth/selfrec_bridge_nontail_promote.wasm.jitstats index 39c36fbd0ec..9352e9e4f9b 100644 --- a/pyre/bench/synth/selfrec_bridge_nontail_promote.wasm.jitstats +++ b/pyre/bench/synth/selfrec_bridge_nontail_promote.wasm.jitstats @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=806 +guard_failures=805 internal_compile_panics=0 loops_aborted=0 loops_compiled=2 diff --git a/pyre/check.py b/pyre/check.py index dc43bfb8d28..06dc137ab07 100644 --- a/pyre/check.py +++ b/pyre/check.py @@ -558,31 +558,62 @@ def pyre_env(): # and the `min_heap_size` floor is applied after it (:2452), so the floor # wins on every host. # + # "Every fixture's working set" is the criterion, but the value was chosen + # against the two fixtures that motivated it, and `str_fstring` sits above + # it. That fixture merges six hot loops into one run, and at 256MB it still + # crossed: it reads `guard_failures` 658 or 659 as a function of nothing but + # how much the process allocated before the loops got hot. Startup + # allocation is the whole input — a 49-variable environment reads 659 and a + # 9-variable one 658, with the same binary, the same nursery and the same + # fixture, non-monotonically in the variable count. That is the same + # preamble-vs-body knife edge described above, and it is what the three + # per-platform overlays were holding: each host has its own startup + # allocation volume, so each landed on its own side of the crossing. + # + # 512MB clears it. `str_fstring` then reads 657 at every nursery from 4MB to + # 32MB and under every perturbation of that startup volume — the same + # invariance that says "no crossing at all" for the two fixtures above. + # Eight other fixtures move with it, none of them a knife edge at either + # value: `arith_int_bool` 2214 -> 2212, `build_set_hashability` 4 -> 2, + # `bytes_split_whitespace_maxsplit` 3 -> 2, + # `delete_negative_open_slice_hot` 1405 -> 1404, `instance_dict_reassign` + # 2 -> 1, `newslice_step_hot` 5 -> 3 and `unpack_ex_hot` 3 -> 2 lose a + # crossing, and `inlined_helper_mutation` 3 -> 4 relocates one. No + # `loops_compiled` or `bridges_compiled` moves anywhere in the suite, which + # is the signature that says the collection schedule is the only thing that + # changed. + # + # The cost is memory, and it is paid by the one fixture that was over the + # old threshold: `str_fstring` peaks at 618MB against 519MB. Every other + # fixture is flat to within a megabyte (`closure_per_call` 179 -> 180MB, + # `recursive_call_frame_relocation` 159 -> 160MB), because a threshold only + # costs what a fixture actually retains. + # # Both pins reach the wasm backend only because `pyre-wasm-runner` hands # them to the guest explicitly (`pyre_set_gc_env`): a wasm32-unknown-unknown # module's `std::env` is permanently empty, so setting them here does # nothing on its own there. # # Reaching the guest is not the same as clearing the crossing there. 256MB - # removes it on both native backends and does not in the guest, which reads - # `recursive_call_frame_relocation` 638 and `closure_per_call` 420 against - # their 636 and 414 — the same knife edge described above, one step along - # it. Historically the two ran different interpreter allocation models: - # `PYRE_GC_INTERP` was off natively and on in the guest, so only the guest - # counted interpreter-created int/float boxes toward the old-gen threshold. - # The gate is now default-on everywhere and the allocation model is shared; - # this note remains because the recorded wasm memory/counter boundary below - # predates that convergence and still explains why the 256MB floor was - # chosen instead of tuning a platform-specific overlay. + # removed it on both native backends and did not in the guest, whose + # baselines held `recursive_call_frame_relocation` 638 and + # `closure_per_call` 415 against their 636 and 414 — the same knife edge + # described above, one step along it. Historically the two ran different + # interpreter allocation models: `PYRE_GC_INTERP` was off natively and on in + # the guest, so only the guest counted interpreter-created int/float boxes + # toward the old-gen threshold. The gate is now default-on everywhere and + # the allocation model is shared, but the recorded counter boundary outlived + # that convergence, because the threshold was still inside the guest's + # working set. # - # Left where it is deliberately. 384MB and above does land the guest exactly - # on the native counts, but it buys agreement with memory: `closure_per_call` - # peaks at 459MB there against 358MB here. The three baselines are recorded - # per backend and each is deterministic — five repeats of both fixtures - # under the guest report one number — so the difference costs nothing but - # the sensitivity this note is here to name: a change in interpreter-path - # allocation volume moves the wasm numbers while the native ones sit still. - env.setdefault("PYPY_GC_MIN", str(256 * 1024 * 1024)) + # The value below is past it, so the guest now lands exactly on the native + # counts: both fixtures re-recorded to 636 and 414, identical across all + # three backends. It buys that agreement with memory — `closure_per_call` + # peaked at 459MB under the guest at 384MB against 358MB at 256MB. The + # sensitivity this note is here to name outlives the convergence: a change + # in interpreter-path allocation volume moves the wasm numbers while the + # native ones sit still. + env.setdefault("PYPY_GC_MIN", str(512 * 1024 * 1024)) # Keep the bench directory off `sys.path` (`-P`), so the jit-stats counters # describe the fixture rather than the directory it happens to sit in. # @@ -1691,40 +1722,36 @@ def _jitstats_baseline_path(self, backend, script): # and all three backends, and the band this gate replaced is exactly # what let eight baselines go stale unnoticed. # - # No fixture uses this today. The mechanism is kept because a genuine - # host disagreement is worth recording exactly rather than widening the - # gate for everyone, but the four overlays that existed -- all of them - # `str_fstring`'s -- were removed once the disagreement they recorded - # was traced to its cause, and that trace is the reason to be slow about - # writing another. + # No fixture has one at present, and the last set to need them is worth + # recording because of how it ended. `str_fstring` was carried by three: + # the runners read guard_failures 658/659 on ubuntu dynasm/cranelift, + # the inverse 659/658 on macos, and 658/658 on windows, with every other + # gated counter identical (six loops, three bridges) and each mismatch + # reproducing in both stability reruns. The split was stable per host, so + # it read as a host disagreement. # - # `str_fstring` read `guard_failures` 657 or 658 with every other gated - # counter identical (six loops, three bridges), and which one it read - # differed by backend and by runner. The cause was not the backend. At - # the trip counts it used, its old-gen use crossed the major-collection - # threshold pinned above; the crossing arms the eval-breaker, a back-edge - # poll guard fails, and re-entry after that bailout lands in a bridge - # whose own guard then fails once -- one extra `guard_failures` that is a - # real deopt, not a poll, so the `back_edge_polls` split cannot absorb - # it. Whether re-entry trips that bridge guard depends on where the - # crossing falls, and walking `PYPY_GC_MIN` in 8MB steps puts *both* - # backends on the extra failure at thresholds one step apart (cranelift - # at 256MB, dynasm at 264MB, neither at 272MB). The runners were not - # disagreeing about a fact; each sat at a different point of one curve. + # It was not one. The knife edge is a major-collection crossing landing + # either side of the peeled preamble (see `pyre_env`), and its input is + # how much the process allocated before the loops got hot — a quantity + # every host computes differently and nothing about the platform fixes. + # Adding check.py's two wasm environment keys, semantically ignored by + # the native backend, moves a local macOS cranelift build 658 -> 659 + # while the number of major collections stays at 11; the guard sequence + # gains one trace-6 fail-5 exit that the compiled-trace log maps to the + # GuardFalse on the eval-breaker poll. Raising `PYPY_GC_MIN` past the + # fixture's working set removed the crossing, and all three overlays + # came out with it. # - # Anything that moves total allocation volume moves that point -- - # including the byte length of the environment, since the child inherits - # the parent's. So an overlay written against such a value records a - # phase, not a property, and re-rolls under an unrelated change. The fix - # was to take the fixture off the threshold rather than to record which - # side of it each runner landed on; see its header. + # That is the shape to look for before writing one: a counter that + # splits per host and moves under a perturbation with no semantic + # content is not a host disagreement, it is a boundary the harness has + # not pinned yet. An overlay would have frozen it per platform instead. # - # An overlay also shadows the shared baseline permanently, so one written - # against a value that later converges becomes a failure main does not - # have: `recursive_call_frame_relocation` was given one for 637, and - # windows later converged on its shared value. Read all three runners - # back before adding one, and prefer removing the fixture's dependence on - # the host input to recording the host. + # An overlay also shadows the shared baseline permanently, so one + # written against a value that later converges becomes a failure main + # does not have: `recursive_call_frame_relocation` was given one for + # 637, and windows later converged on its shared value. Read all three + # runners back before adding or retaining another. source = Path(script) if os.environ.get("GITHUB_ACTIONS") == "true": github_runner = source.with_name( diff --git a/pyre/extra_tests/parity_tests/fileio_stat_atopen_staleness.py b/pyre/extra_tests/parity_tests/fileio_stat_atopen_staleness.py index 5281cda069f..61944919dba 100644 --- a/pyre/extra_tests/parity_tests/fileio_stat_atopen_staleness.py +++ b/pyre/extra_tests/parity_tests/fileio_stat_atopen_staleness.py @@ -10,6 +10,7 @@ """ import os +import sys import tempfile directory = tempfile.mkdtemp() @@ -49,12 +50,16 @@ assert stream.read(8) == b"\x00" * 8 # Seekability is a property of the descriptor, so the answer may be kept, but -# it belongs to that stream alone. -read_fd, write_fd = os.pipe() -with open(read_fd, "rb") as pipe: - assert pipe.seekable() is False - assert pipe.seekable() is False -os.close(write_fd) +# it belongs to that stream alone. Only the unseekable half is platform-bound: +# a Windows anonymous-pipe descriptor answers `seekable()` True, under the +# reference CPython as much as here, so asking it there compares a backend +# against a failing reference. The seekable half below still runs everywhere. +if sys.platform != "win32": + read_fd, write_fd = os.pipe() + with open(read_fd, "rb") as pipe: + assert pipe.seekable() is False + assert pipe.seekable() is False + os.close(write_fd) with open(path, "rb") as stream: assert stream.seekable() is True assert stream.seekable() is True diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index b79736f7711..c5e154b3de8 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -15740,7 +15740,24 @@ fn fileio_method_truncate(args: &[PyObjectRef]) -> Result i32; + } + let rc = crt_call!(_chsize_s(fd, size as i64)); + if rc != 0 { + return Err(fd_errno_err(rc)); + } + fileio_clear_stat_atopen(self_obj); + return Ok(index); + } + #[cfg(any(all(not(unix), not(windows)), feature = "sandbox"))] { let _ = (fd, size); return Err(crate::PyError::not_implemented(