Skip to content

fix: Handle &mut function in defunctionalize#8665

Merged
aakoshh merged 13 commits intomasterfrom
af/8662-fix-defunct-mut-func
May 24, 2025
Merged

fix: Handle &mut function in defunctionalize#8665
aakoshh merged 13 commits intomasterfrom
af/8662-fix-defunct-mut-func

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented May 23, 2025

Description

Problem*

Resolves #8662

Summary*

The defunctionalize pass did not handle &mut function parameters, just function. OTOH the post-check looked for function references and failed. This didn't cause a problem on CI because the assertion only works in debug mode, and CI runs in release mode. Only by accident did we find that using cargo run to try to compile one of the contracts in aztec-packages fails due to the post check.

Additional Context

The AST fuzzer did not find this case because it doesn't generate reference parameters yet.

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.

@aakoshh aakoshh requested review from a team, guipublic and michaeljklein May 23, 2025 14:15
@aakoshh aakoshh enabled auto-merge May 23, 2025 14:45
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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: e59311a Previous: c37ded9 Ratio
semaphore-depth-10 0.025 s 0.019 s 1.32

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

CC: @TomAFrench

@aakoshh
Copy link
Contributor Author

aakoshh commented May 24, 2025

My changes to the defunctionalization resulted in the expected SSA string, but later during inlining it turned out that the value of it was wrong, causing a crash during Brillig gen.

The fix was this: 642e660
But the unit test doesn't capture the problem.

I tested the crash with this command:

$ cargo run -q -p nargo_cli -- --program-dir /Users/aakoshh/Work/aztec/aztec-packages/noir-projects/noir-contracts/contracts/app/easy_private_voting compile --silence-warnings --package easy_private_voting_contract --show-ssa
...
The application panicked (crashed).
Message:  internal error: entered unreachable code: unsupported function call type NumericConstant { constant: 33, typ: NativeField }
Location: compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs:766

There is a part which before defunctionalization looks like this:

