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 initial support for DWARF Fission #8055

Merged
merged 74 commits into from
Apr 24, 2024

Conversation

yowl
Copy link
Contributor

@yowl yowl commented Mar 7, 2024

Wasmtime supports debugging with DWARF, and this PR adds initial support for DWARF fission. DWARF fission has advantages in linking and wasm size (https://developer.chrome.com/blog/faster-wasm-debugging) as it separates the debug_info section into a separate file leaving a DW_TAG_skeleton_unit in the main wasm file.

This PR only supports DWARF pacakges, .dwp files. It should also be possible to support.dwo files, but I've not done that here.

@yowl yowl changed the title Add initial support for Dwarf Fission Add initial support for DWARF Fission Mar 7, 2024
@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Mar 7, 2024
Copy link

github-actions bot commented Mar 7, 2024

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasi", "wasmtime:api", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've got some high level comments about the rough structure here, mostly to help centralize things bit more in the "dwarf processing area" and additionally API guidance for enabling usage of this in all case but not necessarily requiring it to be front-and-center in the embedding API.

crates/wasi-common/WASI Outdated Show resolved Hide resolved
crates/wasmtime/src/component/component.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/engine.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
crates/environ/src/module_environ.rs Outdated Show resolved Hide resolved
crates/environ/src/module_artifacts.rs Outdated Show resolved Hide resolved
crates/cranelift/src/debug/transform/mod.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/compile.rs Outdated Show resolved Hide resolved
@yowl
Copy link
Contributor Author

yowl commented Mar 17, 2024

I think I've addressed the first set of feedback and I think it looks cleaner. There are some safe-to-deploy and some feature looking errors to deal with still

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good! I wanted to offer, though, if you'd like I'm happy to help out with the refactoring required to make ModuleBuilder come into existence. That's a big chunk of this change and isn't related directly to DWARF fission, so I can help out land the refactorings required to get ModuleBuilder itself in place and that would help slim down this change to just DWARF fission.

I say this because I've got a number of comments about specific details about the API, but I don't want to bog you down too much if you're mainly interested in the DWARF bits.

crates/environ/src/module_environ.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/compile.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/engine.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/lib.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/module.rs Outdated Show resolved Hide resolved
@yowl
Copy link
Contributor Author

yowl commented Mar 18, 2024

I wanted to offer, though, if you'd like I'm happy to help out with the refactoring required to make ModuleBuilder come into existence.

That would be very kind, thanks! You are right my primary goal is DWARF fission, so I can land dotnet/runtimelab#2264 which I can't really do properly without wasmtime support.

@github-actions github-actions bot added the winch Winch issues or pull requests label Mar 18, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Apr 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 22, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Apr 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 22, 2024
@alexcrichton
Copy link
Member

@yowl this failure looks legitimate in the sense of probably-caused-by-this PR. Do you know if that perhaps means the LLDB we're using in CI is missing something and/or is too old?

@yowl
Copy link
Contributor Author

yowl commented Apr 22, 2024

This looks relevant llvm/llvm-project#55575 Seems like we have to add some missing python stuff.

@yowl
Copy link
Contributor Author

yowl commented Apr 23, 2024

Although its odd the other tests are passing. What version of Ubuntu is it? Can we run lldb -P to see what paths are expected to be present?

@alexcrichton
Copy link
Member

I'm not sure myself, we don't have anything special beyond the standard github actions runners. If you'd like you can push up a commit with "prtest:full" in the commit message and it'll run the full test suite here on the PR to help debug.

@yowl
Copy link
Contributor Author

yowl commented Apr 24, 2024

@alexcrichton , change a couple of things

Added workaround for https://bugs.launchpad.net/ubuntu/+source/llvm-defaults/+bug/1972855
Forced use of latest lldb available, 15
Added logic to CodeBuilder to load dwp when a wasm path is supplied.
Fixed inconsistency in source file names

@alexcrichton alexcrichton added this pull request to the merge queue Apr 24, 2024
Merged via the queue into bytecodealliance:main with commit fb4f4cd Apr 24, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants