Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content of a struct gets "destroyed" #5771

Closed
benesjan opened this issue Aug 20, 2024 · 7 comments · Fixed by #5959 or #6434
Closed

Content of a struct gets "destroyed" #5771

benesjan opened this issue Aug 20, 2024 · 7 comments · Fixed by #5959 or #6434
Labels
bug Something isn't working

Comments

@benesjan
Copy link
Contributor

benesjan commented Aug 20, 2024

Aim

I was just working on an arbitrary task in aztec-nr and my code started to fail in an e2e test because a value in a struct was suddently zero.

Expected Behavior

Struct should not get reset to "zero".

Bug

A content of a struct gets reset to zero. A bug will be obvious when running the reproduction

To Reproduce (updated on 30.9.2024)

  1. git clone https://github.com/AztecProtocol/aztec-packages.git
  2. cd aztec-packages
  3. git fetch origin janb/context-bug-reproduction && git checkout janb/context-bug-reproduction
  4. cd noir-projects/noir-contracts/contracts/test_contract
  5. nargo execute --package test_contract --silence-warnings

Once you run these commands you should get the following error:

image

To Reproduce (original occurrence)

  1. git clone https://github.com/AztecProtocol/aztec-packages.git
  2. cd aztec-packages
  3. git fetch origin dereferencing-bug && git checkout dereferencing-bug
  4. cd noir-projects/noir-contracts/contracts/token_with_refunds_contract
  5. nargo execute --package token_with_refunds_contract --silence-warnings

Once you run these commands you should get the following error:
image

Workaround

None

Workaround Description

No response

Additional Context

No response

Project Impact

Blocker

Blocker Context

It prevents merging of this PR

Nargo Version

nargo version = 0.33.0 noirc version = 0.33.0+26b87408b9360b55a59ed1589a0275dfc5ef9a02 (git version hash: 26b87408b9360b55a59ed1589a0275dfc5ef9a02, is dirty: false)

NoirJS Version

No response

Proving Backend Tooling & Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@benesjan benesjan added the bug Something isn't working label Aug 20, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Aug 20, 2024
@TomAFrench
Copy link
Member

This branch is very out of date with the current master branch of aztec-packages so migrating it across to this repository is then difficult. Can you update this so that it pulls aztec-nr from a fixed commit from https://github.com/AztecProtocol/aztec-nr?

@benesjan
Copy link
Contributor Author

@TomAFrench pointing it to a specific version in aztec-nr repo is not possible because I've modified aztec-nr to not do oracle calls. But I've just merged the branch with master. Is that enough?

@jfecher
Copy link
Contributor

jfecher commented Aug 21, 2024

When I tested this I just had to replace each std::unsafe::zeroed path with std::mem::zeroed

github-merge-queue bot pushed a commit that referenced this issue Sep 4, 2024
# Description

## Problem\*

SSA can be difficult to debug manually, particularly for very large
files. Even when comparing two sources side by side it can be hard to
find exactly where they differ since one optimization difference earlier
on can affect where ValueIds start in every function afterward.

## Summary\*

This PR adds a pass to normalize ids in an SSA program - restarting from
v0 after every SSA pass instead of continuing from the previous end. The
goal of this is to be able to take two SSA programs and easily diff them
to find out where they start differing.

E.g. using this on two files containing the final SSA from
#5771 in both failing and
passing versions, it is clear that they differ in exactly one ValueId in
one instruction.

## Additional Context

This new pass is only run when `--show-ssa` is specified, and is run
before each printout.

## Documentation\*

Check one:
- [x] 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](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <[email protected]>
@jfecher
Copy link
Contributor

jfecher commented Sep 5, 2024

We had some breaking changes recently with to_be_bytes getting its argument removed. I fixed up the uses there and pushed an update to the dereferencing-bug branch but now with a more recent version of the compiler I'm getting a different error.

Originally after I made the changes I was getting a Could not resolve some references to the array. All references must be resolved at compile time error, although after changing some of the code I'm no longer getting this and can't replicate it. Now I'm getting a panic:

The application panicked (crashed).
Message:  internal error: entered unreachable code: No size for slice v701 = Instruction { instruction: Id(990), position: 0, typ: Reference(Reference(Array([Numeric(NativeField), Numeric(Unsigned { bit_size: 32 }), Numeric(NativeField)], 16))) }
Location: compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs:158

Seems to occur with or without --show-ssa so it is probably not a result of the new normalize values pass.

github-merge-queue bot pushed a commit that referenced this issue Sep 6, 2024
…load (#5959)

# Description

## Problem\*

Resolves #5771 

This was a bug found on
AztecProtocol/aztec-packages#8378 from
#5935. However it looks to
inadvertently fix the linked issue as well.

## Summary\*

We were just directly inserting a new expression and alias for a load
result. This was overriding whatever expression of AliasSet may have
been there before. This PR switches to checking whether the result
already has an expression, which if it does to use that. It then checks
whether the result already has an alias set, if it does we add to the
alias set rather than overriding it.

## Additional Context



## Documentation\*

Check one:
- [X] 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](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Sep 6, 2024
@benesjan
Copy link
Contributor Author

benesjan commented Sep 9, 2024

@vezenovm @jfecher I have bad news. Rebased my PR on master which has Noir with Maxim's commit and the issue just occurs in a different place. Do you want me to create another reproduction?

@jfecher
Copy link
Contributor

jfecher commented Sep 9, 2024

@benesjan Ah, yes I was worried about that 😅. That'd be great if you could, thank you

@benesjan benesjan reopened this Sep 9, 2024
@benesjan
Copy link
Contributor Author

benesjan commented Sep 30, 2024

@jfecher finally got to this. I updated the reproduction description above (janb/context-bug-reproduction branch).

The behavior was a bit different this time. Now it didn't nuke the whole context but instead it just reset the side_effects_counter to 0. This should never happen as the counter is always just incremented. I would say it's the same bug because it occurred in a similar situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
3 participants