From 9792920f8cdec92aa2c650de8cfd0a85fa7dce52 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Mon, 11 Dec 2023 21:17:22 -0800 Subject: fix(tvix/eval): fix branching on catchable defaults (b/343) This commit adds Opcode::OpJumpIfCatchable, which can be inserted ahead of most VM operations which expect a boolean on the stack, in order to handle catchables in branching position properly. Other than remembering to patch the jump, no other changes should be required. This commit also fixes b/343 by emitting this new opcode when compiling if-then-else. There are probably other places where we need to do the same thing. Change-Id: I48de3010014c0bbeba15d34fc0d4800e0bb5a1ef Reviewed-on: https://cl.tvl.fyi/c/depot/+/10288 Tested-by: BuildkiteCI Reviewed-by: tazjin Autosubmit: Adam Joseph --- tvix/eval/src/compiler/mod.rs | 6 ++++++ tvix/eval/src/opcode.rs | 6 ++++++ .../tests/tvix_tests/eval-okay-test-catchables-in-default-args.exp | 1 + .../tests/tvix_tests/eval-okay-test-catchables-in-default-args.nix | 1 + .../notyetpassing/eval-okay-test-catchables-in-default-args.exp | 1 - .../notyetpassing/eval-okay-test-catchables-in-default-args.nix | 1 - tvix/eval/src/value/mod.rs | 1 + tvix/eval/src/vm/mod.rs | 7 +++++++ 8 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.exp create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.nix delete mode 100644 tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.exp delete mode 100644 tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.nix (limited to 'tvix/eval/src') diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 01e07304bc3f..4edb5f204533 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -864,6 +864,10 @@ impl Compiler<'_> { self.compile(slot, node.condition().unwrap()); self.emit_force(&node.condition().unwrap()); + let throw_idx = self.push_op( + OpCode::OpJumpIfCatchable(JumpOffset(0)), + &node.condition().unwrap(), + ); let then_idx = self.push_op( OpCode::OpJumpIfFalse(JumpOffset(0)), &node.condition().unwrap(), @@ -879,6 +883,7 @@ impl Compiler<'_> { self.compile(slot, node.else_body().unwrap()); self.patch_jump(else_idx); // patch jump *over* else body + self.patch_jump(throw_idx); // patch jump *over* else body } /// Compile `with` expressions by emitting instructions that @@ -1328,6 +1333,7 @@ impl Compiler<'_> { OpCode::OpJump(n) | OpCode::OpJumpIfFalse(n) | OpCode::OpJumpIfTrue(n) + | OpCode::OpJumpIfCatchable(n) | OpCode::OpJumpIfNotFound(n) | OpCode::OpJumpIfNoFinaliseRequest(n) => { *n = offset; diff --git a/tvix/eval/src/opcode.rs b/tvix/eval/src/opcode.rs index 762cff7b5282..467798177550 100644 --- a/tvix/eval/src/opcode.rs +++ b/tvix/eval/src/opcode.rs @@ -130,6 +130,12 @@ pub enum OpCode { /// of the stack is `false`. OpJumpIfFalse(JumpOffset), + /// Pop one stack item and jump forward in the bytecode + /// specified by the number of instructions in its usize + /// operand, *if* the value at the top of the stack is a + /// Value::Catchable. + OpJumpIfCatchable(JumpOffset), + /// Jump forward in the bytecode specified by the number of /// instructions in its usize operand, *if* the value at the top /// of the stack is the internal value representing a missing diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.exp new file mode 100644 index 000000000000..c508d5366f70 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.exp @@ -0,0 +1 @@ +false diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.nix new file mode 100644 index 000000000000..0523cf864c3b --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-test-catchables-in-default-args.nix @@ -0,0 +1 @@ +(builtins.tryEval (({ foo ? throw "up" }: if foo then 1 else 2) { })).success diff --git a/tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.exp b/tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.exp deleted file mode 100644 index c508d5366f70..000000000000 --- a/tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.exp +++ /dev/null @@ -1 +0,0 @@ -false diff --git a/tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.nix b/tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.nix deleted file mode 100644 index 0523cf864c3b..000000000000 --- a/tvix/eval/src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-default-args.nix +++ /dev/null @@ -1 +0,0 @@ -(builtins.tryEval (({ foo ? throw "up" }: if foo then 1 else 2) { })).success diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 003567bfd440..300824b88ff7 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -639,6 +639,7 @@ impl Value { gen_is!(is_number, Value::Integer(_) | Value::Float(_)); gen_is!(is_bool, Value::Bool(_)); gen_is!(is_attrs, Value::Attrs(_)); + gen_is!(is_catchable, Value::Catchable(_)); /// Compare `self` against other using (fallible) Nix ordering semantics. /// diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 6128e3a601a0..642b6317caee 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -562,6 +562,13 @@ impl<'o> VM<'o> { } } + OpCode::OpJumpIfCatchable(JumpOffset(offset)) => { + debug_assert!(offset != 0); + if self.stack_peek(0).is_catchable() { + frame.ip += offset; + } + } + OpCode::OpJumpIfNoFinaliseRequest(JumpOffset(offset)) => { debug_assert!(offset != 0); match self.stack_peek(0) { -- cgit 1.4.1