Stream: git-wasmtime

Topic: wasmtime / PR #4782 cranelift: Add LibCalls to the interp...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:19):

afonso360 opened PR #4782 from interp-libcalls to main:

:wave: Hey,

This does not fix any of the issues reported by the fuzzer so far, but I think that is just a coincidence, since at some point it will generate a libcall and we cannot handle those either.

This allows the interpreter to register libcall handlers and invoke them when called.

I switched the fuzzer generated libcall from Udiv to Ishl, reason being that we don't want libcalls that can trap. Both functions have identical signatures so we shouldn't have any input format changes to the fuzzer.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:21):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:28):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:28):

afonso360 created PR review comment:

I saw some sources online that point to this being the correct behaviour, but nothing definitive. Does anyone know if this is correct?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:29):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:29):

afonso360 deleted PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:29):

afonso360 created PR review comment:

I saw some sources online that point to this being the correct behaviour, but nothing definitive. Does anyone know if this is correct?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:29):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:33):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 14:40):

afonso360 edited PR #4782 from interp-libcalls to main:

:wave: Hey,

This does not fix any of the issues reported by the fuzzer so far, but I think that is just a coincidence, since at some point it will generate a libcall and we cannot handle those either.

Edit: Actually this may have already been reported, but have returned the same error as #4758 so it wasn't reported separately.

This allows the interpreter to register libcall handlers and invoke them when called.

I switched the fuzzer generated libcall from Udiv to Ishl, reason being that we don't want libcalls that can trap. Both functions have identical signatures so we shouldn't have any input format changes to the fuzzer.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 15:34):

afonso360 created PR review comment:

I think it might also be a good idea to return Result<SmallVec<[V; 1]>, TrapCode> and allow these to trap controllably.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 15:34):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 15:34):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 15:34):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 16:34):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 16:34):

afonso360 created PR review comment:

I think this is undefined behaviour, so I've added trap codes to the libcall handlers and reverted back to div

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 16:35):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 16:36):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 16:41):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 17:59):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 17:59):

jameysharp created PR review comment:

I'm not sure I understand. We've recently defined shifts to be computed modulo the bit width. So I think Ishl64 is a fine choice, but it should evaluate to a << (b % 64) for all values of b. That in turn means that we don't need to check the preconditions on Rust's implementation of left-shift: it would panic in debug builds if the shift amount is 64 or higher, but we'll never hit that case.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 18:05):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 18:05):

afonso360 created PR review comment:

These libcalls end up being linked to the platform's runtime library if the interpreted version succeeds, so we need to make sure that they match the behaviour agreed by the runtime libraries and not our own.

We also can't really panic here since it would cause a fuzz error.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 18:07):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 18:07):

afonso360 created PR review comment:

Another option would be to define our own symbol for the JIT as well, and then we control both sides, the interpreted version and the compiled version.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 18:07):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2022 at 18:11):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2022 at 14:03):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2022 at 16:36):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2022 at 16:36):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2022 at 16:36):

jameysharp created PR review comment:

Since this SmallVec type ends up appearing in other modules, I'd suggest introducing a type alias for it. That way it's easy later either to change it to a regular Vec, or to put more elements in the inline array.

Also, &dyn Fn is probably more general than necessary. A closure that doesn't capture anything from its environment is compatible with a fn type, and I'd be surprised if interpreter libcall handlers ever needed context.

pub type LibCallValues<V> = SmallVec<[V; 1]>;
pub type LibCallHandler<V> = fn(LibCall, LibCallValues<V>) -> Result<LibCallValues<V>, TrapCode>;

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2022 at 16:36):

jameysharp created PR review comment:

What do you think of defining this function in function_generator.rs next to the array of allowed libcalls?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2022 at 07:30):

afonso360 created PR review comment:

The only reason I'm hesitant to do this is that fuzzgen is supposed to be executor agnostic (i.e. icache needs none of this), so it feels kinda weird to have a interpreter specific function there.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2022 at 07:30):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2022 at 07:30):

afonso360 updated PR #4782 from interp-libcalls to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2022 at 14:23):

afonso360 has marked PR #4782 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2022 at 19:38):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2022 at 19:38):

jameysharp created PR review comment:

Ah, that makes sense. But this is weird for the same reason that the function references are: only outer fuzz target knows which functions and which libcalls are okay to call. So I think the set of allowed libcalls (and functions) should be provided by the fuzz target. But I also don't think that needs to block merging this PR, it's just something I think we should revisit soon.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2022 at 19:53):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2022 at 19:53):

afonso360 created PR review comment:

So I think the set of allowed libcalls (and functions) should be provided by the fuzz target.

I like that!

But I also don't think that needs to block merging this PR, it's just something I think we should revisit soon.

:+1:

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2022 at 20:36):

jameysharp merged PR #4782.

Last updated: Dec 23 2024 at 12:05 UTC