Skip to content

fix: avoid division by zero#8181

Closed
guipublic wants to merge 4 commits intomasterfrom
gd/issue_8175
Closed

fix: avoid division by zero#8181
guipublic wants to merge 4 commits intomasterfrom
gd/issue_8175

Conversation

@guipublic
Copy link
Contributor

Description

Problem*

Resolves #8175

Summary*

The zero check is not working for some reason when doing constant folding division in acir-gen, instead I check the number that is used for the constant folding.

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*

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

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

Benchmark suite Current: 2c4ed3d Previous: ae008d3 Ratio
test_report_noir-lang_noir_bigcurve_ 278 s 220 s 1.26

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

CC: @TomAFrench

@guipublic guipublic requested a review from a team April 23, 2025 14:54
// If `lhs` and `rhs` are known constants then we can calculate the result at compile time.
// `rhs` must be non-zero.
(Some(lhs_const), Some(rhs_const), _) if !rhs_const.is_zero() => {
(Some(lhs_const), Some(rhs_const), _) if rhs_const.to_u128() != 0 => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I checked this and the case where there's a difference is for this field element: 340282366920938463463374607431768211456.
That's not zero, but to_u128 will return zero as it overflows the maximum u128.

Just noting this here, the fix is probably good.

@aakoshh
Copy link
Contributor

aakoshh commented Apr 23, 2025

🥷 #8180

@aakoshh
Copy link
Contributor

aakoshh commented Apr 23, 2025

I thought about fixing it this way, but decided that we can figure it out at compile time, we have the information (the constants) we just shouldn’t divide.

@guipublic
Copy link
Contributor Author

Superseded by #8180

@guipublic guipublic closed this Apr 25, 2025
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 in acir_context: attempt to divide by zero

3 participants