Stream: git-wasmtime

Topic: wasmtime / PR #8972 CLIF in explorer

Wasmtime GitHub notifications bot (Jul 18 2024 at 15:50):

vulc41n opened PR #8972 from vulc41n:main to bytecodealliance:main:

As discussed in #8626, I added a new pane to the explorer to see the CLIF emitted.

It creates a temporary directory next to the html file, and discards it when it's done.

Feel free to criticize as it is my first contribution on this project :smile:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jul 18 2024 at 15:50):

vulc41n requested alexcrichton for a review on PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 15:50):

vulc41n requested wasmtime-core-reviewers for a review on PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 16:03):

vulc41n updated PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 16:39):

alexcrichton requested fitzgen for a review on PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen submitted PR review:

Thanks! This looks really good! Just a few nitpicks below before we merge this.

I'm excited to get a view of the CLIF alongside the Wasm and the asm!

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen submitted PR review:

Thanks! This looks really good! Just a few nitpicks below before we merge this.

I'm excited to get a view of the CLIF alongside the Wasm and the asm!

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen created PR review comment:

I don't think we want to apply this CSS all the time, only if we have CLIF to view. If we are doing wasmtime explore but using Winch instead of Cranelift, there won't be any CLIF to display, so it doesn't make sense to reserve a third of the screen for it.

I think we can resolve this by using flexbox instead of width percents. That way, if there are three things inside the container, they will each be a third of the width, and if there are two, they will each be half the width.

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen created PR review comment:

I think we also want to scroll the relevant assembly into view as well, no?

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen created PR review comment:

And for what it's worth, we overwrite the html file.

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen created PR review comment:

I think we can just overwrite the existing files, otherwise doing wasmtime explore in the same location twice will be really annoying, since we'd have to remove a bunch of stuff before we could run the command again.

Wasmtime GitHub notifications bot (Jul 18 2024 at 17:23):

fitzgen created PR review comment:

Oh, actually, let's just use a temp directory for this. We already use the tempfile crate as a dev-dependency for the wasmtime crate, so I think we can also add that as a normal, but optional, dependency and then turn the dependency on when the explore cargo feature is enabled.

Wasmtime GitHub notifications bot (Jul 18 2024 at 20:24):

vulc41n submitted PR review.

Wasmtime GitHub notifications bot (Jul 18 2024 at 20:24):

vulc41n created PR review comment:

I think we can just overwrite the existing files, otherwise doing wasmtime explore in the same location twice will be really annoying, since we'd have to remove a bunch of stuff before we could run the command again.

Actually, it removes the directory afterwards, that's why I thought it was unsafe to delete an existing directory. I will change to use the temp directory

Wasmtime GitHub notifications bot (Jul 18 2024 at 20:34):

vulc41n updated PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 20:34):

vulc41n requested elliottt for a review on PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 20:34):

vulc41n requested wasmtime-default-reviewers for a review on PR #8972.

Wasmtime GitHub notifications bot (Jul 18 2024 at 20:44):

vulc41n commented on PR #8972:

Thank you for taking the time to review my changes, I hope the code is fine now :smile:

Wasmtime GitHub notifications bot (Jul 19 2024 at 18:58):

fitzgen submitted PR review:

This is looking really good! I tried running it locally, and when using Cranelift, this is super awesome. However, when using Winch, the explorer no longer works:

$ cargo run -- explore ~/scratch/hello.wasm -C compiler=winch
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.20s
     Running `target/debug/wasmtime explore /home/nick/scratch/hello.wasm -C compiler=winch`
Error: clif output not supported

Could you make sure this doesn't break Winch support in wasmtime explore before we land this? Thanks!!

Wasmtime GitHub notifications bot (Jul 19 2024 at 18:58):

fitzgen created PR review comment:

Let's make this an optional dependency and enable it when the explore cargo feature is enabled.

tempfile = { workspace = true }

[features]
# ...
explore = ["dep:wasmtime-explorer", "dep:tempfile"]
#...

Wasmtime GitHub notifications bot (Jul 19 2024 at 18:58):

fitzgen submitted PR review:

