Print return values of run tests if type is not boolean. #1231

krk · 2019-11-14T22:30:15Z

If return value of a run test is not boolean, an error is created with
the message "Function return type is not boolean, return value is {}".

This was very briefly discussed on gitter with @sunfishcode.
This PR provides a way to introspect into the run test variable values, helpful while writing tests and debugging.

abrown · 2019-11-14T23:34:24Z

Good idea; I've needed something like this in the past. @nbp and I had talked about extending the ; run syntax indicating that a function is supposed to be run with something like run: %fn(3, 5) == 42. With multiple lines we could then test different arguments to a function and verify their results, presumably then printing out errors like expected 42, found 99. Is there any way this could implement something towards that direction?

abrown · 2019-11-14T23:49:03Z

For context: #872

krk · 2019-11-15T08:56:55Z

I have initially suggested adding a new opcode for printing the argument similar to what dbg! does in rust, @sunfishcode suggested that it would be too much work.

If the intention is to print results of comparisons instead of a single value, maybe the function can return a multilane value and we could print the data as two values that are half width of the return type - which would be a workaround.

Extending the run comment run: %fn(3, 5) == 42 to parse expressions and print both values brings the possibility of inventing a new language that could have more features like other operators, other types of functions etc. I think it would be overly complicated for me to implement at this stage.

On the other hand, one might say that a new ; print "value 1", v1 type of comment command would seem idiomatic w.r.t. the SSA expressions and variable names used in the code. If this is relatively easier to implement than a new dbg! opcode, I can try to implement that. Optionally, result of this ;print command could be available to check commands, if that makes sense.

abrown · 2019-11-15T17:06:46Z

I think it would be overly complicated for me to implement at this stage

Makes sense, let's just go with the simple print idea then. I suspect that implementing ; print "value 1", v1 might be just as tricky as adding a new print instruction (we would have to mess around inside the function). To keep it simpler and still move towards the comparison idea, could we:

keep ; run with its current logic of only running boolean-returning functions and testing their result (and eventually move this to ; run: %fn(2) > 42)--optionally we could rename ; run to ; run: test for consistency
add ; run: print which would use much of your current code to run a function and then print out the results if it can

What do you think of that?

krk · 2019-11-16T17:43:18Z

Would it mean that ; run: print would make the tests fail always, as it is the case currently?

krk · 2019-11-16T21:53:13Z

Sure thing, it makes sense to modify the run comment now.

I have renamed ;run to ;run: test and added print and assertion commands.
Assertion only supports equality and inequality: ; run: %fn() == 42 and ; run: %fn() != 0x2a.
Printing always fails the test and prints the return value: ; run: print.

I do not like my parse_options function, is there an idiomatic way to refactor it into?

abrown · 2019-11-17T03:57:09Z

Nice work! I'll take a closer look on Monday.

abrown

Thanks for doing this! See my comments for some improvements but I think this approach makes sense. I will use these new features for sure 😄. If anything I said didn't make sense I'll be on the Mozilla IRC if you want to discuss.

abrown · 2019-11-18T17:36:18Z

cranelift-filetests/src/function_runner.rs

+    args: Vec<u128>,
+    operator: Option<Operator>,
+    expected: Option<u128>,
+}


To be more clear about what this is used for (and to make construction simpler), we could do:

enum FunctionRunnerAction { Print, Test, TestWithResult(operator: Operator, expected: ...) }

And I don't see that args is used (yet 😄) so we could leave it out for now.

abrown · 2019-11-18T17:47:33Z

cranelift-filetests/src/function_runner.rs

+        Self {
+            function,
+            isa,
+            options: Self::parse_options(comment),


It occurred to me that it might be better to not pass comment in through a bunch of functions: we might want to pass in an already parsed Options/FunctionRunnerAction in some cases and I see below some cases where we are forced to provide a comment string when we really don't need to. What if new just put a default value here (like the original behavior FunctionRunnerAction::Test) and then we had a function like:

fn with_action(self, action: FunctionRunnerAction) -> Self { self.action = action; self }

If users, like test_run.rs, want to override the default action they could do something roughly like FunctionRunner::new(...).with_action(parse(comment)).

I have introduced a builder FunctionRunnerBuilder to unify it with other with functions.

cranelift-filetests/src/function_runner.rs

abrown · 2019-11-18T18:02:34Z

cranelift-filetests/src/function_runner.rs

+                    "Function arguments more than supported count: {}",
+                    self.options.args.len()
+                )),
+            },


This was a bit hard to read but I think I get it now; it might be more clear with an enum like I mentioned above:

match action { Print => println!(...), Test => check(...), TestWithResult => check(...), }

This simplified things a lot!

