Bug 166199

Summary: WebAssembly: construct 32-bit encodedJSValue properly
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 165805    
Bug Blocks: 159775    
Attachments:
Description Flags
patch
mark.lam: review+, mark.lam: commit-queue-
patch none

Description JF Bastien 2016-12-20 11:52:01 PST 
Mark pointed out here: https://bugs.webkit.org/show_bug.cgi?id=165805#c3
That I was using { } to construct default encodedJSValue, and that's wrong on 32-bit. I think I changed to { } based on a review, but that was wrong. wasm doesn't do 64-bit, but we may as well Do It Right.
Comment 1 Mark Lam 2016-12-20 11:58:25 PST 
(In reply to comment #0)
> Mark pointed out here: https://bugs.webkit.org/show_bug.cgi?id=165805#c3
> That I was using { } to construct default encodedJSValue, and that's wrong
> on 32-bit. I think I changed to { } based on a review, but that was wrong.
> wasm doesn't do 64-bit, but we may as well Do It Right.

I think you mean "wasm doesn't do 32-bit".
Comment 2 JF Bastien 2016-12-20 12:01:10 PST 
(In reply to comment #1)
> (In reply to comment #0)
> > Mark pointed out here: https://bugs.webkit.org/show_bug.cgi?id=165805#c3
> > That I was using { } to construct default encodedJSValue, and that's wrong
> > on 32-bit. I think I changed to { } based on a review, but that was wrong.
> > wasm doesn't do 64-bit, but we may as well Do It Right.
> 
> I think you mean "wasm doesn't do 32-bit".

Oops yes.
Comment 3 JF Bastien 2016-12-20 12:02:40 PST 
I'm making this change semi-automatically. First I run:
  git grep RETURN ./Source/JavaScriptCore/wasm/ | grep "{ })" | cut -f1 -d: | sort -u | xargs -n1 -I{} gsed -i 's/RETURN_IF_EXCEPTION(\([a-zA-Z]*\), { });/RETURN_IF_EXCEPTION(\1, encodedJSValue());/' {}

And then I switch back all returns which were actually JSValue (the compiler chokes on these bad conversions).
Comment 4 JF Bastien 2016-12-20 12:06:38 PST 
Created attachment 297545 [details]
patch

It compiles and passes all tests!
Comment 5 Mark Lam 2016-12-20 12:09:46 PST 
Comment on attachment 297545 [details]
patch

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

r=me with the bug fixed.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:40
> -    RETURN_IF_EXCEPTION(throwScope, { });
> +    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());

This one is not right.  The return type is uint32_t.  It's proper to return {} here and wrong to return encodedJSValue().
Comment 6 Radar WebKit Bug Importer 2016-12-20 13:59:04 PST 
<rdar://problem/29759741>
Comment 7 JF Bastien 2016-12-20 13:59:30 PST 
Created attachment 297553 [details]
patch

(In reply to comment #5)
> Comment on attachment 297545 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297545&action=review
> 
> r=me with the bug fixed.
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:40
> > -    RETURN_IF_EXCEPTION(throwScope, { });
> > +    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
> 
> This one is not right.  The return type is uint32_t.  It's proper to return
> {} here and wrong to return encodedJSValue().

Oops yes, I hadn't seen that! Good catch.
Comment 8 WebKit Commit Bot 2016-12-20 14:17:09 PST 
Comment on attachment 297553 [details]
patch

Clearing flags on attachment: 297553

Committed r210038: <http://trac.webkit.org/changeset/210038>
Comment 9 WebKit Commit Bot 2016-12-20 14:17:13 PST 
All reviewed patches have been landed.  Closing bug.