Stream: git-wasmtime

Topic: wasmtime / PR #1436 Parse run commands

Wasmtime GitHub notifications bot (Mar 30 2020 at 19:59):

abrown opened PR #1436 from parse-run-commands to master:

This PR adds the ability to parse CLIF run commands like run: %fn0(42, 4.2) == true. It seems like a good first step towards closing out https://github.com/bytecodealliance/cranelift/pull/1231; it also factors out some of the functionality that was implemented separately in https://github.com/bytecodealliance/wasmtime/pull/1223 and incorporates it into cranelift-reader.

My plan is to use this parsing in FunctionRunner to:

• compare the results of running CLIF functions to values other than true (the current hard-coded behavior); this would be useful in testing
• print the results of running a function in FunctionRunner with, e.g., print: %fn(42) (this was the original point of https://github.com/bytecodealliance/cranelift/pull/1231)
• allow running CLIF functions with varied signatures; currently FunctionRunner can only run functions of () -> b*. To do this, I think I need to do something along the lines of WasmTy or Val in wasmtime-api. @alexcrichton, what do you think? ValueData is a bit like Val and I will need to cast the function compiled by FunctionRunner to types known at runtime.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 30 2020 at 19:59):

abrown requested nbp for a review on PR #1436.

Wasmtime GitHub notifications bot (Mar 30 2020 at 20:00):

abrown requested alexcrichton and nbp for a review on PR #1436.

Wasmtime GitHub notifications bot (Mar 30 2020 at 20:39):

abrown updated PR #1436 from parse-run-commands to master:

This PR adds the ability to parse CLIF run commands like run: %fn0(42, 4.2) == true. It seems like a good first step towards closing out https://github.com/bytecodealliance/cranelift/pull/1231; it also factors out some of the functionality that was implemented separately in https://github.com/bytecodealliance/wasmtime/pull/1223 and incorporates it into cranelift-reader.

My plan is to use this parsing in FunctionRunner to:

• compare the results of running CLIF functions to values other than true (the current hard-coded behavior); this would be useful in testing
• print the results of running a function in FunctionRunner with, e.g., print: %fn(42) (this was the original point of https://github.com/bytecodealliance/cranelift/pull/1231)
• allow running CLIF functions with varied signatures; currently FunctionRunner can only run functions of () -> b*. To do this, I think I need to do something along the lines of WasmTy or Val in wasmtime-api. @alexcrichton, what do you think? ValueData is a bit like Val and I will need to cast the function compiled by FunctionRunner to types known at runtime.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 30 2020 at 21:48):

abrown updated PR #1436 from parse-run-commands to master:

This PR adds the ability to parse CLIF run commands like run: %fn0(42, 4.2) == true. It seems like a good first step towards closing out https://github.com/bytecodealliance/cranelift/pull/1231; it also factors out some of the functionality that was implemented separately in https://github.com/bytecodealliance/wasmtime/pull/1223 and incorporates it into cranelift-reader.

My plan is to use this parsing in FunctionRunner to:

• compare the results of running CLIF functions to values other than true (the current hard-coded behavior); this would be useful in testing
• print the results of running a function in FunctionRunner with, e.g., print: %fn(42) (this was the original point of https://github.com/bytecodealliance/cranelift/pull/1231)
• allow running CLIF functions with varied signatures; currently FunctionRunner can only run functions of () -> b*. To do this, I think I need to do something along the lines of WasmTy or Val in wasmtime-api. @alexcrichton, what do you think? ValueData is a bit like Val and I will need to cast the function compiled by FunctionRunner to types known at runtime.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 31 2020 at 18:09):

abrown requested alexcrichton, nbp and bnjbvr for a review on PR #1436.

Wasmtime GitHub notifications bot (Apr 03 2020 at 17:22):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 17:22):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 17:22):

bnjbvr created PR Review Comment:

The 8 should be a 16 here, since we want to parse as hexadecimal, right? (if i'm correct, can you add a test case for this, please?)

Also, why do we parse it as a u16? Would this allow numbers above the imm8 range?

Wasmtime GitHub notifications bot (Apr 03 2020 at 17:22):

bnjbvr created PR Review Comment:

nit: can you do let value = if text.starts_with... {} else {}; instead? (avoids the mut qualifier on value)

Wasmtime GitHub notifications bot (Apr 03 2020 at 17:22):

bnjbvr created PR Review Comment:

Isn't this possibly inferred by the compiler for Vec<T> when T implements Display? Or can we use {:?} for the Vec<T> type and it'll do the right thing here?

Wasmtime GitHub notifications bot (Apr 03 2020 at 17:22):

bnjbvr created PR Review Comment:

nit: I think you can remove both the Ok and ? here, right?

Wasmtime GitHub notifications bot (Apr 03 2020 at 18:49):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 18:49):

abrown created PR Review Comment:

Yes, good point!

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:08):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:08):

abrown created PR Review Comment:

I think this was introduced in 2300eec8 so I went ahead and took the same let value = ... approach for i16 and i32.

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:08):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:08):

abrown created PR Review Comment:

Yeah, and similarly removed this in i16 and i32.

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:14):

abrown updated PR #1436 from parse-run-commands to master:

This PR adds the ability to parse CLIF run commands like run: %fn0(42, 4.2) == true. It seems like a good first step towards closing out https://github.com/bytecodealliance/cranelift/pull/1231; it also factors out some of the functionality that was implemented separately in https://github.com/bytecodealliance/wasmtime/pull/1223 and incorporates it into cranelift-reader.

My plan is to use this parsing in FunctionRunner to:

• compare the results of running CLIF functions to values other than true (the current hard-coded behavior); this would be useful in testing
• print the results of running a function in FunctionRunner with, e.g., print: %fn(42) (this was the original point of https://github.com/bytecodealliance/cranelift/pull/1231)
• allow running CLIF functions with varied signatures; currently FunctionRunner can only run functions of () -> b*. To do this, I think I need to do something along the lines of WasmTy or Val in wasmtime-api. @alexcrichton, what do you think? ValueData is a bit like Val and I will need to cast the function compiled by FunctionRunner to types known at runtime.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:14):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:14):

abrown created PR Review Comment:

This is fixed: u8::from_str_radix(&text[2..], 16)

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:28):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:28):

abrown created PR Review Comment:

It complained that I hadn't implemented Debug for T, IIRC. I think this boils down to: how do I use the Display of T as a Debug so that I can use the Debug of Vec<T>? Preferably without adding Debug to the whole type hierarchy of T... I don't yet see a good Rust-y way of doing this.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:24):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:24):

abrown created PR Review Comment:

@alexcrichton suggested I use impl Display instead of DataValue, which is a good solution to avoid this function; but I remembered that I made this because sometimes I need the data values enclosed in parentheses and sometimes in brackets. E.g. %fn() = [1, 2, 3] vs %fn(1, 2, 3) == 42. I'm going to leave this function in for that reason.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:25):

abrown merged PR #1436.

Last updated: Dec 23 2024 at 12:05 UTC