-
Notifications
You must be signed in to change notification settings - Fork 54
test: Add test for debug locations #810
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
Conversation
|
Similar situation is when an |
Interesting. I see we set the passed
I wonder how it ends up being lost. Or do we pass an empty span here? Any ideas? EDIT: Oh, I'm just slow. #812 (comment) fixes it, right? |
|
I double checked now, without changes from #812 (comment) (so basically with this branch only), we have this situation: As you can see Here is the fix 0xMiden/miden-vm#2449. So, only with this fix (we can completely forget about #812 for now), we see: But, for the test I added here, the |
|
So we have source location for the panic function call but no source code for the unreachable op inside it. I wonder if miden-debug could go up the stack and find the first source span. But it would not help when we're running in the VM through the miden-client. So whatever solution we find needs to be implemented in the VM. |
|
@djolertrk good catch! Great job! |
|
I think we should also print the source info in the call stack trace for each frame where it is known. @bitwalker Do we have a call stack trace or plans for it in the fast processor VM? |
71b06b2 to
b9ac3b7
Compare
42033a4 to
fd57be2
Compare
greenhat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thank you! Please check my comments.
tests/integration/src/rust_masm_tests/debug_source_locations.rs
Outdated
Show resolved
Hide resolved
| &[ | ||
| "--debug", | ||
| "full", | ||
| &format!("-Ztrim-path-prefix={example_path_str}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make cargo-miden use the -Ztrim-path-prefix option. I mean the test should call cargo_miden::run rather than use cargo with midenc.
The cargo-miden is the advised way of compiling the Miden package. We're not advertising the cargo -> midenc workflow that much, leaving it for people who want to tinker with the compiler. So cargo-miden should produce a Miden package with proper source location info.
| .unwrap() | ||
| .parent() | ||
| .unwrap() | ||
| .join("examples") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples folder is for user-facing examples, ones that might be interesting to the users. For internal test fixtures like this one the tests/rust-apps-wasm/rust-sdk/ is the best fit.
b125e5b to
66b0426
Compare
We have all the necessary information available, but we only display sources for a frame if explicitly requested in the debugger today. In the TUI that's ideal, but we may want to introduce functionality in the executor itself that lets us print the source snippet for each frame when printing an exception in our test suite (as an example). |
b31bbd4 to
ad91208
Compare
greenhat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Only cargo-miden fix left.
| [ | ||
| "--debug".to_string(), | ||
| "full".to_string(), | ||
| format!("-Ztrim-path-prefix={fixture_path_str}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #810 (comment) I meant that cargo-miden should calculate and pass -Ztrim-path-prefix under the hood without asking the user. So that user runs cargo miden build --debug full and gets the Miden package with correct source locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, thanks! It should be addressed in the latest commit.
ad91208 to
a65aa75
Compare
greenhat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Thank you! One last thing, please rebase it on the latest next branch.
a65aa75 to
145e5bc
Compare
|
@djolertrk Oh, I cannot merge it due to missing commit signatures. Please sign the commit. |
145e5bc to
16f9af2
Compare
This demonstrates that simple "assert-based debugging" does not work.
The reason is that there is no source locations attached to
ub.unreachable.It can be confirmed by using following commands:
And to run the integration test:
This addresses #806.