Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use floats for wasmtime::component::Val::Float* #5510

Merged

Conversation

lann
Copy link
Contributor

@lann lann commented Jan 3, 2023

The definitions of wasmtime::component::Val::Float{32,64} mirrored wasmtime::Val::F{32,64} by using integers as their wrapped types, storing the bit representation of their floating point values. This was necessary for the core Wasm f32/f64 types because Rust floats don't have guaranteed NaN bit representations.

#5480 (comment) @jameysharp:
We do this in Cranelift too. I don't know if it's for the same reason that Wasmtime's API works this way, but Cranelift's reason was discussed in #2251 (comment). In short, Rust doesn't make any guarantees about the bit-representation of NaN values being preserved in floating-point types. So if you care about that, you need to keep the raw bits in an integer type until the time when you want to do actual floating-point operations on them.

The component model float32/float64 types require NaN canonicalization, so we can use normal Rust f{32,64} instead.

Closes #5480

The definitions of `wasmtime::component::Val::Float{32,64}` mirrored
`wasmtime::Val::F{32,64}` by using integers as their wrapped types,
storing the bit representation of their floating point values.
This was necessary for the core Wasm `f32`/`f64` types because Rust
floats don't have guaranteed NaN bit representations.

The component model `float32`/`float64` types require NaN
canonicalization, so we can use normal Rust `f{32,64}` instead.

Closes bytecodealliance#5480
@alexcrichton alexcrichton enabled auto-merge (squash) January 3, 2023 19:21
@lann
Copy link
Contributor Author

lann commented Jan 3, 2023

I failed to mention: this removes the #[derive(Eq)] from Val and some related structs because f{32,64} aren't Eq. I think this makes sense in this context but it's technically a breaking change.

@alexcrichton
Copy link
Member

Yeah that's ok and I think it makes sense as well due to the usage of floats it shouldn't be applicable for Val anyway.

@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself labels Jan 3, 2023
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Subscribe to Label Action

cc @fitzgen, @peterhuene

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

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

  • fitzgen: fuzzing
  • peterhuene: wasmtime:api

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

Learn more.

@alexcrichton alexcrichton merged commit 0029ff9 into bytecodealliance:main Jan 3, 2023
@lann lann deleted the component-val-use-native-floats branch January 3, 2023 21:07
@alexcrichton
Copy link
Member

It looks like this broke fuzzing here because nan is no longer equal to nan. @lann would you be up for helping to fix this? I think the way to fix it would be to write a custom equality function which considers two NaN values to be equal.

@lann
Copy link
Contributor Author

lann commented Jan 5, 2023

Would it make sense to change the test code? e.g.

match (expected, actual) {
    // NaNs never compare equal
    (Val::Float32(expected), Val::Float32(actual)) if expected.is_nan() => assert!(actual.is_nan()),
    (Val::Float64(expected), Val::Float64(actual)) if expected.is_nan() => assert!(actual.is_nan()),
    (expected, actual) => assert_eq!(expected, actual),
};

@alexcrichton
Copy link
Member

More-or-less that's what needs to happen, but floats can appear recursively as well so the equality change there would need to be applied deeply through types like lists and recors too

lann added a commit to lann/wasmtime that referenced this pull request Jan 5, 2023
In bytecodealliance#5510 we changed the value types of these variants from u{32,64} to
f{32,64}. One side effect of this change was that two NaN values would
no longer compare equal. While this is behavior complies with IEEE-754
floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since
the component model requires NaN canonicalization, all NaN bit
representations compare equal, which is different from the original
behavior.

This also gives Vals the semantics of Eq again, so that has been
reintroduced to related types as well.
lann added a commit to lann/wasmtime that referenced this pull request Jan 5, 2023
In bytecodealliance#5510 we changed the value types of these variants from u{32,64} to
f{32,64}. One side effect of this change was that two NaN values would
no longer compare equal. While this is behavior complies with IEEE-754
floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since
the component model requires NaN canonicalization, all NaN bit
representations compare equal, which is different from the original
behavior.

This also gives Vals the semantics of Eq again, so that trait impl has
been reintroduced to related types as well.
lann added a commit to lann/wasmtime that referenced this pull request Jan 5, 2023
In bytecodealliance#5510 we changed the value types of these variants from u{32,64} to
f{32,64}. One side effect of this change was that two NaN values would
no longer compare equal. While this is behavior complies with IEEE-754
floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since
the component model requires NaN canonicalization, all NaN bit
representations compare equal, which is different from the original
behavior.

This also gives Vals the semantics of Eq again, so that trait impl has
been reintroduced to related types as well.
lann added a commit to lann/wasmtime that referenced this pull request Jan 5, 2023
In bytecodealliance#5510 we changed the value types of these variants from u{32,64} to
f{32,64}. One side effect of this change was that two NaN values would
no longer compare equal. While this is behavior complies with IEEE-754
floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since
the component model requires NaN canonicalization, all NaN bit
representations compare equal, which is different from the original
behavior.

This also gives Vals the semantics of Eq again, so that trait impl has
been reintroduced to related types as well.
lann added a commit to lann/wasmtime that referenced this pull request Jan 6, 2023
In bytecodealliance#5510 we changed the value types of these variants from u{32,64} to
f{32,64}. One side effect of this change was that two NaN values would
no longer compare equal. While this is behavior complies with IEEE-754
floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since
the component model requires NaN canonicalization, all NaN bit
representations compare equal, which is different from the original
behavior.

This also gives Vals the semantics of Eq again, so that trait impl has
been reintroduced to related types as well.
alexcrichton pushed a commit that referenced this pull request Jan 6, 2023
In #5510 we changed the value types of these variants from u{32,64} to
f{32,64}. One side effect of this change was that two NaN values would
no longer compare equal. While this is behavior complies with IEEE-754
floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since
the component model requires NaN canonicalization, all NaN bit
representations compare equal, which is different from the original
behavior.

This also gives Vals the semantics of Eq again, so that trait impl has
been reintroduced to related types as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

components: Use f{32,64} as wrapped type of Val::Float{32,64}
2 participants