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

JF Bastien
Reported 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.
Attachments
patch (18.17 KB, patch)
2016-12-20 12:06 PST, JF Bastien 		mark.lam: review+
mark.lam: commit-queue-
DetailsFormatted DiffDiff
patch (17.34 KB, patch)
2016-12-20 13:59 PST, JF Bastien 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 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".
JF Bastien
Comment 2 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.
JF Bastien
Comment 3 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).
JF Bastien
Comment 4 2016-12-20 12:06:38 PST
Created attachment 297545 [details] patch It compiles and passes all tests!
Mark Lam
Comment 5 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().
Radar WebKit Bug Importer
Comment 6 2016-12-20 13:59:04 PST
<rdar://problem/29759741>
JF Bastien
Comment 7 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.
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2016-12-20 14:17:13 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.