Bug 198106 - WebAssembly: pow functions returns 0 when exponent 1.0 or -1.0
Summary: WebAssembly: pow functions returns 0 when exponent 1.0 or -1.0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: Safari 12
Hardware: iPhone / iPad iOS 12
: P2 Critical
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-22 04:37 PDT by Philipp
Modified: 2019-09-13 14:38 PDT (History)
13 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2019-06-05 16:44 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2019-06-05 16:46 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philipp 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
Comment 1 Radar WebKit Bug Importer 2019-05-22 16:25:46 PDT
<rdar://problem/51047345>
Comment 2 Justin Michaud 2019-05-28 13:25:37 PDT
Created attachment 370778 [details]
Binary from emcc v1.38.31
Comment 3 Justin Michaud 2019-05-28 13:28:08 PDT Comment hidden (obsolete)
Comment 4 Justin Michaud 2019-05-28 13:28:52 PDT Comment hidden (obsolete)
Comment 5 Justin Michaud 2019-05-28 16:46:47 PDT Comment hidden (obsolete)
Comment 6 Justin Michaud 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.
Comment 7 Yusuke Suzuki 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.
Comment 8 Saam Barati 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
Comment 9 Saam Barati 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
Comment 10 Justin Michaud 2019-06-05 11:21:07 PDT
Created attachment 371421 [details]
Pow test module
Comment 11 Justin Michaud 2019-06-05 11:21:21 PDT
Created attachment 371422 [details]
Pow test module
Comment 12 Justin Michaud 2019-06-05 11:25:21 PDT
Created attachment 371423 [details]
Patch
Comment 13 Saam Barati 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.
Comment 14 Saam Barati 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
Comment 15 Justin Michaud 2019-06-05 16:44:49 PDT
Created attachment 371448 [details]
Patch
Comment 16 Justin Michaud 2019-06-05 16:46:17 PDT
Created attachment 371449 [details]
Patch
Comment 17 WebKit Commit Bot 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>
Comment 18 WebKit Commit Bot 2019-06-05 17:29:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Saam Barati 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!
Comment 20 Philipp 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?
Comment 21 CoreyDotCom 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
Comment 22 CoreyDotCom 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.