Skip to content

chore(test): Replicate comptime stack overflow in a test#8473

Merged
TomAFrench merged 2 commits intomasterfrom
af/comptime-stack-overflow
May 13, 2025
Merged

chore(test): Replicate comptime stack overflow in a test#8473
TomAFrench merged 2 commits intomasterfrom
af/comptime-stack-overflow

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented May 13, 2025

Description

Problem*

Resolves an issue frequently mentioned by @rkarabut, that the comptime vs brillig fuzzer gets stack overflow.

Summary*

I ran the following test after adding a few prints to check how far it gets before it gets a stack overflow, and printed the source that caused it.

cargo test -p noir_ast_fuzzer_fuzz comptime_vs_brillig -- --nocapture

We get this:

thread 'targets::comptime_vs_brillig::tests::fuzz_with_arbtest' has overflowed its stack
fatal runtime error: stack overflow

It turned out it was during the call to prepare_and_compile_snippet. I added a unit test with one of the offending examples:

cargo test -p noir_ast_fuzzer test_prepare
    Finished `test` profile [optimized + debuginfo] target(s) in 0.14s
     Running unittests src/lib.rs (/Users/aakoshh/Work/aztec/noir/target/debug/deps/noir_ast_fuzzer-43119138323f732a)

running 1 test

thread 'compare::comptime::tests::test_prepare_and_compile_snippet' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `-p noir_ast_fuzzer --lib`

I cannot replicate this stack overflow with nargo.

See comments for why.

Decided to lower the limits in comptime fuzzing.

Additional Context

After adding some prints, the overflow appears in the Elaborator:

$ cargo test -p noir_ast_fuzzer test_prepare -- --nocapture
...
elaborate_trait_impl
elaborate_functions
elaborate_function 881
elaborate_function 882
elaborate_functions
elaborate_function 883
elaborate_function 884
check_and_pop_function_context
collect_defs with ModCollector
elaborate_items
elaborate_items
run_attributes
elaborate_functions
elaborate_function 14515
elaborate_function 14517
elaborate_function 14516

thread 'compare::comptime::tests::test_prepare_and_compile_snippet' has overflowed its stack
fatal runtime error: stack overflow

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
Copy link
Contributor Author

aakoshh commented May 13, 2025

Ary Borenszweig
It's interesting because it doesn't overflow if you paste that code into a file and compile it
But... if you change ctx_limit to 50 then it stack overflows
My guess is that because it runs in a test, the stack has already some depth so it reaches the overflow faster

That said... the situation can be improved a bit
If you apply this patch, it doesn't stack overflow anymore, at least not with 25:

diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs
index 605c46115a..9b0d6416f6 100644
--- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs
+++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs
@@ -508,6 +508,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {

     /// Evaluate an expression and return the result.
     /// This will automatically dereference a mutable variable if used.
+    #[inline(always)]
     pub fn evaluate(&mut self, id: ExprId) -> IResult<Value> {
         match self.evaluate_no_dereference(id)? {
             Value::Pointer(elem, true, _) => Ok(elem.borrow().clone()),
@@ -518,6 +519,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
     /// Evaluating a mutable variable will dereference it automatically.
     /// This function should be used when that is not desired - e.g. when
     /// compiling a `&mut var` expression to grab the original reference.
+    #[inline(always)]
     fn evaluate_no_dereference(&mut self, id: ExprId) -> IResult<Value> {
         match self.elaborator.interner.expression(&id) {
             HirExpression::Ident(ident, _) => self.evaluate_ident(ident, id),
@@ -710,6 +712,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
         evaluate_integer(typ, value, location)
     }

+    #[inline(always)]
     pub fn evaluate_block(&mut self, mut block: HirBlockExpression) -> IResult<Value> {
         let last_statement = block.statements.pop();
         self.push_scope();
@@ -1053,6 +1056,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
         Ok(Value::Quoted(Rc::new(tokens)))
     }

+    #[inline(always)]
     pub fn evaluate_statement(&mut self, statement: StmtId) -> IResult<Value> {
         match self.elaborator.interner.statement(&statement) {
             HirStatement::Let(let_) => self.evaluate_let(let_),

I used that trick to allow for more deeply nested structures in the parser too: #8347
When I kept adding #[inline] it would cause compile times (to compile the compiler) to balloon
Like, it never finished compiling

@aakoshh aakoshh marked this pull request as ready for review May 13, 2025 12:06
@aakoshh aakoshh requested a review from rkarabut May 13, 2025 12:07
@aakoshh
Copy link
Contributor Author

aakoshh commented May 13, 2025

Thanks for investigating @asterite 🙏

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 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 23fc3cc Previous: 05ae1f3 Ratio
private-kernel-tail 1.376 s 1.08 s 1.27

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

CC: @TomAFrench

@TomAFrench TomAFrench added this pull request to the merge queue May 13, 2025
Merged via the queue into master with commit a21ac45 May 13, 2025
122 checks passed
@TomAFrench TomAFrench deleted the af/comptime-stack-overflow branch May 13, 2025 16:12
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 14, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: pass Field to ToBits intrinsic in remove_bit_shifts optimization
(noir-lang/noir#8493)
fix: don't produce `index Field` in value merger
(noir-lang/noir#8492)
fix: allowing accessing associated constants via `Self::...`
(noir-lang/noir#8403)
fix: remove unused generic in static_assert
(noir-lang/noir#8488)
fix: disallow generics on entry points
(noir-lang/noir#8490)
chore(test): Replicate comptime stack overflow in a test
(noir-lang/noir#8473)
feat: `#[test(only_fail_with = "...")]`
(noir-lang/noir#8460)
fix: variable used in fmtstr inside lambda wasn't tracked as captured
(noir-lang/noir#8487)
chore(test): Add more tests for defunctionalization
(noir-lang/noir#8481)
END_COMMIT_OVERRIDE

Co-authored-by: AztecBot <tech@aztecprotocol.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.

4 participants