Stream: git-wasmtime

Topic: wasmtime / issue #5535 Treat `wasmtime::component::Val::F...

Wasmtime GitHub notifications bot (Jan 05 2023 at 21:57):

lann commented on issue #5535:

As requested by @alexcrichton in https://github.com/bytecodealliance/wasmtime/pull/5510#issuecomment-1372719177

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

github-actions[bot] commented on issue #5535:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

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

• peterhuene: wasmtime:api

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Jan 05 2023 at 22:26):

jameysharp commented on issue #5535:

When we had a similar concern in Cranelift we ended up introducing a separate function that can just special-case floats and use a derived implementation of PartialEq otherwise: bitwise_eq.

Although you don't want bitwise equality here, the same pattern should work. It avoids the need to list every enum variant, and the possibility of bugs when new variants are added.

Another way to avoid bugs is to have two cases for each variant, like the implementation of PartialEq for DataValue. But I think here it's fine to use assert! instead of assert_eq! in the fuzz target and call a specialized function to do the right check.

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

lann commented on issue #5535:

a separate function that can just special-case floats

That certainly could work, but it is significantly more code because of the component model's composite and parameterized types.

Wasmtime GitHub notifications bot (Jan 05 2023 at 22:49):

jameysharp commented on issue #5535:

That certainly could work, but it is significantly more code because of the component model's composite and parameterized types.

Huh, I don't see why. I'm suggesting putting back the derive(PartialEq) for Val, and then writing something like this:

impl Val {
    pub fn funny_eq(&self, other: &Val) -> bool {
        match (self, other) {
            // This breaks conformance with IEEE-754 equality to simplify testing logic.
            (Self::Float32(l), Self::Float32(r)) => l == r || (l.is_nan() && r.is_nan()),
            (Self::Float64(l), Self::Float64(r)) => l == r || (l.is_nan() && r.is_nan()),
            _ => self == other,
        }
    }
}

Or, if it's only intended to be used in one place, just put this match there.

Wasmtime GitHub notifications bot (Jan 05 2023 at 22:51):

jameysharp commented on issue #5535:

I saw why immediately after I posted that comment. I forgot the derived implementations wouldn't be calling this. I take it all back!

So this PR seems about as good as it can be, except I'd still propose the second pattern I mentioned to ensure that the compiler will catch the error if new variants are added to Val without updating the implementation of PartialEq.

Wasmtime GitHub notifications bot (Jan 06 2023 at 14:11):

lann commented on issue #5535:

Another way to avoid bugs is to have two cases for each variant, like the implementation of PartialEq for DataValue.

Done.

Wasmtime GitHub notifications bot (Jan 06 2023 at 14:59):

sunfishcode commented on issue #5535:

I don't have full context here, but I wanted to make sure it's considered that floating-point equality considers -0 and 0 to be equal, while most testing and fuzzing use cases should treat them as inequal.

Wasmtime GitHub notifications bot (Jan 06 2023 at 15:01):

lann commented on issue #5535:

I don't have full context here, but I wanted to make sure it's considered that floating-point equality considers -0 and 0 to be equal, while most testing and fuzzing use cases should treat them as inequal.

I _think_ the issue is just NaN inequality, but I don't entirely understand the fuzzing code in question.

Wasmtime GitHub notifications bot (Jan 06 2023 at 16:40):

alexcrichton commented on issue #5535:

I'm going to go ahead and merge this since it fixes the fuzz issues that have cropped up, but I think it'd be reasonable to have a follow-up for +/- 0 handling as well.

Last updated: Oct 23 2024 at 20:03 UTC