Bug 166199 - WebAssembly: construct 32-bit encodedJSValue properly
Summary: WebAssembly: construct 32-bit encodedJSValue properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on: 165805
Blocks: 159775
  Show dependency treegraph
 
Reported: 2016-12-20 11:52 PST by JF Bastien
Modified: 2016-12-20 14:17 PST (History)
6 users (show)

See Also:


Attachments
patch (18.17 KB, patch)
2016-12-20 12:06 PST, JF Bastien
mark.lam: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
patch (17.34 KB, patch)
2016-12-20 13:59 PST, JF Bastien
no flags 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-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.