Bug 166416 - WebAssembly: Make the spec-tests/start.wast.js test pass
Summary: WebAssembly: Make the spec-tests/start.wast.js test pass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-22 00:13 PST by Saam Barati
Modified: 2016-12-22 10:32 PST (History)
12 users (show)

See Also:


Attachments
patch (7.67 KB, patch)
2016-12-22 00:32 PST, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-12-22 00:13:16 PST
Two things are needed:
1. Our start functions are borked if the thing is an import (the current code assumes all imports are exports, which is wrong)
2. Unreachable should throw an exception, not crash.
Comment 1 Radar WebKit Bug Importer 2016-12-22 00:20:40 PST
<rdar://problem/29784532>
Comment 2 Saam Barati 2016-12-22 00:32:54 PST
Created attachment 297648 [details]
patch
Comment 3 Yusuke Suzuki 2016-12-22 01:20:56 PST
Comment on attachment 297648 [details]
patch

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

r=me

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:185
>          if (!m_startFunction.get()) {

I think this is always true, right? If so, let's drop this brace.
Comment 4 JF Bastien 2016-12-22 09:08:39 PST
Comment on attachment 297648 [details]
patch

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:185
>>          if (!m_startFunction.get()) {
> 
> I think this is always true, right? If so, let's drop this brace.

Indeed, the erroneous code I had above was the only code which touched m_startFunction. This should now never be set when we get here.

I think you should also move this code down here:
97    bool hasStart = !!moduleInformation.startFunctionIndexSpace;
98    auto startFunctionIndexSpace = moduleInformation.startFunctionIndexSpace.value_or(0);
Comment 5 Saam Barati 2016-12-22 10:23:16 PST
Comment on attachment 297648 [details]
patch

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

>>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:185
>>>          if (!m_startFunction.get()) {
>> 
>> I think this is always true, right? If so, let's drop this brace.
> 
> Indeed, the erroneous code I had above was the only code which touched m_startFunction. This should now never be set when we get here.
> 
> I think you should also move this code down here:
> 97    bool hasStart = !!moduleInformation.startFunctionIndexSpace;
> 98    auto startFunctionIndexSpace = moduleInformation.startFunctionIndexSpace.value_or(0);

Sounds good, I'll make these changes.
Comment 6 Saam Barati 2016-12-22 10:32:03 PST
landed in:
https://trac.webkit.org/changeset/210102