Bug 165893 - WebAssembly: test_BuilderJSON.js is broken
Summary: WebAssembly: test_BuilderJSON.js is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
: 165907 (view as bug list)
Depends on: 165886
Blocks: 163919
  Show dependency treegraph
 
Reported: 2016-12-14 22:09 PST by JF Bastien
Modified: 2016-12-15 12:00 PST (History)
4 users (show)

See Also:


Attachments
Partial fix (2.28 KB, patch)
2016-12-14 22:09 PST, JF Bastien
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2016-12-14 22:47 PST, Keith Miller
msaboff: review+
keith_miller: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-12-14 22:09:02 PST
Bug #165886 broke it.

I started looking into it, but should do this in a separate patch.
Comment 1 JF Bastien 2016-12-14 22:09:34 PST
Created attachment 297167 [details]
Partial fix
Comment 2 Keith Miller 2016-12-14 22:47:52 PST
Created attachment 297170 [details]
Patch
Comment 3 Mark Lam 2016-12-14 22:56:09 PST
Comment on attachment 297170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297170&action=review

> JSTests/wasm/Builder.js:40
> +    case "i32": return Math.round(value) === value && LLB.varint32Min <= value && value <= LLB.varuint32Max;

Is the use of LLB.varuint32Max correct for i32?  Should it be LLB.varint32Max?
Comment 4 JF Bastien 2016-12-15 08:45:33 PST
Comment on attachment 297170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297170&action=review

Thanks! Looks good.

>> JSTests/wasm/Builder.js:40
>> +    case "i32": return Math.round(value) === value && LLB.varint32Min <= value && value <= LLB.varuint32Max;
> 
> Is the use of LLB.varuint32Max correct for i32?  Should it be LLB.varint32Max?

Here we want everything that can be encoded as a 32-bit integer because that's what wasm supports: i32 is sign-agnostic, the operations on some i32 values are sign-aware. We however want to expose this in a convenient manner to JS, so we allow [INT_MIN, UINT_MAX] as the input range. The code below then further restricts inputs which should be positive as needed: the JSON file identifies each opcode's immediate and for e.g. function_index we say "it has to be a positive i32".

So this is correct.
Comment 5 Michael Saboff 2016-12-15 08:48:05 PST
Comment on attachment 297170 [details]
Patch

r=me
Comment 6 Keith Miller 2016-12-15 08:54:16 PST
Committed r209863: <http://trac.webkit.org/changeset/209863>
Comment 7 Ryan Haddad 2016-12-15 12:00:18 PST
*** Bug 165907 has been marked as a duplicate of this bug. ***