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

Add backtrace to forge #2679

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add backtrace to forge #2679

wants to merge 4 commits into from

Conversation

ksew1
Copy link
Collaborator

@ksew1 ksew1 commented Nov 15, 2024

Closes #2468

Introduced changes

  • Forge will display backtrace for contracts in format similar to rust

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@ksew1 ksew1 marked this pull request as draft November 15, 2024 11:39
@ksew1 ksew1 force-pushed the backtrace branch 3 times, most recently from fdd8f68 to b9666d3 Compare November 15, 2024 13:06
@ksew1
Copy link
Collaborator Author

ksew1 commented Nov 15, 2024

This was inspired by Rust backtrace, but there are a couple of things to note in terms of user experience.

When Forge encounters an error, it suggests enabling a backtrace for more context:

Failure data:
    (0x454E545259504F494E545F4E4F545F464F554E44 ('ENTRYPOINT_NOT_FOUND'), 0x454E545259504F494E545F4641494C4544 ('ENTRYPOINT_FAILED'))

note: run with `SNFORGE_BACKTRACE=1` environment variable to display a backtrace

Enabling backtrace provides a call stack:

Failure data:
    (0x454E545259504F494E545F4E4F545F464F554E44 ('ENTRYPOINT_NOT_FOUND'), 0x454E545259504F494E545F4641494C4544 ('ENTRYPOINT_FAILED'))

error occurred in contract 'InnerContract' at pc: '72'
possible stack backtrace:
   0: InnerContract::inner_call
       at src/lib.cairo:47:9
   1: InnerContract::inner
       at src/lib.cairo:38:13
   2: InnerContract::__wrapper__inner
       at src/lib.cairo:37:9

[...]

Tests: 0 passed, 1 failed, 0 skipped

However ...

  • If functions are not inlined, only the line causing the failure is shown, without the functions that call them:

    0: InnerContract::inner_call
         at src/lib.cairo:47:9
    

    The Cairo compiler inlines aggressively, so often you will get the whole call stack from the entry point, which is really helpful when debugging. But in the worst-case scenario, you only get the last line. I was thinking because of that should we always display the last line, but I believe the whole call stack is really helpful.

  • Currently, the VM does not halt immediately upon encountering a panic and do bubbles it up, so backtraces may display only the contract's entry point.

In terms of UX, if you prefer something different, we can easily change this as the backend will probably stay the same.

@ksew1
Copy link
Collaborator Author

ksew1 commented Nov 15, 2024

Also, I will add documentation in a separate PR to avoid making this one any bigger.

@ksew1 ksew1 marked this pull request as ready for review November 15, 2024 14:13
crates/forge-runner/src/backtrace.rs Outdated Show resolved Hide resolved
crates/forge/tests/data/backtrace_panic/Scarb.toml Outdated Show resolved Hide resolved
crates/forge/tests/data/backtrace_vm_error/Scarb.toml Outdated Show resolved Hide resolved
crates/forge/tests/e2e/backtrace.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/backtrace.rs Show resolved Hide resolved
crates/forge-runner/src/test_case_summary.rs Show resolved Hide resolved
Comment on lines +86 to +89
"perhaps the contract was compiled without the following entry in Scarb.toml under [profile.dev.cairo]:
unstable-add-statements-code-locations-debug-info = true

or scarb version is less than 2.8.0
Copy link
Member

Choose a reason for hiding this comment

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

This can be a follow up task: Can we explicitly check both Scarb version and toml for these keys and show to the users the things that are incorrect?

So it outputs an error like: unstable-add-statements-code-locations-debug-info = true in Scarb.toml for backtrace to work or scarb version must be at least 2.8.0 etc?

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Good job 👌

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.

Implement printing out latest executed line on failures
3 participants