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

Debugger panic when assessing complex slice #4967

Closed
vezenovm opened this issue May 2, 2024 · 1 comment · Fixed by #4986
Closed

Debugger panic when assessing complex slice #4967

vezenovm opened this issue May 2, 2024 · 1 comment · Fixed by #4986
Labels
bug Something isn't working

Comments

@vezenovm
Copy link
Contributor

vezenovm commented May 2, 2024

Aim

I want to run the debugger on this program:

fn main() {
    let var1: [(i32, u8)] = [(1, 2)];
    let var2: [(i32, u8)] = var1;
}

@ggiraldez @mverzilli

Expected Behavior

I expected the debugger to complete successfully.

Bug

I get this output:

The application panicked (crashed).
Message:  called `Option::unwrap()` on a `None` value
Location: /mnt/user-data/maxim/noir/compiler/noirc_printable_type/src/lib.rs:316

To Reproduce

  1. Make a program with the main posted above
  2. nargo debug
  3. continue

Project Impact

Nice-to-have

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@vezenovm vezenovm added the bug Something isn't working label May 2, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 2, 2024
@ggiraldez
Copy link
Contributor

This issue manifests with the debugger, but it's a problem with the array to slice coercion intrinsic. When simplifying the coercion, the code does not take into account that for SSA, complex types such as tuples and structs are flattened, resulting in an array representation with length of elements times the size of the complex type (as a result of the flattening). This bug is easily reproducible with this code, which fails the assertion:

fn main() {
    let var1: [(i32, u8)] = [(1, 2)];
    assert(var1.len() == 1);
}

Then, when trying to decodes the printable value from the type, the foreign function gets the wrong length of the slice, and attempts to decode more elements than what's available. This is also reproducible with printing the value:

fn main() {
    let var1: [(i32, u8)] = [(1, 2)];
    dep::std::println(var1);
}

The debugger uses the same decoding machinery as println, so this crashes in the same way as described in the OP.

I will submit a PR in a moment.

github-merge-queue bot pushed a commit that referenced this issue May 6, 2024
…ray of complex types (#4986)

# Description

## Problem\*

Resolves #4967

## Summary\*

There is a problem in the [array to slice
coercion](https://github.com/noir-lang/noir/blob/07930d4373a393146210efae69e6ec40171f047b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs#L87)
code. When evaluating the coercion, it does not take into account that
for SSA, complex types such as tuples and structs are flattened,
resulting in an array representation with length of elements times the
size of the complex type (as a result of the flattening). This results
in the replacement slice having an incorrect size.

Then, because the slice has an inflated size, the code which decodes the
elements of the slice for printing or tracking (in the debugger) crashes
when it attempts to decode beyond the values that it has available.

## Additional Context

There are two additional minor changes introduced in this PR:

1. Removed the `Optional<>` for `PrintableType::Array`'s length. This
was introduced to be able to print slices (setting the length to
`None`), but those are now represented properly with
`PrintableType::Slice`.
2. Added support for printing slices.

Both are pretty small changes which I made while investigating the
issue, but I'm willing to send separate PRs for those if required.

I also expanded the existing test cases to verify both the coerced slice
length and printing of slices.

## 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 May 6, 2024
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
2 participants