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

Fix Float::copysign impl #293

Merged
merged 2 commits into from
Jan 3, 2022
Merged

Fix Float::copysign impl #293

merged 2 commits into from
Jan 3, 2022

Conversation

Robbepop
Copy link
Member

@Robbepop Robbepop commented Jan 1, 2022

I discovered today that the official WebAssembly spec test suite assertions and wasmi results diverge in a particular case with the copysign instruction.
For F32::from_bits(0xFFFC_0000).copysign(F32::from_bits(0)) the expected result according to the WebAssembly Spec test suite is 0x7FFC_0000, however, wasmi currently returns 0xFFFC_0000.
The Rust documentation mandates the same behavior from copysign as WebAssembly does: https://doc.rust-lang.org/std/primitive.f32.html#method.copysign

The bug was not caught because the current wasmi Wasm spec test runner seems to improperly handle NaN values.

Fortunately the bug fix was easy, however, I still wonder why we are going to great lengths like this.

@codecov-commenter
Copy link

Codecov Report

Merging #293 (ac18ee5) into master (8b27c2b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
+ Coverage   80.00%   80.02%   +0.01%     
==========================================
  Files          30       30              
  Lines        4672     4676       +4     
==========================================
+ Hits         3738     3742       +4     
  Misses        934      934              
Impacted Files Coverage Δ
src/value.rs 69.45% <100.00%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b27c2b...ac18ee5. Read the comment docs.

@athei
Copy link
Collaborator

athei commented Jan 3, 2022

The bug was not caught because the current wasmi Wasm spec test runner seems to improperly handle NaN values.

Shouldn't we fix this first then? To make sure that this does not brake other things.

@Robbepop
Copy link
Member Author

Robbepop commented Jan 3, 2022

Shouldn't we fix this first then? To make sure that this does not brake other things.

The v1 implementation caught this bug since v1 properly tests this property.
The v0 implementation of the Wasm spec test builds on wabt whereas v1 builds on wast crate.
The fix is very isolated - we are just touching the one particular float operation.
I agree that ideally we should also fix the Wasm spec test suite for v0 but this should best be done in another PR imo.

@athei
Copy link
Collaborator

athei commented Jan 3, 2022

This code is used by v0 and v1?

@Robbepop
Copy link
Member Author

Robbepop commented Jan 3, 2022

This code is used by v0 and v1?

yes, v1 already implements the fix. This PR backports it to v0.
Same for #295 in case you wonder.

@Robbepop Robbepop merged commit 8825aed into master Jan 3, 2022
@athei athei deleted the rf-fix-copysign branch January 3, 2022 14:01
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jul 12, 2022
This fixes a bug recently found on oss-fuzz which was fixed in
wasmi-labs/wasmi#295 and wasmi-labs/wasmi#293.
alexcrichton added a commit to bytecodealliance/wasmtime that referenced this pull request Jul 12, 2022
This fixes a bug recently found on oss-fuzz which was fixed in
wasmi-labs/wasmi#295 and wasmi-labs/wasmi#293.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants