From cb1ceee58b11b0ce6f8845361af3418d13c506bd Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 29 Feb 2024 11:35:29 +0000 Subject: [PATCH 1/5] fix: build noir_codegen when publishing (#4448) # Description ## Problem\* Resolves #4446 ## Summary\* We're currently not building `noir_codegen` before we publish so I've updated the build script. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f460b3db711..4987602c709 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ "build:types": "yarn workspace @noir-lang/types run build", "build:backend_barretenberg": "yarn workspace @noir-lang/backend_barretenberg run build", "build:noir_js": "yarn workspace @noir-lang/noir_js run build", - "build:js:only": "yarn build:types && yarn build:backend_barretenberg && yarn build:noir_js", + "build:js:only": "yarn workspaces foreach -vtp --from \"{@noir-lang/types,@noir-lang/backend_barretenberg,@noir-lang/noir_js,@noir-lang/noir_codegen}\" run build", "prepare:publish": "yarn clean && yarn install:from:nix && yarn build:js:only", "nightly:version": "yarn workspaces foreach run nightly:version", "publish:all": "yarn install && yarn workspaces foreach run publish" From ebaf05ab10834dd10e04c7ea5130f96c6cdf98ed Mon Sep 17 00:00:00 2001 From: Jordan Ellis Coppard Date: Thu, 29 Feb 2024 21:50:58 +0800 Subject: [PATCH 2/5] fix(flake): stop flake.nix removing ignored-tests.txt (#4455) # Description Fixes a problem with `flake.nix` that prevented building packages via said flake. ## Problem\* Currently `flake.nix` does not keep files ending with `.txt` when building packages, this means https://github.com/noir-lang/noir/blob/master/tooling/debugger/build.rs#L44 fails and so does the build.
Expand for full error trace. ```txt @nix { "action": "setPhase", "phase": "unpackPhase" } unpacking sources unpacking source archive /nix/store/s9q28qnr32r9cd62pm3vapvq87ipdpyq-source source root is source @nix { "action": "setPhase", "phase": "patchPhase" } patching sources Executing configureCargoCommonVars copying cargo artifacts from /nix/store/21iwpq5w9854ga1mmb8g4r4r2p20202b-nargo-deps-0.24.0/target to target @nix { "action": "setPhase", "phase": "updateAutotoolsGnuConfigScriptsPhase" } updateAutotoolsGnuConfigScriptsPhase @nix { "action": "setPhase", "phase": "configurePhase" } configuring will append /private/tmp/nix-build-nargo-0.24.0.drv-0/source/.cargo-home/config.toml with contents of /nix/store/rl1r6p506a4hk57049rsql0cnd9f6236-vendor-cargo-deps/config.toml default configurePhase, nothing to do @nix { "action": "setPhase", "phase": "buildPhase" } building ++ command cargo --version cargo 1.73.0 (9c4383fb5 2023-08-26) ++ command cargo build --release --message-format json-render-diagnostics Compiling acir_field v0.40.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/acvm-repo/acir_field) Compiling iter-extended v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/compiler/utils/iter-extended) Compiling fm v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/compiler/fm) Compiling arena v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/compiler/utils/arena) Compiling noirc_driver v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/compiler/noirc_driver) Compiling nargo v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/tooling/nargo) Compiling bb_abstraction_leaks v0.11.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/tooling/bb_abstraction_leaks) Compiling nargo_fmt v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/tooling/nargo_fmt) Compiling bn254_blackbox_solver v0.39.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/acvm-repo/bn254_blackbox_solver) Compiling noir_debugger v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/tooling/debugger) Compiling nargo_cli v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/tooling/nargo_cli) Compiling brillig v0.40.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/acvm-repo/brillig) error: failed to run custom build command for `noir_debugger v0.24.0 (/private/tmp/nix-build-nargo-0.24.0.drv-0/source/tooling/debugger)` note: To improve backtraces for build dependencies, set the CARGO_PROFILE_RELEASE_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation. Caused by: process didn't exit successfully: `/private/tmp/nix-build-nargo-0.24.0.drv-0/source/target/release/build/noir_debugger-64219c61a62b2d11/build-script-build` (exit status: 101) --- stdout cargo:rerun-if-changed=tests cargo:rerun-if-changed=ignored-tests.txt cargo:rerun-if-changed=/private/tmp/nix-build-nargo-0.24.0.drv-0/source/test_programs --- stderr thread 'main' panicked at tooling/debugger/build.rs:44:74: called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" } stack backtrace: 0: rust_begin_unwind at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:595:5 1: core::panicking::panic_fmt at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:67:14 2: core::result::unwrap_failed at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/result.rs:1652:5 3: core::result::Result::unwrap 4: build_script_build::generate_debugger_tests 5: build_script_build::main 6: core::ops::function::FnOnce::call_once note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. warning: build failed, waiting for other jobs to finish.. ```
Expand for the (probably poorly written) _consuming_ `flake.nix` If you use the patched version `noir.url` (this PR) the build now works. ```nix { description = "Tikan -- Fog of War Chess"; inputs = { nixpkgs.url = "github:nixos/nixpkgs/nixpkgs-unstable"; flake-parts.url = "github:hercules-ci/flake-parts"; devshell.url = "github:numtide/devshell"; noir.url = "github:noir-lang/noir/db9ea8481197286e38b89269aa6e8f14acf1ce93"; # noir.url = "github:tsujp/noir/0cf043d81e2da7c4b23c4b18e0c3a944dd7ea017"; }; outputs = inputs@{ self, nixpkgs, flake-parts, ... }: flake-parts.lib.mkFlake { inherit inputs; } { imports = [ inputs.devshell.flakeModule ]; systems = [ "aarch64-linux" "aarch64-darwin" "x86_64-linux" "x86_64-darwin" ]; perSystem = { pkgs, ... }: rec { packages.noir = inputs.noir.packages; devshells.default = { packages = with pkgs; [ bun ] ++ [ packages.noir.${system}.nargo ]; }; }; }; } ```
## Documentation\* Check one: - [x] No documentation needed. - ~[ ] Documentation included in this PR.~ - ~[ ] **[Exceptional Case]** Documentation to be submitted in a separate PR.~ # PR Checklist\* - [x] I have tested the changes locally. - ~[ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.~ --- flake.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flake.nix b/flake.nix index 4c5db8bfaae..5125dad06be 100644 --- a/flake.nix +++ b/flake.nix @@ -81,7 +81,7 @@ # Custom filter with various file extensions that we rely upon to build packages # Currently: `.nr`, `.sol`, `.sh`, `.json`, `.md` and `.wasm` filter = path: type: - (builtins.match ".*\.(nr|sol|sh|json|md|wasm)$" path != null) || (craneLib.filterCargoSources path type); + (builtins.match ".*\.(nr|sol|sh|json|md|wasm|txt)$" path != null) || (craneLib.filterCargoSources path type); }; # TODO(#1198): It'd be nice to include these flags when running `cargo clippy` in a devShell. From 21fc4b85763dccae6dce0a46a318718c3c913471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Thu, 29 Feb 2024 17:42:28 +0100 Subject: [PATCH 3/5] feat: Add overflow and underflow checks for unsigned integers in brillig (#4445) # Description ## Problem\* When overflow checks were added they were only added for ACIR. This adds add, sub and multiply overflow checks for unsigned operations in brillig. ## Summary\* ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/brillig/brillig_gen/brillig_block.rs | 111 +++++++++++++++--- .../noirc_evaluator/src/brillig/brillig_ir.rs | 30 +++-- .../brillig_fns_as_values/Prover.toml | 2 +- .../brillig_fns_as_values/src/main.nr | 4 +- .../brillig_wrapping/Nargo.toml | 6 + .../brillig_wrapping/Prover.toml | 2 + .../brillig_wrapping/src/main.nr | 8 ++ .../brillig_overflow_checks/Nargo.toml | 5 + .../brillig_overflow_checks/Prover.toml | 0 .../brillig_overflow_checks/src/main.nr | 23 ++++ 10 files changed, 158 insertions(+), 33 deletions(-) create mode 100644 test_programs/execution_success/brillig_wrapping/Nargo.toml create mode 100644 test_programs/execution_success/brillig_wrapping/Prover.toml create mode 100644 test_programs/execution_success/brillig_wrapping/src/main.nr create mode 100644 test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml create mode 100644 test_programs/noir_test_success/brillig_overflow_checks/Prover.toml create mode 100644 test_programs/noir_test_success/brillig_overflow_checks/src/main.nr diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index f01f60252f6..a3ee3e7ca7e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1,9 +1,7 @@ use crate::brillig::brillig_ir::brillig_variable::{ type_to_heap_value_type, BrilligArray, BrilligVariable, BrilligVector, SingleAddrVariable, }; -use crate::brillig::brillig_ir::{ - BrilligBinaryOp, BrilligContext, BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE, -}; +use crate::brillig::brillig_ir::{BrilligBinaryOp, BrilligContext}; use crate::ssa::ir::dfg::CallStack; use crate::ssa::ir::instruction::ConstrainError; use crate::ssa::ir::{ @@ -1197,7 +1195,7 @@ impl<'block> BrilligBlock<'block> { let left = self.convert_ssa_single_addr_value(binary.lhs, dfg); let right = self.convert_ssa_single_addr_value(binary.rhs, dfg); - let brillig_binary_op = + let (brillig_binary_op, is_signed) = convert_ssa_binary_op_to_brillig_binary_op(binary.operator, &binary_type); self.brillig_context.binary_instruction( @@ -1206,6 +1204,93 @@ impl<'block> BrilligBlock<'block> { result_variable.address, brillig_binary_op, ); + + self.add_overflow_check(brillig_binary_op, left, right, result_variable, is_signed); + } + + fn add_overflow_check( + &mut self, + binary_operation: BrilligBinaryOp, + left: SingleAddrVariable, + right: SingleAddrVariable, + result: SingleAddrVariable, + is_signed: bool, + ) { + let (op, bit_size) = if let BrilligBinaryOp::Integer { op, bit_size } = binary_operation { + (op, bit_size) + } else { + return; + }; + + match (op, is_signed) { + (BinaryIntOp::Add, false) => { + let condition = self.brillig_context.allocate_register(); + // Check that lhs <= result + self.brillig_context.binary_instruction( + left.address, + result.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::LessThanEquals, bit_size }, + ); + self.brillig_context.constrain_instruction( + condition, + Some("attempt to add with overflow".to_string()), + ); + self.brillig_context.deallocate_register(condition); + } + (BinaryIntOp::Sub, false) => { + let condition = self.brillig_context.allocate_register(); + // Check that rhs <= lhs + self.brillig_context.binary_instruction( + right.address, + left.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::LessThanEquals, bit_size }, + ); + self.brillig_context.constrain_instruction( + condition, + Some("attempt to subtract with overflow".to_string()), + ); + self.brillig_context.deallocate_register(condition); + } + (BinaryIntOp::Mul, false) => { + // Multiplication overflow is only possible for bit sizes > 1 + if bit_size > 1 { + let is_right_zero = self.brillig_context.allocate_register(); + let zero = self.brillig_context.make_constant(0_usize.into(), bit_size); + self.brillig_context.binary_instruction( + zero, + right.address, + is_right_zero, + BrilligBinaryOp::Integer { op: BinaryIntOp::Equals, bit_size }, + ); + self.brillig_context.if_not_instruction(is_right_zero, |ctx| { + let condition = ctx.allocate_register(); + // Check that result / rhs == lhs + ctx.binary_instruction( + result.address, + right.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::UnsignedDiv, bit_size }, + ); + ctx.binary_instruction( + condition, + left.address, + condition, + BrilligBinaryOp::Integer { op: BinaryIntOp::Equals, bit_size }, + ); + ctx.constrain_instruction( + condition, + Some("attempt to multiply with overflow".to_string()), + ); + ctx.deallocate_register(condition); + }); + self.brillig_context.deallocate_register(is_right_zero); + self.brillig_context.deallocate_register(zero); + } + } + _ => {} + } } /// Converts an SSA `ValueId` into a `RegisterOrMemory`. Initializes if necessary. @@ -1403,8 +1488,6 @@ impl<'block> BrilligBlock<'block> { } /// Returns the type of the operation considering the types of the operands -/// TODO: SSA issues binary operations between fields and integers. -/// This probably should be explicitly casted in SSA to avoid having to coerce at this level. pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type { match (lhs_type, rhs_type) { (_, Type::Function) | (Type::Function, _) => { @@ -1419,10 +1502,6 @@ pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type (_, Type::Slice(..)) | (Type::Slice(..), _) => { unreachable!("Arrays are invalid in binary operations") } - // If either side is a Field constant then, we coerce into the type - // of the other operand - (Type::Numeric(NumericType::NativeField), typ) - | (typ, Type::Numeric(NumericType::NativeField)) => typ.clone(), // If both sides are numeric type, then we expect their types to be // the same. (Type::Numeric(lhs_type), Type::Numeric(rhs_type)) => { @@ -1441,7 +1520,7 @@ pub(crate) fn type_of_binary_operation(lhs_type: &Type, rhs_type: &Type) -> Type pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( ssa_op: BinaryOp, typ: &Type, -) -> BrilligBinaryOp { +) -> (BrilligBinaryOp, bool) { // First get the bit size and whether its a signed integer, if it is a numeric type // if it is not,then we return None, indicating that // it is a Field. @@ -1461,10 +1540,6 @@ pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( BinaryOp::Mul => BrilligBinaryOp::Field { op: BinaryFieldOp::Mul }, BinaryOp::Div => BrilligBinaryOp::Field { op: BinaryFieldOp::Div }, BinaryOp::Eq => BrilligBinaryOp::Field { op: BinaryFieldOp::Equals }, - BinaryOp::Lt => BrilligBinaryOp::Integer { - op: BinaryIntOp::LessThan, - bit_size: BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE, - }, _ => unreachable!( "Field type cannot be used with {op}. This should have been caught by the frontend" ), @@ -1500,7 +1575,9 @@ pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( // If bit size is available then it is a binary integer operation match bit_size_signedness { - Some((bit_size, is_signed)) => binary_op_to_int_op(ssa_op, *bit_size, is_signed), - None => binary_op_to_field_op(ssa_op), + Some((bit_size, is_signed)) => { + (binary_op_to_int_op(ssa_op, *bit_size, is_signed), is_signed) + } + None => (binary_op_to_field_op(ssa_op), false), } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 90608974f98..f1a8f24ed03 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -29,17 +29,6 @@ use acvm::{ use debug_show::DebugShow; use num_bigint::BigUint; -/// Integer arithmetic in Brillig is limited to 127 bit -/// integers. -/// -/// We could lift this in the future and have Brillig -/// do big integer arithmetic when it exceeds the field size -/// or we could have users re-implement big integer arithmetic -/// in Brillig. -/// Since constrained functions do not have this property, it -/// would mean that unconstrained functions will differ from -/// constrained functions in terms of syntax compatibility. -pub(crate) const BRILLIG_INTEGER_ARITHMETIC_BIT_SIZE: u32 = 127; /// The Brillig VM does not apply a limit to the memory address space, /// As a convention, we take use 64 bits. This means that we assume that /// memory has 2^64 memory slots. @@ -356,6 +345,21 @@ impl BrilligContext { self.enter_section(end_section); } + /// This instruction issues a branch that jumps over the code generated by the given function if the condition is truthy + pub(crate) fn if_not_instruction( + &mut self, + condition: MemoryAddress, + f: impl FnOnce(&mut BrilligContext), + ) { + let (end_section, end_label) = self.reserve_next_section_label(); + + self.jump_if_instruction(condition, end_label.clone()); + + f(self); + + self.enter_section(end_section); + } + /// Adds a label to the next opcode pub(crate) fn enter_context(&mut self, label: T) { self.debug_show.enter_context(label.to_string()); @@ -533,7 +537,7 @@ impl BrilligContext { result: MemoryAddress, operation: BrilligBinaryOp, ) { - self.debug_show.binary_instruction(lhs, rhs, result, operation.clone()); + self.debug_show.binary_instruction(lhs, rhs, result, operation); match operation { BrilligBinaryOp::Field { op } => { let opcode = BrilligOpcode::BinaryFieldOp { op, destination: result, lhs, rhs }; @@ -1082,7 +1086,7 @@ impl BrilligContext { } /// Type to encapsulate the binary operation types in Brillig -#[derive(Clone)] +#[derive(Clone, Copy)] pub(crate) enum BrilligBinaryOp { Field { op: BinaryFieldOp }, Integer { op: BinaryIntOp, bit_size: u32 }, diff --git a/test_programs/execution_success/brillig_fns_as_values/Prover.toml b/test_programs/execution_success/brillig_fns_as_values/Prover.toml index 11497a473bc..4dd6b405159 100644 --- a/test_programs/execution_success/brillig_fns_as_values/Prover.toml +++ b/test_programs/execution_success/brillig_fns_as_values/Prover.toml @@ -1 +1 @@ -x = "0" +x = "1" diff --git a/test_programs/execution_success/brillig_fns_as_values/src/main.nr b/test_programs/execution_success/brillig_fns_as_values/src/main.nr index ea3148915b8..9248bff2f4c 100644 --- a/test_programs/execution_success/brillig_fns_as_values/src/main.nr +++ b/test_programs/execution_success/brillig_fns_as_values/src/main.nr @@ -7,9 +7,9 @@ struct MyStruct { fn main(x: u32) { assert(wrapper(increment, x) == x + 1); assert(wrapper(increment_acir, x) == x + 1); - assert(wrapper(decrement, x) == std::wrapping_sub(x, 1)); + assert(wrapper(decrement, x) == x - 1); assert(wrapper_with_struct(MyStruct { operation: increment }, x) == x + 1); - assert(wrapper_with_struct(MyStruct { operation: decrement }, x) == std::wrapping_sub(x, 1)); + assert(wrapper_with_struct(MyStruct { operation: decrement }, x) == x - 1); // https://github.com/noir-lang/noir/issues/1975 assert(increment(x) == x + 1); } diff --git a/test_programs/execution_success/brillig_wrapping/Nargo.toml b/test_programs/execution_success/brillig_wrapping/Nargo.toml new file mode 100644 index 00000000000..a52246ba908 --- /dev/null +++ b/test_programs/execution_success/brillig_wrapping/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "brillig_wrapping" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/brillig_wrapping/Prover.toml b/test_programs/execution_success/brillig_wrapping/Prover.toml new file mode 100644 index 00000000000..346fd2764a7 --- /dev/null +++ b/test_programs/execution_success/brillig_wrapping/Prover.toml @@ -0,0 +1,2 @@ +x = 0 +y = 255 diff --git a/test_programs/execution_success/brillig_wrapping/src/main.nr b/test_programs/execution_success/brillig_wrapping/src/main.nr new file mode 100644 index 00000000000..4153a466057 --- /dev/null +++ b/test_programs/execution_success/brillig_wrapping/src/main.nr @@ -0,0 +1,8 @@ +use dep::std; + +unconstrained fn main(x: u8, y: u8) { + assert(std::wrapping_sub(x, 1) == y); + assert(std::wrapping_add(y, 1) == x); + assert(std::wrapping_mul(y, y) == 1); +} + diff --git a/test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml b/test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml new file mode 100644 index 00000000000..b2d47d258ed --- /dev/null +++ b/test_programs/noir_test_success/brillig_overflow_checks/Nargo.toml @@ -0,0 +1,5 @@ +[package] +name = "brillig_overflow_checks" +type = "bin" +authors = [""] +[dependencies] diff --git a/test_programs/noir_test_success/brillig_overflow_checks/Prover.toml b/test_programs/noir_test_success/brillig_overflow_checks/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/noir_test_success/brillig_overflow_checks/src/main.nr b/test_programs/noir_test_success/brillig_overflow_checks/src/main.nr new file mode 100644 index 00000000000..5d73ef96d49 --- /dev/null +++ b/test_programs/noir_test_success/brillig_overflow_checks/src/main.nr @@ -0,0 +1,23 @@ +use dep::std::field::bn254::{TWO_POW_128, assert_gt}; + +#[test(should_fail_with = "attempt to add with overflow")] +unconstrained fn test_overflow_add() { + let a: u8 = 255; + let b: u8 = 1; + assert_eq(a + b, 0); +} + +#[test(should_fail_with = "attempt to subtract with overflow")] +unconstrained fn test_overflow_sub() { + let a: u8 = 0; + let b: u8 = 1; + assert_eq(a - b, 255); +} + +#[test(should_fail_with = "attempt to multiply with overflow")] +unconstrained fn test_overflow_mul() { + let a: u8 = 128; + let b: u8 = 2; + assert_eq(a * b, 0); +} + From ac60ef5e12fcfb907fbdcff709d7cbad05f2b939 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 1 Mar 2024 04:18:23 -0600 Subject: [PATCH 4/5] fix: Variables from trait constraints being permanently bound over when used within a trait impl (#4450) # Description ## Problem\* Resolves #4436 ## Summary\* This issue stemmed from calling another trait method within an impl for the trait. The trait method required a trait constraint of the same trait to be valid and was using the trait with its original type variables. These would later be bound over when an impl was searched for for this trait. I've added a somewhat hacky solution of avoiding inserting `t = t` bindings from the trait constraint when checking identifiers. Avoiding inserting this binding means `t` in this case will later be instantiated to a fresh type variable later on in the `instantiate_with_bindings` call. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/hir/type_check/expr.rs | 5 ++- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- .../regression_4436/Nargo.toml | 5 +++ .../regression_4436/src/main.nr | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 test_programs/execution_success/regression_4436/Nargo.toml create mode 100644 test_programs/execution_success/regression_4436/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index b78f07c88f2..7b854e58fca 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -326,7 +326,10 @@ impl<'interner> TypeChecker<'interner> { assert_eq!(the_trait.generics.len(), constraint.trait_generics.len()); for (param, arg) in the_trait.generics.iter().zip(&constraint.trait_generics) { - bindings.insert(param.id(), (param.clone(), arg.clone())); + // Avoid binding t = t + if !arg.occurs(param.id()) { + bindings.insert(param.id(), (param.clone(), arg.clone())); + } } } diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index e105da1ccf0..b70aa43701c 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1558,7 +1558,7 @@ impl Type { } /// True if the given TypeVariableId is free anywhere within self - fn occurs(&self, target_id: TypeVariableId) -> bool { + pub fn occurs(&self, target_id: TypeVariableId) -> bool { match self { Type::Array(len, elem) => len.occurs(target_id) || elem.occurs(target_id), Type::String(len) => len.occurs(target_id), diff --git a/test_programs/execution_success/regression_4436/Nargo.toml b/test_programs/execution_success/regression_4436/Nargo.toml new file mode 100644 index 00000000000..0904d858596 --- /dev/null +++ b/test_programs/execution_success/regression_4436/Nargo.toml @@ -0,0 +1,5 @@ +[package] +name = "regression_4436" +type = "bin" +authors = [""] +compiler_version = ">=0.22.0" diff --git a/test_programs/execution_success/regression_4436/src/main.nr b/test_programs/execution_success/regression_4436/src/main.nr new file mode 100644 index 00000000000..834ea3250cc --- /dev/null +++ b/test_programs/execution_success/regression_4436/src/main.nr @@ -0,0 +1,31 @@ +trait LibTrait { + fn broadcast(); + fn get_constant() -> Field; +} + +global STRUCT_A_LEN: Field = 3; +global STRUCT_B_LEN: Field = 5; + +struct StructA; +struct StructB; + +impl LibTrait for StructA { + fn broadcast() { + Self::get_constant(); + } + + fn get_constant() -> Field { + 1 + } +} +impl LibTrait for StructB { + fn broadcast() { + Self::get_constant(); + } + + fn get_constant() -> Field { + 1 + } +} + +fn main() {} From cb4c1c5264b95d01f69d99f916ced71ad9cdc9d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Fri, 1 Mar 2024 11:51:00 +0100 Subject: [PATCH 5/5] feat: skip redundant range checks in brillig (#4460) # Description ## Problem\* Avoid codegening overhead for no-op range checks generated by https://github.com/noir-lang/noir/issues/4456 ## Summary\* ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/brillig/brillig_gen/brillig_block.rs | 59 ++++++++++--------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index a3ee3e7ca7e..c04d8475f08 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -624,37 +624,40 @@ impl<'block> BrilligBlock<'block> { } Instruction::RangeCheck { value, max_bit_size, assert_message } => { let value = self.convert_ssa_single_addr_value(*value, dfg); - // Cast original value to field - let left = SingleAddrVariable { - address: self.brillig_context.allocate_register(), - bit_size: FieldElement::max_num_bits(), - }; - self.convert_cast(left, value); + // SSA generates redundant range checks. A range check with a max bit size >= value.bit_size will always pass. + if value.bit_size > *max_bit_size { + // Cast original value to field + let left = SingleAddrVariable { + address: self.brillig_context.allocate_register(), + bit_size: FieldElement::max_num_bits(), + }; + self.convert_cast(left, value); - // Create a field constant with the max - let max = BigUint::from(2_u128).pow(*max_bit_size) - BigUint::from(1_u128); - let right = self.brillig_context.make_constant( - FieldElement::from_be_bytes_reduce(&max.to_bytes_be()).into(), - FieldElement::max_num_bits(), - ); + // Create a field constant with the max + let max = BigUint::from(2_u128).pow(*max_bit_size) - BigUint::from(1_u128); + let right = self.brillig_context.make_constant( + FieldElement::from_be_bytes_reduce(&max.to_bytes_be()).into(), + FieldElement::max_num_bits(), + ); - // Check if lte max - let brillig_binary_op = BrilligBinaryOp::Integer { - op: BinaryIntOp::LessThanEquals, - bit_size: FieldElement::max_num_bits(), - }; - let condition = self.brillig_context.allocate_register(); - self.brillig_context.binary_instruction( - left.address, - right, - condition, - brillig_binary_op, - ); + // Check if lte max + let brillig_binary_op = BrilligBinaryOp::Integer { + op: BinaryIntOp::LessThanEquals, + bit_size: FieldElement::max_num_bits(), + }; + let condition = self.brillig_context.allocate_register(); + self.brillig_context.binary_instruction( + left.address, + right, + condition, + brillig_binary_op, + ); - self.brillig_context.constrain_instruction(condition, assert_message.clone()); - self.brillig_context.deallocate_register(condition); - self.brillig_context.deallocate_register(left.address); - self.brillig_context.deallocate_register(right); + self.brillig_context.constrain_instruction(condition, assert_message.clone()); + self.brillig_context.deallocate_register(condition); + self.brillig_context.deallocate_register(left.address); + self.brillig_context.deallocate_register(right); + } } Instruction::IncrementRc { value } => { let rc_register = match self.convert_ssa_value(*value, dfg) {