Bug 166199 – WebAssembly: construct 32-bit encodedJSValue properly

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[Mark Lam]

(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".

[JF Bastien]

(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.

[JF Bastien]

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).

[JF Bastien]

Created attachment 297545 [details]
patch

It compiles and passes all tests!

[Mark Lam]

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().

[Radar WebKit Bug Importer]

<rdar://problem/29759741>

[JF Bastien]

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.

[WebKit Commit Bot]

Comment on attachment 297553 [details]
patch

Clearing flags on attachment: 297553

Committed r210038: <http://trac.webkit.org/changeset/210038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.