Add ability to call CLIF functions with arbitrary arguments in filetests

[abrown]

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

wasmtime/crates/api/src/func.rs

Lines 510 to 526 in 3b7cb6e

	let mut values_vec = vec![0; max(params.len(), self.ty.results().len())];
	// Store the argument values into `values_vec`.
	let param_tys = self.ty.params().iter();
	for ((arg, slot), ty) in params.iter().zip(&mut values_vec).zip(param_tys) {
	if arg.ty() != *ty {
	return Err(Trap::new("argument type mismatch"));
	}
	if !arg.comes_from_same_store(&self.store) {
	return Err(Trap::new(
	"cross-`Store` values are not currently supported",
	));
	}
	unsafe {
	arg.write_value_to(slot);
	}
	}

,

wasmtime/crates/jit/src/compiler.rs

Line 260 in 3b7cb6e

pub fn make_trampoline(

). To avoid re-compiling Trampolines for the same function signatures, Trampolines are cached in the SingleFunctionCompiler.

[abrown]

I expect this to fail because there are no encodings for booleans in load and store. @sunfishcode and others: should we add these encodings or should make_trampoline do something special for booleans?

[github-actions]

Subscribe to Label Action

cc @bnjbvr

Details

This issue or pull request has been labeled: "cranelift"

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

• bnjbvr: cranelift

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

Learn more.

[abrown]

@alexcrichton, here's the PR with the trampolines I was referring to. (You might notice they look pretty similar to the ones you created 😉).

[alexcrichton]

Nice! FWIW reading this over I think there's actually not a whole lot that we'd share between wasmtime and cranelift. Only a few small loops/etc would be shared, and otherwise it seems like we'd probably bend over backwards too much trying to cater to both use cases. In that sense I'd personally be ok if we just left this duplication in the two?

[abrown]

Ok, I think this is ready: since the x86 backend does not have encodings for loading and storing booleans, I took @sunfishcode's suggestion and convert boolean values from (load + icmp_imm ne ..., 0) and to (bint + store).

[bnjbvr]

Thanks! (and sorry about the long review times)
Looking good, nice jobs with the trampolines. A few suggestions below to harmonize the code a bit.

[bnjbvr]

Instead, could this look if trimmed is exactly run or print? I don't see the value in the ambiguity. If you agree with this, then we could rename this function is_run_or_print_command (since otherwise, there's a "run_command" called "run" and another called "print", which is a bit confusing).

In addition to this, could this return the command it found (so an Option, with None equivalent to current's false), instead of letting the caller clean up comments again and analyze the value?

[abrown]

Yeah, this is a good direction. These weird trimmed_comment_chars and is_potential_run_command functions were trying to do some lookahead to see if we actually needed to really parse the comment. I moved that logic into parse_run_command (which is in the parser module, where this functionality should be) and made it return ParseResult<Option<RunCommand>> instead of ParseResult<RunCommand>. The signature is a bit more complex but I think it is headed more in the direction you are suggesting.

[bnjbvr]

Could it also check that the host architecture is equal to the requested ISA's architecture?

[abrown]

Those checks should already exist at a higher level, e.g. test_run.rs, and I didn't want to limit this code too much because there are cases (x86 32-bit vs 64-bit) where multiple Cranelift ISAs could run on the same machine.

[bjorn3]

32bit x86 doesn't work in 64bit processes.

[bnjbvr]

Could we just do a write here, using a transmute? It avoids the indirection through Trampoline::write_value_to, which is a bit unsettling.

If we did the same for reads, then the need for the Trampoline data structure is much lower, and could probably be avoided. Or we could hide the writing and reading into a different new UnboxedValues struct type (that'd be a vec); then the slot_size could be an attribute of this struct, a const func returning the size of the values it's storing (and then we can remove a bunch of comments, and have more confidence in the size being sync'd with the storage type). What do you think?

[abrown]

I like having the Trampoline structure around to know what we are talking about; otherwise we have an Mmap floating around that is implicitly a trampoline. But that could be a type alias, I guess. I think your UnboxedValues makes sense and I started down this road but I'm not too confident about the transmute stuff--I will ping you on Zulip to see what you think.

[bnjbvr]

LGTM, thanks!