Bug 198106

Summary: WebAssembly: pow functions returns 0 when exponent 1.0 or -1.0
Product: WebKit Reporter: Philipp <philipp.gloor>
Component: WebAssemblyAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Critical CC: alonzakai, commit-queue, corey.lucier, ews-watchlist, fpizlo, justin_michaud, keith_miller, mark.lam, msaboff, philipp.gloor, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: iPhone / iPad   
OS: iOS 12   
Attachments:
Description Flags
Binary from emcc v1.38.31
none
Binary from emcc v1.38.31 (Wasm)
none
Text format output from wabt
none
Pow test module
none
Pow test module
none
Patch
none
Patch
none
Patch none

Philipp
Reported 2019-05-22 04:37:14 PDT
This issue occured when we updated from Emscripten 1.38.6 to 1.38.30 (might appear before) but it only showed up on iOS 12.2/12.3/12.4 Beta. That's why I'm posting this here. This code has unexpected behaviour when the exponent is 1.0 or -1.0, i.e. the function returns 0. The code in C++ is: // wasm_bug.cpp #include <emscripten/bind.h> #include <cstdio> void pow_test(double exp) { printf("pow(10, %f): %f\n", exp, pow(10.0, exp)); } EMSCRIPTEN_BINDINGS() { emscripten::function("pow_test", &pow_test); } compiled with: emcc --bind -O2 wasm_bug.cpp -o out.js
Attachments
Binary from emcc v1.38.31 (43.46 KB, application/x-javascript)
2019-05-28 13:25 PDT, Justin Michaud 		no flags
Details
Binary from emcc v1.38.31 (Wasm) (26.88 KB, application/octet-stream)
2019-05-28 13:28 PDT, Justin Michaud 		no flags
Details
Text format output from wabt (296.29 KB, text/plain)
2019-05-28 13:28 PDT, Justin Michaud 		no flags
Details
Pow test module (60.13 KB, application/x-javascript)
2019-06-05 11:21 PDT, Justin Michaud 		no flags
Details
Pow test module (26.88 KB, application/octet-stream)
2019-06-05 11:21 PDT, Justin Michaud 		no flags
Details
Patch (3.52 KB, patch)
2019-06-05 11:25 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (6.45 KB, patch)
2019-06-05 16:44 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (6.45 KB, patch)
2019-06-05 16:46 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-05-22 16:25:46 PDT
<rdar://problem/51047345>
Justin Michaud
Comment 2 2019-05-28 13:25:37 PDT
Created attachment 370778 [details] Binary from emcc v1.38.31
Justin Michaud
Comment 3 2019-05-28 13:28:08 PDT Comment hidden (obsolete)
Justin Michaud
Comment 4 2019-05-28 13:28:52 PDT Comment hidden (obsolete)
Justin Michaud
Comment 5 2019-05-28 16:46:47 PDT Comment hidden (obsolete)
Justin Michaud
Comment 6 2019-06-04 16:57:55 PDT
The following program exhibits different behavior on Mac/iOS: out.wat: (module (func (export "test") (result f64) f64.const 42 f64.const 42 i32.const -1 i32.const -1 i32.eq select )) out.js: import { test } from 'out.wasm' print("test: " + test()) Run as module with --wasmBBQUsesAir=0 --defaultB3OptLevel=0 to reproduce. iOS gives 0, Mac gives 42.
Yusuke Suzuki
Comment 7 2019-06-04 17:06:46 PDT
(In reply to Justin Michaud from comment #6) > The following program exhibits different behavior on Mac/iOS: > > out.wat: > > (module > (func (export "test") (result f64) > f64.const 42 > f64.const 42 > i32.const -1 > i32.const -1 > i32.eq > select > )) > > out.js: > import { test } from 'out.wasm' > > print("test: " + test()) > > Run as module with --wasmBBQUsesAir=0 --defaultB3OptLevel=0 to reproduce. > iOS gives 0, Mac gives 42. Nice! It sounds like a good starting point to investigate this problem.
Saam Barati
Comment 8 2019-06-04 18:49:18 PDT
(In reply to Justin Michaud from comment #6) > The following program exhibits different behavior on Mac/iOS: > > out.wat: > > (module > (func (export "test") (result f64) > f64.const 42 > f64.const 42 > i32.const -1 > i32.const -1 > i32.eq > select > )) > > out.js: > import { test } from 'out.wasm' > > print("test: " + test()) > > Run as module with --wasmBBQUsesAir=0 --defaultB3OptLevel=0 to reproduce. > iOS gives 0, Mac gives 42. If I were to guess, this is a bug in B3 strength reduction, or in B3 fold path constants
Saam Barati
Comment 9 2019-06-04 18:49:50 PDT
When I have bugs like this, I like to make B3 not run some optimization passes to isolate the issue
Justin Michaud
Comment 10 2019-06-05 11:21:07 PDT
Created attachment 371421 [details] Pow test module
Justin Michaud
Comment 11 2019-06-05 11:21:21 PDT
Created attachment 371422 [details] Pow test module
Justin Michaud
Comment 12 2019-06-05 11:25:21 PDT
Created attachment 371423 [details] Patch
Saam Barati
Comment 13 2019-06-05 11:27:36 PDT
Comment on attachment 371423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371423&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2462 > + m_assembler.fcsel<64>(dest, thenCase, elseCase, ARM64Condition(cond)); Please also add a standalone testmasm test.
Saam Barati
Comment 14 2019-06-05 11:28:20 PDT
Comment on attachment 371423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371423&action=review > Source/JavaScriptCore/ChangeLog:8 > + Fix bug caused by using fcsel sX instead of fcsel dX on an f64 value in the wasm select opcode. nit: This effects Wasm, but isn't just Wasm. It effects anyone using our macro assembler
Justin Michaud
Comment 15 2019-06-05 16:44:49 PDT
Created attachment 371448 [details] Patch
Justin Michaud
Comment 16 2019-06-05 16:46:17 PDT
Created attachment 371449 [details] Patch
WebKit Commit Bot
Comment 17 2019-06-05 17:29:00 PDT
Comment on attachment 371449 [details] Patch Clearing flags on attachment: 371449 Committed r246134: <https://trac.webkit.org/changeset/246134>
WebKit Commit Bot
Comment 18 2019-06-05 17:29:01 PDT
All reviewed patches have been landed. Closing bug.
Saam Barati
Comment 19 2019-06-05 18:22:38 PDT
Comment on attachment 371449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371449&action=review > Source/JavaScriptCore/assembler/testmasm.cpp:1088 > + RUN(testMoveDoubleConditionally32()); Nice!
Philipp
Comment 20 2019-08-09 06:42:56 PDT
I updated my iPad to 12.4 and tested my initial example. The problem still remains. Did this fix make it into 12.4?
CoreyDotCom
Comment 21 2019-09-13 14:36:15 PDT
Can someone please comment as to what version this is landing in and if the fix will make it to a 12.5? I work for Adobe and we have several projects being derailed because of this WASM issue. Thanks. @justin
CoreyDotCom
Comment 22 2019-09-13 14:38:23 PDT
For what it's worth it does seem fixed in ios 13 but obviously doesn't help our production users now.
Note You need to log in before you can comment on or make changes to this bug.