Stream: git-wasmtime

Topic: wasmtime / PR #5482 Add a cranelift-compile fuzz target

Wasmtime GitHub notifications bot (Dec 20 2022 at 18:56):

elliottt opened PR #5482 from trevor/compile-fuzz-all-targets to main:

Add a cranelift-compile fuzz target that generates test cases to try to compile for all supported architectures.

<!--

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 (Dec 20 2022 at 19:11):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:14):

elliottt has marked PR #5482 as ready for review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:14):

elliottt requested jameysharp for a review on PR #5482.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:20):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:30):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:30):

afonso360 created PR review comment:

I can't comment on the exact line (61), but can we print only the correct target {} line here?

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:30):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:30):

afonso360 created PR review comment:

I think we might be able to reuse this for the icache fuzzer as well. Feel free to ignore it if it's too much work to change that.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:46):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:02):

elliottt created PR review comment:

That's a great point! I'd be happy to pull this type back into cranelift-fuzzgen so that we can use it in both targets :+1:

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:02):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:07):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:07):

elliottt created PR review comment:

Yep! I'll duplicate the match in cranelift-compile.rs here.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:19):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:21):

elliottt created PR review comment:

@afonso360 is this what you had in mind?

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:21):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:26):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:27):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:27):

elliottt created PR review comment:

I've added a new struct FunctionWithIsa in cranelift-fuzzgen, and will rework cranelift-icache in a separate PR once this one has landed.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:35):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:35):

afonso360 created PR review comment:

Yeah that looks good!

The reason for that is mostly that when we generate a function it is only guaranteed to compile for a particular architecture (due to unimplemented instructions), and having all of them there is slightly misleading.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:57):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Dec 20 2022 at 21:10):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Dec 21 2022 at 01:40):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 21 2022 at 01:40):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 21 2022 at 01:40):

jameysharp created PR review comment:

Wishlist: keep this list in cranelift/codegen/src/isa/mod.rs instead, and guard each entry with the same #[cfg] attributes as lookup/isa_builder!.

Wasmtime GitHub notifications bot (Dec 21 2022 at 01:40):

jameysharp created PR review comment:

I believe this alternative is guaranteed to work for all future targets, by relying on target_lexicon's own mapping between its enum values and strings:

        writeln!(f, "target {}", self.isa.triple().architecture)?;

A wishlist item: share most of the code in the Debug implementations of FunctionWithIsa and TestCase. Both print the same flags, target, and function. They just have different preambles (I believe you can move "test compile" before the flags), and TestCase also prints sample outputs at the end, but the middles are the same.

Wasmtime GitHub notifications bot (Dec 21 2022 at 01:40):

jameysharp created PR review comment:

run_func_passes builds a TargetIsa which probably doesn't match self.isa. I think the passes it runs probably don't actually look at anything except the target-independent flags, so that's probably fine, but I'm not sure. I think I've been confused about this before and forgotten the answer. Maybe @afonso360 knows?

Wasmtime GitHub notifications bot (Dec 21 2022 at 10:09):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Dec 21 2022 at 10:09):

afonso360 created PR review comment:

Yeah it does. We only need to build it because canonicalize_nans requires it, although it doesn't use it besides the enable_verifier flag.

It would remove quite a bit of code to not have to build it again, but it would be nice if we could avoid verifying it there, since I benchmarked it once and it ended up showing on the profiling. (We still run the verifier while compiling)

The other passes don't use the target ISA.

Wasmtime GitHub notifications bot (Jan 04 2023 at 01:43):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Jan 04 2023 at 01:43):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Jan 04 2023 at 21:44):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Jan 05 2023 at 05:01):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Jan 05 2023 at 05:06):

elliottt edited PR #5482 from trevor/compile-fuzz-all-targets to main:

Fuzz additional targets in the cranelift-icache target. The list of targets fuzzed is controlled by the targets enabled in fuzz/Cargo.toml.

This PR also reworks how instruction disabling is done in function generator, moving the deny-list to a function to make the decision at runtime instead of compile time.
<!--

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 (Jan 05 2023 at 05:08):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Jan 05 2023 at 05:28):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Wasmtime GitHub notifications bot (Jan 05 2023 at 05:33):

elliottt updated PR #5482 from trevor/compile-fuzz-all-targets to main.

Last updated: Dec 23 2024 at 12:05 UTC