Wasmtime GitHub notifications bot (Aug 18 2023 at 18:53): Wasmtime GitHub notifications bot (Aug 18 2023 at 18:53):afonso360 opened PR #6859 from afonso360:delete-pseudo-opts to bytecodealliance:main:
:wave: Hey,
This transform is wrong for the inputs in the attached testcase. Both of these instructions differ with their NaN behaviours.
It was introduced in #5546 and has been sort of unnoticed until #6843 caused some wasmtime testcode to fire this optimization rule and produce this counterexample.
@jameysharp has also a nice example of how https://github.com/bytecodealliance/wasmtime/pull/6843#issuecomment-1684226551 the testcode now fires this rule (with #6843) where it hadn't before.
I have no idea how fuzzgen hasn't found this yet! It's a really simple input so it should have been found pretty much instantly.
I've been trying to look at why over the past week but haven't come to any conclusions yet. Right now I'm trying to look into the nan canonicalization pass and trying to check if it would mask this behavior.
Wasmtime GitHub notifications bot (Aug 18 2023 at 18:53):afonso360 requested abrown for a review on PR #6859.
Wasmtime GitHub notifications bot (Aug 18 2023 at 18:53):afonso360 requested wasmtime-compiler-reviewers for a review on PR #6859.
Wasmtime GitHub notifications bot (Aug 18 2023 at 18:54):afonso360 edited PR #6859:
:wave: Hey,
This transform is wrong for the inputs in the attached testcase. Both of these instructions differ with their NaN behaviours.
It was introduced in #5546 and has been sort of unnoticed until #6843 caused some wasmtime testcode to fire this optimization rule and produce this counterexample.
@jameysharp has also a nice example of how the testcode now fires this rule (with #6843) where it hadn't before.
I have no idea how fuzzgen hasn't found this yet! It's a really simple input so it should have been found pretty much instantly.
I've been trying to look at why over the past week but haven't come to any conclusions yet. Right now I'm trying to look into the nan canonicalization pass and trying to check if it would mask this behavior.
Wasmtime GitHub notifications bot (Aug 18 2023 at 18:57):afonso360 edited PR #6859:
:wave: Hey,
This transform is wrong for the inputs in the attached testcase. Both of these instructions differ with their NaN behaviours.
It was introduced in #5546 and has been sort of unnoticed until #6843 caused some wasmtime testcode to fire this optimization rule and produce this counterexample.
@jameysharp has also a nice example of how the testcode now fires this rule (with #6843) where it hadn't before.
I have no idea how fuzzgen hasn't found this yet! It's a really simple input so it should have been found pretty much instantly.
I've been trying to look at why over the past week but haven't come to any conclusions yet. Right now I'm trying to look into the nan canonicalization pass and trying to check if it would mask this behavior.
CC: @alexcrichton
Wasmtime GitHub notifications bot (Aug 18 2023 at 20:37):alexcrichton submitted PR review:
Ah as some extra background on this @afonso360 emailed security@bytecodealliance.org with this issue since it ran a risk of affecting wasm which we'd typically consider a security vulnerability (e.g. executing guests incorrectly). I discussed this this morning with @fitzgen and @sunfishcode and we talked about this issue in depth and concluded that this doesn't actually affect wasm (as @afonso360 initially concluded but sought confirmation of too).
In investigating this though we reached the same conclusion as you @jameysharp which is that we could fix the rule by swapping the operands. We were, however, surprised by the definition of
f{min,max}_pseudoin CLIF (which inherit their definitions from the simd proposal for wasm, which we found equally odd). Our conclusion was that we should probably just remove these rules for now and furthermore delete thef{min,max}_pseudoinstructions in favor of their expanded forms usingselectandfcmp. The x86 backend could then pattern match the two instructions together to achieve the desired instructions on x86 (which appeared to be the original motivation for adding them to the spec anyway)So if you agree then that seems like the least-risky way to move forward, effectively removing the confusion around operand orderings of
f{min,max}_pseudo, and instead having that subtelty only in the wasm->CLIF translation as well as the x86 backend.
Wasmtime GitHub notifications bot (Aug 21 2023 at 16:48):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 21 2023 at 17:54):alexcrichton merged PR #6859.
Last updated: Dec 23 2024 at 12:05 UTC