bnjbvr

Just wanted to add: could we keep the previous behavior that ; run is equivalent to ; run: test, so we don't have to modify all the existing test cases, and remove ; run: test too? (or find a better sub-directive name after the :, since "test" is quite overloaded in the context of testing)

To print the return value of a run test, use "; run print", to assert on the return value, use "; run %fn() == 42".

abrown · 2019-11-19T17:59:22Z

cranelift-codegen/src/result.rs

    /// This always represents a bug, either in the code that generated IR for Cranelift, or a bug
    /// in Cranelift itself.
-    #[error("Verifier errors")]
+    #[error("Verifier errors\n{0}")]


I don't think we want to do this (I just learned): #1240

abrown · 2019-11-20T19:00:26Z

cranelift-filetests/src/function_runner.rs

+        match s {
+            "==" => Ok(Operator::EQ),
+            "!=" => Ok(Operator::NEQ),
+            _ => Err(format!("Unsupported operator {}", s)),


A minor re-factoring suggestion: elsewhere in cranelift I've seen this type of thing as impl FromStr for Operator { ... } so that we can just call .parse() up on line 320. Not a big deal but it keeps the operator implementation together.

abrown · 2019-11-20T19:01:09Z

cranelift-filetests/src/function_runner.rs

+    /// Build a function runner from an action, a function and the ISA to run on.
+    pub fn finish(self) -> FunctionRunner {
+        FunctionRunner::new(self.function, self.isa.unwrap(), self.action)
+    }


Nit: elsewhere in the project we are calling this build.

cranelift-codegen/src/isa/mod.rs calls it finish.

cranelift/cranelift-codegen/src/isa/mod.rs

Line 151 in da996fe

pub fn finish(self, shared_flags: settings::Flags) -> Box<dyn TargetIsa> {

Yeah, in this particular case it's this file which is not consistent with the Rust style, so we should fix it (later).

abrown · 2019-11-20T19:08:10Z

cranelift-filetests/src/function_runner.rs

+        match &self.action {
+            FunctionRunnerAction::Test => {
+                if !ret_value_type.is_bool() {
+                    return Err(String::from("Function return type is not boolean"));


Let's add the context.func.name here as well so that we can more easily find it.

abrown · 2019-11-20T19:09:55Z

cranelift-filetests/src/function_runner.rs

+                if result {
+                    return Ok(());
+                }
+                return Err(format!("Failed: {}", context.func.name.to_string()));


Re-factoring nit: I'm not really used to seeing return much in Rust 😄:

if Self::invoke::<bool>(&code_page) { Ok(()) } else { Err(format!("Failed: {}", context.func.name.to_string())) }

abrown · 2019-11-20T19:11:51Z

cranelift-filetests/src/function_runner.rs

+                Operator::EQ,
+                ConstantData::default().append(1),
+                true,
+            ),


Perhaps we should not overload check; why not just invoke here and println! the returned value?

abrown · 2019-11-20T19:34:38Z

cranelift-codegen/src/ir/constant.rs

+        Ok(self.into_vec()[0] != 0)
+    }
+}
+


I don't think we should modify ConstantData for what we are doing; it is designed for constant pool stuff and adding these conversions might make it harder to decouple later from test functionality. Why not build our own Value enum:

enum Value { Bool(bool), F32(f32), F64(f64), ... }

I wouldn't worry too much about 128-bit types just yet until we get the basics in place. Then I would add a way to parse into this (using Rust's FromStr/parse) from text and a type:

impl Value { fn parse_as(text: &str, type: Type) -> Result<Self, ...> { match type { B8 | B16 | ... => Ok(Value::Bool(text.parse()?)), F32 => Ok(Value::F32(text.parse()?)), ... }

Then I would #[derive(Eq, PartialEq)] on the enum so we can compare them to each other and add some From implementations so that invoke can get the raw value (e.g. f32), wrap it in a Value with Value::from (or into), and then compare it against the parsed Values that we get from the comments. (Hopefully that explains the other comments I've made; this isn't the only way to do things but it doesn't involve touching other components like ConstantData and it seems natural to Rust--from the little I know about Rust).

Let me change my tune a bit: I now see down below that you implemented some parsing logic in parser.rs; if you want to use that for the FromStr on Value instead of the built-in Rust parsing that seems OK. We could use methods like Parser::new(text).match_bool(...) instead and Value may have to use different inner types (e.g. F32(Ieee32) instead of F32(f32))--but that seems fine. The underlying thing that I'm getting at is that we likely don't want to touch ConstantData in this case.

abrown · 2019-11-20T19:37:19Z

cranelift-reader/src/parser.rs

+        };
+        Ok(constant_data)
+    }
+


See my parsing comments about Value; we may not need to change the parser.

docs/testing.rst

filetests/function_runner/types.clif

abrown

Looks good to me; nice work. We could probably simplify parsing more but we can do that when we finish out the %fn(42) == 2 syntax (do we have an issue for that, BTW?). @bnjbvr, I'm good to merge but giving you a chance to take a look. In the meantime, @krk, any idea why CI is failing?

abrown · 2019-11-23T04:12:03Z

(I will be out next week on vacation and it looks like @bnjbvr may be as well so it might not get merged until after Thanskgiving week).

krk · 2019-12-30T12:02:37Z

ping @abrown @bnjbvr @bjorn3

bnjbvr · 2020-01-06T14:03:35Z

I'm back from vacation, I'll try to take a look at it this week.

fitzgen

Hi @krk -- thanks for this PR!

I'm not super familiar with this area of code, but since Benjamin tagged me for review (I assume because he's got a lot on his plate at the moment) I've taken a look and left a few comments.

Overall, this looks good, but I think there are a few things we could clean up a bit before landing, and it seems like there are still some unresolved discussions from earlier rounds of review that we should figure out before merging this as well.

If any of my comments are unclear, please let me know! I'm happy to clarify.

Cheers!

fitzgen · 2020-01-06T20:39:39Z

cranelift-filetests/src/function_runner.rs

+        let mut parts: Vec<&str> = comment.split_ascii_whitespace().collect();
+
+        if parts.len() < 3 {
+            panic!("Invalid syntax for test assertion, expected \"%fn() <operator> <value>\", found \"{}\"", comment);


We shouldn't panic on parse errors, we should return a fallible ParseResult like other parsing functions do.

Additionally, it seems a little strange that we have this parsing code in the cranelift-filetests crate, when AFAICT all the other parsing code is in the cranelift-reader crate. Unless there is a good reason not to, we should match existing conventions and keep the parsing code inside cranelift-reader.

fitzgen · 2020-01-06T20:41:09Z

cranelift-reader/src/parser.rs

-    // Match and consume either a hexadecimal Uimm128 immediate (e.g. 0x000102...) or its literal list form (e.g. [0 1 2...])
-    fn match_constant_data(&mut self, controlling_type: Type) -> ParseResult<ConstantData> {
+    /// Match and consume either a hexadecimal Uimm128 immediate (e.g. 0x000102...) or its literal list form (e.g. [0 1 2...])
+    pub fn match_constant_data(&mut self, controlling_type: Type) -> ParseResult<ConstantData> {


I believe that moving that parsing code into this cranelift-reader crate would let us avoid making all these methods public as well.

fitzgen · 2020-01-06T20:43:20Z

cranelift-filetests/src/function_runner.rs

+                Self::I8(data.into())
+            }
+            types::I16 => {
+                let data = p.match_constant_data(ty).unwrap();


Was this still going to happen?

fitzgen · 2020-01-06T20:43:46Z

cranelift-filetests/src/function_runner.rs

+            types::I128 => {
+                let data = p.match_constant_data(ty).unwrap();
+                let data = ConstantDataU128::try_into(data).unwrap();
+                Self::I128(data.rotate_left(64))


It isn't clear to me what the resolution is here, if any.

fitzgen · 2020-01-06T20:53:42Z

cranelift-filetests/src/function_runner.rs

+    ($type: ty, $name: ident) => {
+        struct $name {}
+        impl $name {
+            fn try_into(d: ConstantData) -> Result<$type, String> {


Instead of making an ad-hoc try_into method, we should follow convention and implement the TryFrom trait, which gives us a try_into via the blanket impl TryInto<T> for U where U: TryFrom<T>. For details, see: https://doc.rust-lang.org/stable/std/convert/trait.TryInto.html and https://doc.rust-lang.org/stable/std/convert/trait.TryFrom.html

If I'm reading this code right, it would be something like this:

struct $name(ConstantData); impl TryFrom<$name> for $type { // ... }

But this also makes me wonder why we don't implement TryFrom<ConstantData> for $type directly instead of creating all these temporary ConstantData$Type types. Is that something that you explored at all? I think it would be the best course to take, unless it doesn't work for some reason or I am overlooking something else.

alexcrichton · 2020-02-28T23:35:49Z

Thanks for the PR again, and as a procedural note the Cranelift repository has now merged into the wasmtime repository.

PRs are no longer landing in this repository, and unfortunately there's no "one button" solution to move a PR to the wasmtime repository. A script has been prepared, however, to assist you in transferring this PR to the wasmtime repo. Feel free to reach out on Zulip with any questions!

krk · 2020-04-04T20:56:41Z

Sorry for abandoning this PR.

It seems that functionality is being added: bytecodealliance/wasmtime#1223

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and bytecodealliance#1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

@alexcrichton

This resolves the work started in bytecodealliance/cranelift#1231 and #1436. Cranelift filetests currently have the ability to run CLIF functions with a signature like `() -> b*` and check that the result is true under the `test run` directive. This PR adds the ability to call functions with arbitrary arguments and non-boolean returns and either print the result or check against a list of expected results: - `run` commands look like `; run: %add(2, 2) == 4` or `; run: %add(2, 2) != 5` and verify that the executed CLIF function returns the expected value - `print` commands look like `; print: %add(2, 2)` and print the result of the function to stdout To make this work, this PR compiles a single Cranelift `Function` into a `CompiledFunction` using a `SingleFunctionCompiler`. Because we will not know the signature of the function until runtime, we use a `Trampoline` to place the values in the appropriate location for the calling convention; this should look a lot like what @alexcrichton is doing with `VMTrampoline` in wasmtime (see https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/api/src/func.rs#L510-L526, https://github.com/bytecodealliance/wasmtime/blob/3b7cb6ee64469470fcdd68e185abca8eb2a1b20a/crates/jit/src/compiler.rs#L260). To avoid re-compiling `Trampoline`s for the same function signatures, `Trampoline`s are cached in the `SingleFunctionCompiler`.

krk force-pushed the print-run-retval branch from da67801 to 7312d56 Compare November 16, 2019 21:47

krk force-pushed the print-run-retval branch from 7312d56 to cffae1e Compare November 16, 2019 21:55

abrown self-requested a review November 17, 2019 03:56

abrown reviewed Nov 18, 2019

View reviewed changes

bnjbvr reviewed Nov 19, 2019

View reviewed changes

krk force-pushed the print-run-retval branch 2 times, most recently from 730bd3d to 72f6ea8 Compare November 19, 2019 15:05

Add print and assertion options to the run tests.

18afb26

To print the return value of a run test, use "; run print", to assert on the return value, use "; run %fn() == 42".

krk force-pushed the print-run-retval branch from 72f6ea8 to 18afb26 Compare November 19, 2019 15:09

abrown reviewed Nov 19, 2019

View reviewed changes

Revert verifier errors format string.

b5cc84a

abrown reviewed Nov 20, 2019

View reviewed changes

docs/testing.rst Show resolved Hide resolved

abrown reviewed Nov 20, 2019

View reviewed changes

filetests/function_runner/types.clif Show resolved Hide resolved

krk added 5 commits November 21, 2019 21:22

Implement FromStr for Operator.

892a6b0

Add function name to errors.

5e78a80

Remove some return keywords.

0e1b3de

Refactor print_only parameter of check into a new print function.

ba49813

Add Value enum to represent parsed values in function_runner.

62084b5

krk requested a review from abrown November 22, 2019 21:51

abrown approved these changes Nov 23, 2019

View reviewed changes

krk added 3 commits December 18, 2019 17:58

Remove unnecessary return.

32d94ec

Add descriptive panic message.

bd443f8

Reword error message.

0be5b8d

krk requested a review from abrown December 18, 2019 17:03

bnjbvr requested review from fitzgen and removed request for bnjbvr January 6, 2020 17:53

fitzgen reviewed Jan 6, 2020

View reviewed changes

krk mentioned this pull request Feb 28, 2020

Multi-value returns do not work in Windows ABI bytecodealliance/wasmtime#1162

Closed

abrown mentioned this pull request Mar 30, 2020

Parse run commands bytecodealliance/wasmtime#1436

Merged

krk closed this Apr 4, 2020

abrown mentioned this pull request Apr 15, 2020

Add ability to call CLIF functions with arbitrary arguments in filetests bytecodealliance/wasmtime#1517

Merged

Print return values of run tests if type is not boolean. #1231

Print return values of run tests if type is not boolean. #1231

Uh oh!

Conversation

krk commented Nov 14, 2019

Uh oh!

abrown commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abrown commented Nov 14, 2019

Uh oh!

krk commented Nov 15, 2019

Uh oh!

abrown commented Nov 15, 2019

Uh oh!

krk commented Nov 16, 2019

Uh oh!

krk commented Nov 16, 2019

Uh oh!

abrown commented Nov 17, 2019

Uh oh!

abrown left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abrown left a comment

Choose a reason for hiding this comment

Uh oh!

abrown commented Nov 23, 2019

Uh oh!

krk commented Dec 30, 2019

Uh oh!

bnjbvr commented Jan 6, 2020

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

abrown commented Nov 14, 2019 •

edited

Loading