cranelift: Allow call and call_indirect in runtests

[afonso360]

👋 Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.
• Reuses trampolines based on signature (something we lost in cranelift: Use cranelift-jit in runtests #4453)
• Internally auto renames functions to ExtName::User so that they are callable in JIT.
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Register all functions at once in test interpreter so that we can also enable call tests there
• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
• Generate call's in the fuzzer

[github-actions]

Subscribe to Label Action

cc @fitzgen

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

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

• fitzgen: fuzzing

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

Learn more.

[afonso360]

@jameysharp @cfallin I think this is ready, would you be able to review it?

[jameysharp]

I haven't had opportunity to give this a thorough review yet, but I've skimmed it, and at a high level I think it's a good idea. Not only does this PR allow us to test cases we can't test today, but it sounds like it can run the tests faster (by reusing compilation results and by short-circuiting unnecessary work sooner).

So far my only complaint is that the 16-character limit on function names forces changing a lot of tests, when I would think it'd be better to just increase the limit on name length. At a quick glance I didn't see where that length limit comes from. Does making that limit bigger have any effect on other uses of Cranelift, or is it just a limitation of the filetest infrastructure?

[jameysharp]

Tell me if I have this right: these tests only worked before because the actual function name was ignored, so it didn't matter that the run comments said little when the function declaration said big.

[afonso360]

Yes, test run (used to) ignore the actual name of the function.

We have had similar issues when we started adding test interpret to the runtests, since it calls functions by their ExternalName, however the interpreter also isn't used in this file.

[cfallin]

So far my only complaint is that the 16-character limit on function names forces changing a lot of tests, when I would think it'd be better to just increase the limit on name length

Historically at least, the limit was to keep the size of ExternalName reasonable; these are embedded and carried in the ir::Function and in "production" use, are two u32s only (the u0:0 format) because the embedder is supposed to own names and Cranelift handles indices only. I wasn't around for that design decision and I'm not sure if it would make sense to support a boxed variant for the "one-off test function with an owned name" case; that would be the way to go, IMHO, rather than bumping the limit and inflating the fixed-size field.

[jameysharp]

Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?

[afonso360]

Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?

I think this would be a good idea, if we can remove TestCase from ExternalName I think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!

Edit: Had a look around the code, and we probably also need to change the parser and a few other things for this.
I think it would be best if it was a separate PR, I can put this on hold while I try to figure it out.

[jameysharp]

I think this would be a good idea, if we can remove TestCase from ExternalName I think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!

I can confirm it's currently 20 bytes with TestCase, and only 8 bytes without, so that sounds good!

I've just done an experiment, reducing the length limit of test-case names to 6 bytes, so it fits inside an 8-byte ExternalName without having to make all the changes needed to actually remove TestCase. I'm running valgrind --tool=dhat target/release/wasmtime compile on the pulldown-cmark benchmark from Sightglass, with the wasmtime CLI built with --no-default-features to disable parallel compilation and other sources of nondeterminism.

With that experimental setup, the smaller ExternalName makes a positive difference, but it's... small. Total bytes allocated drops by 40,504 bytes out of about 171MiB. Maximum heap size drops by 1,096 bytes out of about 12MiB. And the program executes about 1 million fewer instructions, out of close to 2 billion.

So I think this is totally worth doing, but... don't get your hopes up for much improvement in memory usage or compile speed. 😁 I was really curious which way this would go, and it was fun doing the quick experiment.

[afonso360]

With that experimental setup, the smaller ExternalName makes a positive difference, but it's... small. Total bytes allocated drops by 40,504 bytes out of about 171MiB. Maximum heap size drops by 1,096 bytes out of about 12MiB. And the program executes about 1 million fewer instructions, out of close to 2 billion.

That's... not as much as I hoped for 😄. Thanks for checking anyway!

I started working on this refactor a couple days ago (in progress branch), but it's going to touch a lot of stuff and I don't think its going to be ready anytime soon.

@jameysharp Would it be ok to merge this with the test renames? I'd like to build on this to fix #4758 but its going to take a while if I have to do the refactor first and I'd prefer to use that time to fix the other fuzz issues.

[jameysharp]

I was hesitant, but actually yeah, renaming the test functions to fit within the limit is fine. Let's just plan that later we'll do a partial revert to get the more descriptive test names back.

One alternative that comes to mind is to leave the TestCase option in ExternalName, but make it a Box<[u8]> instead. Then ExternalName has the same size as it does now but hopefully it's easier to switch things over.

I'm happy enough either way though.

That said, I haven't had a chance to review this more carefully yet, so I'm not ready to merge it. Maybe @cfallin can get it into his review queue but he's got a lot on his plate. So maybe we can chat in #4758 about whether there are easier ways to fix the immediate bug there.

[afonso360]

I've filed #4766 to track this refactor.

One alternative that comes to mind is to leave the TestCase option in ExternalName, but make it a Box<[u8]> instead. Then ExternalName has the same size as it does now but hopefully it's easier to switch things over.

Thanks for picking this up! I didn't even consider that as an option, but its a great middle step and solves the immediate issue.

That said, I haven't had a chance to review this more carefully yet, so I'm not ready to merge it. Maybe @cfallin can get it into his review queue but he's got a lot on his plate. So maybe we can chat in #4758 about whether there are easier ways to fix the immediate bug there.

👍

[cfallin]

This all looks correct to me -- thanks for this refactor, as I think it'll be a really useful feature!

The function-renaming business is a little subtle but I don't have any easy suggestions for cleaning it up. It does seem like the "right" answer would be for cranelift-jit to keep a symbol table and support resolving references to testcase names, but that's probably just as complex as this change; and if it does eventually grow that for other reasons then we can remove the renaming layer here.

[jameysharp]

Well, darn. This seems to have broken cranelift-fuzzgen, and I don't understand why. But 20 out of 50 cases I tested from my existing corpus crash now, and it looks like they're all with one of these two messages:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Undeclared function u0:0 is referenced by u0:1!', fuzz/fuzz_targets/cranelift-fuzzgen.rs:73:53
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Duplicate function with name u0:1 found!', fuzz/fuzz_targets/cranelift-fuzzgen.rs:76:10

[jameysharp]

Okay, I'm reasonably convinced that "Undeclared function u0:0 is referenced by u0:1" has the same root cause as #4757 and #4758, although it's a different symptom. Previously these undefined functions caused failures if the interpreter tried to actually call them, or if the JIT tried to compile a call instruction referencing them. Now the new code also fails if there's a function signature declared, even if that function is never called.

I've opened PR #4795 for the other error. It's a small fix.