fix(interpreter): Do not overflow on signed checked ops by vezenovm · Pull Request #8806 · noir-lang/noir

vezenovm · 2025-06-05T16:50:46Z

Description

Problem*

Resolves #8616
Resolves #8621

Does not fully resolve #8618, but both error with the same result now.

Summary*

We should not overflow on a signed add/sub/mul operation immediately. They are non side effectual and we leave checking their overflow to other instructions in the SSA.

Additional Context

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `dc6552b`	Previous: `ce2fc37`	Ratio
`rollup-merge`	`0.004` s	`0.003` s	`1.33`

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

jfecher

Looks good but let's add a test for this case

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `5b9e139`	Previous: `2e5cd41`	Ratio
`test_report_zkpassport_noir_rsa_`	`1` s	`0` s	`+∞`

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

vezenovm · 2025-06-05T19:07:38Z

Looks good but let's add a test for this case

I added a test for each variant of add/sub/mul, safe/overflow, unsigned/signed. Let me know if I missed one.

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs

vezenovm · 2025-06-06T18:10:56Z

Looks like we are getting some comptime_vs_brillig tests are failures in CI @aakoshh. However, when I run NOIR_ARBTEST_SEED=0xef5201d60003781c cargo test -p noir_ast_fuzzer_fuzz comptime_vs_brillig (the failing seed from https://github.com/noir-lang/noir/actions/runs/15495529853/job/43631917730?pr=8806) locally the test passes.

aakoshh · 2025-06-06T18:21:07Z

Yeah I don’t know why it would work differently, maybe it merged master. But it did print both ASTs, can you try those?

vezenovm · 2025-06-06T18:45:16Z

Yeah I don’t know why it would work differently, maybe it merged master. But it did print both ASTs, can you try those?

The ASTs give different results, but non-deterministically...

For the first element of the return array we expect Field(0). However, we got 109746 in comptime. After running again I started getting 0. I have not been able to reproduce Field(109746) in my result again.
I ran for i in {1..100}; do echo "Run #$i"; NOIR_ARBTEST_SEED=0xef5201d60003781c cargo test -p noir_ast_fuzzer_fuzz comptime_vs_brillig || break; done to see if I could reproduce it again but kept getting passes...

aakoshh · 2025-06-06T18:49:21Z

Okay, thanks, I’ll try to find the non-determinism.

But if you try the two ASTs printed, you do get those printed different results, right?

vezenovm · 2025-06-06T18:53:00Z

But if you try the two ASTs printed, you do get those printed different results, right?

I did the first time, but not after trying again. I think I may know the issue. We are out of date with master after #8805 where we starting doing a wrapping shr.

vezenovm · 2025-06-06T18:54:57Z

Don't worry about debugging too deeply, I will fix post #8805 @aakoshh. The non-determinism is still strange though, could be related to merging master and version mismatches.

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs

aakoshh · 2025-06-06T19:02:14Z

Since we mentioned wrapping shifts: #8816

vezenovm · 2025-06-06T19:09:28Z

I did the first time, but not after trying again. I think I may know the issue. We are out of date with master after #8805 where we starting doing a wrapping shr.

The fuzz flakes are definitely due to #8805 causing a mismatch in shr functionality. This job for #8765 is also failing https://github.com/noir-lang/noir/actions/runs/15496878128/job/43636244106?pr=8765. I'm working on a fix.

vezenovm · 2025-06-06T19:12:42Z

So the failing job is a separate bug where we have an interpreter/comptime/runtime mismatch for shift right. This PR is still good to merge as is as it gets the interpreter in line with ACIR/Brillig.

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore(docs): Update noirjs app page to use to beta.6 (noir-lang/noir#8853) fix: support recursive call to main function in SSA parser (noir-lang/noir#8760) chore(SSA): validate that constrain values have the same type (noir-lang/noir#8850) fix: Comptime field division should error when the rhs is zero (noir-lang/noir#8845) chore: redo typo PR by osrm (noir-lang/noir#8840) fix: (SSA interpreter) to_le_bits returns [u1; _], not [u8; _] (noir-lang/noir#8837) chore(ssa): Initial validation module (noir-lang/noir#8765) chore: bump external pinned commits (noir-lang/noir#8834) feat(fuzz): Generate arbitrary constraints (noir-lang/noir#8820) fix: bind self generic type in trait calls via a concrete type in more cases (noir-lang/noir#8827) fix(comptime): Overflow on shl (noir-lang/noir#8829) fix(interpreter): Return -1 for negative shr signed overflow or 0 for positive shr signed overflow (noir-lang/noir#8828) feat(ssa_fuzzer): branching + constrains (noir-lang/noir#8599) chore(docs): Add experimental warning in Debugger docs (noir-lang/noir#8824) fix: Thread errors through remove_if_else instead of panicing when the value merger finds reference values (noir-lang/noir#8783) fix(interpreter): Do not overflow on signed checked ops (noir-lang/noir#8806) feat: short circuit creation of `Type::InfixExpr` containing errors (noir-lang/noir#8826) fix(mem2reg): Keep last stores used in array returned from a function (noir-lang/noir#8801) chore(ci): `cargo clippy` CI script to save time (noir-lang/noir#8787) chore: only follow bindings on interface to `arithmetic` module (noir-lang/noir#8822) fix: bind self generic type in trait calls via a concrete type (noir-lang/noir#8825) chore(docs): Reorder tooling docs (noir-lang/noir#8742) chore: small fix for outdated docs (noir-lang/noir#8821) fix(mem2reg): Keep last stores used in MakeArray (noir-lang/noir#8743) fix!: Error when re-assigning a mutable reference (noir-lang/noir#8790) fix!: indexing arrays with non-u32 is now an error (noir-lang/noir#8804) fix: signed right shift overflows to 0 or -1 (noir-lang/noir#8805) chore(fuzz): Tool to minimize Noir programs with `cvise` (noir-lang/noir#8789) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

do not overflow on signed checked ops

fb192b0

This was referenced Jun 5, 2025

DIE removes overflowing multiplication #8621

Closed

SSA Interpreter: Internal error where Brillig succeeds and ACIR fails with constraint #8616

Closed

vezenovm requested review from a team, aakoshh and jfecher June 5, 2025 16:56

github-actions bot reviewed Jun 5, 2025

View reviewed changes

jfecher requested changes Jun 5, 2025

View reviewed changes

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs Show resolved Hide resolved

add tests

c1c2387

github-actions bot reviewed Jun 5, 2025

View reviewed changes

more tests

dc6552b

vezenovm requested a review from jfecher June 5, 2025 19:07

fix bad ssa for signed add

71ee021

vezenovm mentioned this pull request Jun 6, 2025

fix(comptime): Do not overflow on signed checked ops #8814

Closed

5 tasks

aakoshh reviewed Jun 6, 2025

View reviewed changes

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs Show resolved Hide resolved

add comments to binop macros

5b9e139

Merge branch 'master' into mv/no-overflow-ssa-interpreter-signed-ops

4adfd42

jfecher reviewed Jun 6, 2025

View reviewed changes

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs Show resolved Hide resolved

jfecher approved these changes Jun 6, 2025

View reviewed changes

vezenovm enabled auto-merge June 6, 2025 19:18

vezenovm added this pull request to the merge queue Jun 6, 2025

Merged via the queue into master with commit 851cfd2 Jun 6, 2025
117 checks passed

vezenovm deleted the mv/no-overflow-ssa-interpreter-signed-ops branch June 6, 2025 19:49

noirwhal mentioned this pull request Jun 6, 2025

chore: Release Noir(1.0.0-beta.7) #8520

Merged

vezenovm mentioned this pull request Jun 6, 2025

fix(interpreter): Return -1 for negative shr signed overflow or 0 for positive shr signed overflow #8828

Merged

5 tasks

AztecBot mentioned this pull request Jun 7, 2025

chore: Bump Noir reference AztecProtocol/aztec-packages#14878

Merged

AztecBot mentioned this pull request Jun 10, 2025

chore(next): release 0.88.0 AztecProtocol/aztec-packages#14495

Closed

Conversation

vezenovm commented Jun 5, 2025

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

jfecher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

vezenovm commented Jun 5, 2025

Uh oh!

Uh oh!

vezenovm commented Jun 6, 2025

Uh oh!

aakoshh commented Jun 6, 2025

Uh oh!

vezenovm commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aakoshh commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vezenovm commented Jun 6, 2025

Uh oh!

vezenovm commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aakoshh commented Jun 6, 2025

Uh oh!

vezenovm commented Jun 6, 2025

Uh oh!

vezenovm commented Jun 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

vezenovm commented Jun 6, 2025 •

edited

Loading

aakoshh commented Jun 6, 2025 •

edited

Loading

vezenovm commented Jun 6, 2025 •

edited

Loading