This is looking really good! I tried running it locally, and when using Cranelift, this is super awesome. However, when using Winch, the explorer no longer works:

$ cargo run -- explore ~/scratch/hello.wasm -C compiler=winch
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.20s
     Running `target/debug/wasmtime explore /home/nick/scratch/hello.wasm -C compiler=winch`
Error: clif output not supported

Could you make sure this doesn't break Winch support in wasmtime explore before we land this? Thanks!!

Wasmtime GitHub notifications bot (Jul 19 2024 at 18:59):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Jul 22 2024 at 07:43):

vulc41n updated PR #8972.

Wasmtime GitHub notifications bot (Jul 22 2024 at 07:50):

vulc41n commented on PR #8972:

The dependency is now optional.

I'm not sure how to deal with winch. Should I display 2 panes when compiling with winch or add clif emission support to winch ?

Wasmtime GitHub notifications bot (Jul 22 2024 at 16:31):

fitzgen commented on PR #8972:

@vulc41n the latter: Winch doesn't have an intermediate IR like CLIF, it just goes directly from Wasm to assembly by design. So when using Winch, and we don't have CLIF available, we want to show the current view, and skip the CLIF pane in the explore UI. This is why we wanted to move to flex css, so that the panes would automatically fill the whole container width regardless whether we have CLIF or not.

Does that all make sense?

Wasmtime GitHub notifications bot (Jul 22 2024 at 16:32):

fitzgen edited a comment on PR #8972:

@vulc41n the former: Winch doesn't have an intermediate IR like CLIF, it just goes directly from Wasm to assembly by design. So when using Winch, and we don't have CLIF available, we want to show the current view, and skip the CLIF pane in the explore UI. This is why we wanted to move to flex css, so that the panes would automatically fill the whole container width regardless whether we have CLIF or not.

Does that all make sense?

Wasmtime GitHub notifications bot (Jul 23 2024 at 07:35):

vulc41n updated PR #8972.

Wasmtime GitHub notifications bot (Jul 23 2024 at 07:48):

vulc41n commented on PR #8972:

Thanks for the clarification, I've made the change :smile:

In the future, I hope I can help with more advanced stuff but there are a lot of concepts to grasp

Wasmtime GitHub notifications bot (Jul 23 2024 at 16:40):

fitzgen submitted PR review:

Fantastic, thanks! Appreciate your patience getting all the details sorted out here.

Wasmtime GitHub notifications bot (Jul 23 2024 at 16:41):

fitzgen commented on PR #8972:

In the future, I hope I can help with more advanced stuff but there are a lot of concepts to grasp

Is there a specific area you are interested in getting more involved with? I might be able to find something that could be a good on-ramp for you.

Wasmtime GitHub notifications bot (Jul 23 2024 at 16:58):

fitzgen merged PR #8972.

Wasmtime GitHub notifications bot (Jul 23 2024 at 17:16):

vulc41n commented on PR #8972:

In the future, I hope I can help with more advanced stuff but there are a lot of concepts to grasp

Is there a specific area you are interested in getting more involved with? I might be able to find something that could be a good on-ramp for you.

Thanks, I appreciate. I was working on a source-to-source compiler, but right now I am unemployed and I just want to complete my experience with lower-level subjects. Anything about cranelift or winch would interest me :grinning:

Wasmtime GitHub notifications bot (Jul 23 2024 at 18:12):

fitzgen commented on PR #8972:

@vulc41n, helping add aarch64 support to Winch could be a good fit for you: https://github.com/bytecodealliance/wasmtime/issues/8321

Each instruction is a relatively small amount of work but gets you more familiar with Wasm, aarch64, and Winch. Winch is simpler and easier to get up and running on than Cranelift. And there are a lot of instructions available for filling out!

Wasmtime GitHub notifications bot (Jul 23 2024 at 18:21):

vulc41n commented on PR #8972:

Ok, I will do this :grinning:

Wasmtime GitHub notifications bot (Jul 23 2024 at 18:28):

fitzgen commented on PR #8972:

Feel free to ping me on github or zulip if you have any questions!

Last updated: Nov 22 2024 at 16:03 UTC