Stream: git-wasmtime

Topic: wasmtime / issue #5764 fuzzgen: Refactor name and signatu...

Wasmtime GitHub notifications bot (Feb 15 2023 at 14:15):

afonso360 commented on issue #5764:

Or perhaps we should always pass in the full list of libcalls; what test coverage do we gain by randomly removing libcalls from the list?

This is what ended up happening anyway in #5765, I'm going to backport that. I don't think we lose anything by doing it that way and its a bit cleaner.

It might be nice to only add function declarations to builder.func if they're actually used, but that's a separate matter.

Now that you mention it, that sounds like a great idea! And it solves one of the issues in #5765 which is that, the function "headers" become quite verbose if we pass in too many call targets which is annoying because they currently don't get reduced so even the smallest function possible is still has 6 lines of signatures + 6 lines of function references!

I'm going to give this a go. Would you prefer that as a separate PR or should I bundle it into this one?

I spent a while trying to figure out if this PR makes it any easier to move the list of ALLOWED_LIBCALLS to cranelift-fuzzgen so we don't have to keep it in sync across different modules with the list of supported interpreter implementations. I didn't come up with a good answer, but it's something I'd like to see, if you happen to have any ideas.

The only thing I can think of would be to make Config a associated constant of TestCase, doing a generic arbitrary impl for that and having something like

const CONFIG = Config {
  allowed_libcalls: [
   ...etc.
  ],
  ..Config::default()
}

fuzz_target!(|testcase: TestCase<CONFIG>| ..)

I think this could work, but I'm not sure if we can const eval that?

Wasmtime GitHub notifications bot (Feb 15 2023 at 14:19):

afonso360 edited a comment on issue #5764:

Or perhaps we should always pass in the full list of libcalls; what test coverage do we gain by randomly removing libcalls from the list?

This is what ended up happening anyway in #5765, I'm going to backport that. I don't think we lose anything by doing it that way and its a bit cleaner.

It might be nice to only add function declarations to builder.func if they're actually used, but that's a separate matter.

Now that you mention it, that sounds like a great idea! And it solves one of the issues in #5765 which is that, the function "headers" become quite verbose if we pass in too many call targets which is annoying because they currently don't get reduced so even the smallest function possible is still has 6 lines of signatures + 6 lines of function references!

I'm going to give this a go. Would you prefer that as a separate PR or should I bundle it into this one?

I spent a while trying to figure out if this PR makes it any easier to move the list of ALLOWED_LIBCALLS to cranelift-fuzzgen so we don't have to keep it in sync across different modules with the list of supported interpreter implementations. I didn't come up with a good answer, but it's something I'd like to see, if you happen to have any ideas.

The only thing I can think of would be to make Config a associated constant of TestCase, doing a generic arbitrary impl for that and having something like

const CONFIG = Config {
  allowed_libcalls: &'static [
   ...etc.
  ],
  ..Config::default()
}

fuzz_target!(|testcase: TestCase<CONFIG>| ..)

I think this could work, but I'm not sure if we can const eval that?

Wasmtime GitHub notifications bot (Feb 15 2023 at 14:20):

afonso360 edited a comment on issue #5764:

Or perhaps we should always pass in the full list of libcalls; what test coverage do we gain by randomly removing libcalls from the list?

This is what ended up happening anyway in #5765, I'm going to backport that. I don't think we lose anything by doing it that way and its a bit cleaner.

It might be nice to only add function declarations to builder.func if they're actually used, but that's a separate matter.

Now that you mention it, that sounds like a great idea! And it solves one of the issues in #5765 which is that, the function "headers" become quite verbose if we pass in too many call targets which is annoying because they currently don't get reduced so even the smallest function possible still has 6 lines of signatures + 6 lines of function references!

I'm going to give this a go. Would you prefer that as a separate PR or should I bundle it into this one?

I spent a while trying to figure out if this PR makes it any easier to move the list of ALLOWED_LIBCALLS to cranelift-fuzzgen so we don't have to keep it in sync across different modules with the list of supported interpreter implementations. I didn't come up with a good answer, but it's something I'd like to see, if you happen to have any ideas.

The only thing I can think of would be to make Config a associated constant of TestCase, doing a generic arbitrary impl for that and having something like

const CONFIG = Config {
  allowed_libcalls: &'static [
   ...etc.
  ],
  ..Config::default()
}

fuzz_target!(|testcase: TestCase<CONFIG>| ..)

I think this could work, but I'm not sure if we can const eval that?

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:58):

jameysharp commented on issue #5764:

Regarding cleaning up the function header, I'm happy whether that's in this PR or separate. I'd take this PR even if you don't get around to that. So pick whichever is more convenient for you.

When I look at the associated constant idea, I wonder: why do we export types like TestCase from the cranelift-fuzzgen crate?

If we declared that struct in the cranelift-fuzzgen target instead, we just need to provide implementations of Arbitrary and Debug along with it. The current Arbitrary implementation is very short because it mostly delegates to the FuzzGen struct, and if we put that in the fuzz target then we have the opportunity to tweak the config there. The FuzzGen::generate_host_test method probably should get inlined in the implementation of Arbitrary. But the lower-level methods (generate_flags, generate_func, generate_test_inputs) are sensible things to share across multiple fuzz targets.

I think the same plan works for FunctionWithIsa too. Its current Arbitrary implementation is more complicated but I think all the complexity is reasonable to move to the cranelift-icache fuzz target.

I'd prefer if both Debug implementations call a shared helper from the fuzzgen crate too, for printing the target/flags/function body. Really, I want that even if we don't move these structs to the fuzz targets.

Again, though, none of that needs to happen in _this_ PR.

Wasmtime GitHub notifications bot (Feb 17 2023 at 12:54):

afonso360 commented on issue #5764:

I've changed this to always pass all libcalls.

I like that idea of having each target defining its own Arbitrary type. I'll make that change as a separate PR!

Same thing with the function header cleanup, I'll do that in a separate PR.

Last updated: Dec 23 2024 at 12:05 UTC