Bug 165893

Summary: WebAssembly: test_BuilderJSON.js is broken
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: jfbastien, keith_miller, ryanhaddad, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 165886    
Bug Blocks: 163919    
Attachments:
Description Flags
Partial fix
none
Patch msaboff: review+

JF Bastien
Reported 2016-12-14 22:09:02 PST
Bug #165886 broke it. I started looking into it, but should do this in a separate patch.
Attachments
Partial fix (2.28 KB, patch)
2016-12-14 22:09 PST, JF Bastien 		no flags
DetailsFormatted DiffDiff
Patch (3.84 KB, patch)
2016-12-14 22:47 PST, Keith Miller 		msaboff: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
JF Bastien
Comment 1 2016-12-14 22:09:34 PST
Created attachment 297167 [details] Partial fix
Keith Miller
Comment 2 2016-12-14 22:47:52 PST
Created attachment 297170 [details] Patch
Mark Lam
Comment 3 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?
JF Bastien
Comment 4 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.
Michael Saboff
Comment 5 2016-12-15 08:48:05 PST
Comment on attachment 297170 [details] Patch r=me
Keith Miller
Comment 6 2016-12-15 08:54:16 PST
Committed r209863: <http://trac.webkit.org/changeset/209863>
Ryan Haddad
Comment 7 2016-12-15 12:00:18 PST
*** Bug 165907 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.