brillig(inline) fn add_to_tally_public f0 {
  b0(v2: Field):
    ...
    v48, v49, v50, v51 = call f10(v30, v31, v32, v33, v34, v2) -> (&mut u1, &mut Field, &mut function, Field)

brillig(inline) fn at f10 {
  b0(v2: &mut u1, v3: &mut Field, v4: &mut function, v5: Field, v6: function, v7: Field):
    v9 = call f16(v5, v7) -> Field
    v10, v11, v12, v13 = call v6(v2, v3, v4, v9) -> (&mut u1, &mut Field, &mut function, Field)
    return v10, v11, v12, v13
}

after defunctionalization:

brillig(inline) fn at f10 {
  b0(v2: &mut u1, v3: &mut Field, v4: &mut Field, v5: Field, v6: Field, v7: Field):
    v9 = call f16(v5, v7) -> Field
    v10, v11, v12, v13 = call v6(v2, v3, v4, v9) -> (&mut u1, &mut Field, Field, Field)
    return v10, v11, v12, v13
}

but after inlining:

brillig(inline) fn add_to_tally_public f0 {
  b0(v2: Field):
    ...
    v29, v30, v31, v32 = call Field 33(v3, v5, v7, v27) -> (&mut u1, &mut Field, Field, Field)

and then it crashes because it's trying to call Field 33 instead of f33 which it would if the defunctionalization worked correctly.

Unfortunately my minimised hand written SSA didn't try to call v6, even before the fix it was correctly forwarding to f33. In the interest of moving forward with the fix I gave up trying to capture the exact circumstances for now and just happy that the external tests would not have let this pass.

I'll try to extend the AST fuzzer to use references to cover this case.

@aakoshh aakoshh added this pull request to the merge queue May 24, 2025
Merged via the queue into master with commit a37ba28 May 24, 2025
118 checks passed
@aakoshh aakoshh deleted the af/8662-fix-defunct-mut-func branch May 24, 2025 08:49
@aakoshh
Copy link
Contributor Author

aakoshh commented May 24, 2025

I'll try to replicate it as an integration test as well next week.

github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 1, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(fmt): correctly format mixed secondary attributes and doc comments
(noir-lang/noir#8735)
fix!: require types for trait impl associated constants
(noir-lang/noir#8734)
fix!: Prevent returning references from if expressions
(noir-lang/noir#8731)
fix: cast signed to u1 follow-up
(noir-lang/noir#8730)
fix: cast signed to u1 (noir-lang/noir#8720)
fix: do not mutate arrays later copied inside other arrays
(noir-lang/noir#8701)
chore: box `AsTraitPath` and `TypePath` in `ExpressionKind`
(noir-lang/noir#8716)
fix: general solution for accessing associated constants
(noir-lang/noir#8417)
fix(ssa): Validate checked signed add/sub is followed by a truncate
(noir-lang/noir#8706)
chore: add pre- and post-check on `array_set` optimization pass
(noir-lang/noir#8714)
fix: merge expr bindings with instantiations bindings during
monomorphization (noir-lang/noir#8713)
fix: better way to do LSP file overrides
(noir-lang/noir#8702)
fix(fmt): correct indentation when formatting long struct patterns
(noir-lang/noir#8711)
fix(fuzz): Prevent breaking/continuing out from let blocks in the AST
fuzzer (noir-lang/noir#8708)
chore: remove override for zero length inputs
(noir-lang/noir#8709)
feat: Replace converging jmpif with empty block destinations with a
single jmp (noir-lang/noir#8613)
feat(cli): Add `nargo interpret` command
(noir-lang/noir#8700)
chore: more 1-tuple printing fixes
(noir-lang/noir#8699)
chore(fuzz): Fuzz the SSA parser
(noir-lang/noir#8647)
fix: (SSA parser) translate blocks in logical order rather than syntax
order (noir-lang/noir#8668)
fix(SSA): show and parse range_check's assert_message
(noir-lang/noir#8652)
chore: Show the step number in the SSA message
(noir-lang/noir#8698)
chore(docs): Add pointers to tests
(noir-lang/noir#8695)
chore(fuzz): Capture comptime print output
(noir-lang/noir#8635)
fix: nargo expand reexports correctly implemented
(noir-lang/noir#8693)
feat(cli): Show multiple SSA passes
(noir-lang/noir#8692)
chore(test): Add regression test for defunctionalize
(noir-lang/noir#8691)
chore: add an assertion when parsing SSA that all functions are
well-formed (noir-lang/noir#8671)
feat!: prevent compiling blake3 hashes which barretenberg cannot prove
(noir-lang/noir#8690)
fix(ssa): Remove the array cache from the function inserter
(noir-lang/noir#8607)
chore: bump external pinned commits
(noir-lang/noir#8684)
chore: reenable fuzzing tests in CI
(noir-lang/noir#8688)
fix: Handle `&mut function` in defunctionalize
(noir-lang/noir#8665)
fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and
HOFs in arrays (noir-lang/noir#8672)
chore(ci): avoid running fuzzer tests in the merge queue
(noir-lang/noir#8664)
fix: Make casts in `comptime` consistent with runtime casts
(noir-lang/noir#8669)
fix: relax connectedness requirement on purity analysis pass
(noir-lang/noir#8667)
fix: always use `u32` for indexing arrays in SSA
(noir-lang/noir#8633)
chore: explicitly pull from `next` branch in aztec-packages
(noir-lang/noir#8660)
chore(fuzz): Build the dictionary from the SSA
(noir-lang/noir#8591)
chore(docs): Remove old versioned docs
(noir-lang/noir#8061)
chore: bump external pinned commits
(noir-lang/noir#8634)
fix(SSA): don't use string literal if byte is "form feed" ('\f')
(noir-lang/noir#8653)
chore(defunctionalize): Add regression test for missing lambda variants
panic (noir-lang/noir#8642)
chore(ci): Do not run ast_fuzzer orig vs. morph in ci
(noir-lang/noir#8646)
fix: disable underflow fix for fields
(noir-lang/noir#8631)
fix: revert "fix: error on unused generic in trait impl
(noir-lang/noir#8395)"
(noir-lang/noir#8636)
fix(ssa interpreter): Default to zero when we have an overflowing shl
(noir-lang/noir#8638)
fix: ensure that purity analysis pass explores all functions
(noir-lang/noir#8452)
feat: acir_formal_proofs (noir-lang/noir#8140)
fix: error on unused generic in trait impl
(noir-lang/noir#8395)
fix(expand): use re-exports for non-visibile items
(noir-lang/noir#8374)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compiler crash: defunctionalize post check failure

2 participants