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

[StackIR] Support source maps and DWARF with StackIR #6564

Merged
merged 5 commits into from
May 1, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Apr 29, 2024

Helping #6509, this fixes debugging support for StackIR, which makes it more
possible to use StackIR in more places.

The fix is basically just to pass around some more state, and then to call the
parent with "please write debug info" at the correct times, mirroring the
similar calls in BinaryenIRWriter.

The relevant Emscripten tests pass, and the source map test modified
here produces identical output in StackIR and non-StackIR modes.

Remove --new-wat-parser in the test here - @tlively we don't need that
anymore do we?

@kripken kripken requested a review from tlively April 29, 2024 23:37
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Code LGTM, so LGTM overall if the test changes make sense.

@@ -2303,7 +2303,7 @@ Contains section .debug_info (851 bytes)
Contains section .debug_loc (1073 bytes)
Contains section .debug_ranges (88 bytes)
Contains section .debug_abbrev (333 bytes)
Contains section .debug_line (2682 bytes)
Contains section .debug_line (2642 bytes)
Copy link
Member

Choose a reason for hiding this comment

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

Is this expected and accounted for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so, yes. StackIR saves a few bytes here and there, which allows encoded offsets to be a bit shorter in some cases. Any increase in size (in this or any other field) would have been very suspicious, however. A large decrease would also have been odd.

I did also run llvm-dwarfdump --validate on the output and it is equally valid as before this PR (which is, 99% valid but for one known pre-existing minor issue).

@kripken
Copy link
Member Author

kripken commented Apr 30, 2024

@tlively what about this question?

Remove --new-wat-parser in the test here - @tlively we don't need that anymore do we?

(if we actually don't then I can simplify the test a little more I think)

@tlively
Copy link
Member

tlively commented Apr 30, 2024

Oh yes, sorry. --new-wat-parser can be removed.

@kripken
Copy link
Member Author

kripken commented Apr 30, 2024

Thanks, then I simplified the test now to have autogenerated CHECKs, since the checks are identical between StackIR and BinaryenIR (and without two parsers, there is just one output for BinaryenIR).

@kripken kripken merged commit 7d9e4a8 into WebAssembly:main May 1, 2024
13 checks passed
@kripken kripken deleted the stackir.sourcemap branch May 1, 2024 15:26
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

2 participants