Bug 147293 - Implement WebAssembly module parser
Summary: Implement WebAssembly module parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 147397 147400 147420 147443
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-07-24 23:24 PDT by Sukolsak Sakshuwong
Modified: 2015-07-31 13:35 PDT (History)
8 users (show)

See Also:


Attachments
Patch (34.39 KB, patch)
2015-07-24 23:36 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (34.16 KB, patch)
2015-07-27 05:28 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
remove isStringSource() (32.09 KB, patch)
2015-07-27 22:36 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
rename WASMFormat.h to WASMMagicNumber.h (32.22 KB, patch)
2015-07-28 14:19 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
rename WASMFormat.h to WASMMagicNumber.h (32.22 KB, patch)
2015-07-28 14:34 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Reupload the patch since r187539 should fix the JSWASMModule.h issue in the Windows build. (32.09 KB, patch)
2015-07-28 23:07 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Rebase to ToT. (32.07 KB, patch)
2015-07-29 11:13 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (32.12 KB, patch)
2015-07-30 19:03 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Fix ChangeLog (32.36 KB, patch)
2015-07-31 12:41 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2015-07-24 23:24:40 PDT
Implement WebAssembly module parser for WebAssembly files produced by pack-asmjs <https://github.com/WebAssembly/polyfill-prototype-1>.
Comment 1 Sukolsak Sakshuwong 2015-07-24 23:36:18 PDT
Created attachment 257511 [details]
Patch
Comment 2 Sukolsak Sakshuwong 2015-07-27 05:28:58 PDT
Created attachment 257558 [details]
Patch
Comment 3 Geoffrey Garen 2015-07-27 16:44:43 PDT
Comment on attachment 257558 [details]
Patch

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

r=me with some mixups

> Source/JavaScriptCore/parser/SourceProvider.h:90
> +#if ENABLE(WEBASSEMBLY)
> +        virtual bool isStringSource() const override
> +        {
> +            return true;
> +        }
> +#endif

SourceProvider has a subclass named "StringSourceProvider". This function is too easily confused with that class. 

For now, I think you should remove this function. Instead of performing an isStringSource() check, I think you should just go with the [WebAssembly Source] source string you've provided. In the future, we'll have to see what the spec says about toString on a WebAssembly function.

> Source/JavaScriptCore/wasm/WASMFormat.h:33
> +static const uint32_t wasmMagicNumber = 0x6d736177;

This file should be named WASMMagicNumber.h.
Comment 4 Geoffrey Garen 2015-07-27 16:44:56 PDT
*fixups
Comment 5 Sukolsak Sakshuwong 2015-07-27 22:36:56 PDT
Created attachment 257632 [details]
remove isStringSource()
Comment 6 Mark Lam 2015-07-27 22:44:09 PDT
Comment on attachment 257632 [details]
remove isStringSource()

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

> Source/JavaScriptCore/ChangeLog:29
> +        * wasm/WASMFormat.h: Added.

You missed Geoff’s comment to rename this file as WASMMagicNumber.h because thats what it contains.
Comment 7 Sukolsak Sakshuwong 2015-07-27 22:46:25 PDT
Thank you for the review.

(In reply to comment #3)
> Comment on attachment 257558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257558&action=review
> 
> r=me with some mixups
> 
> > Source/JavaScriptCore/parser/SourceProvider.h:90
> > +#if ENABLE(WEBASSEMBLY)
> > +        virtual bool isStringSource() const override
> > +        {
> > +            return true;
> > +        }
> > +#endif
> 
> SourceProvider has a subclass named "StringSourceProvider". This function is
> too easily confused with that class. 
> 
> For now, I think you should remove this function. Instead of performing an
> isStringSource() check, I think you should just go with the [WebAssembly
> Source] source string you've provided. In the future, we'll have to see what
> the spec says about toString on a WebAssembly function.

I originally named it isStringSourceProvider() but renamed it to isStringSource() after realizing that OpaqueJSScript was another subclass of SourceProvider that should have this method return true.

The reason I used this method was that SourceCode::toUTF8() returns "m_provider->source().impl()->utf8ForRange(m_startChar, m_endChar - m_startChar)". This will crash if source() is "[WebAssembly source]" and m_startChar and m_endChar are out of range. Come to think of it, maybe I shouldn't have m_startChar and m_endChar out of range in the first place.

> > Source/JavaScriptCore/wasm/WASMFormat.h:33
> > +static const uint32_t wasmMagicNumber = 0x6d736177;
> 
> This file should be named WASMMagicNumber.h.

This file will have more constants and enums that define the WASM file format. It will be very similar to <https://github.com/WebAssembly/polyfill-prototype-1/blob/master/src/shared.h> Should I rename it to WASMMagicNumber.h for now? Or maybe WASMConstants.h?
Comment 8 Geoffrey Garen 2015-07-28 12:56:10 PDT
> The reason I used this method was that SourceCode::toUTF8() returns
> "m_provider->source().impl()->utf8ForRange(m_startChar, m_endChar -
> m_startChar)". This will crash if source() is "[WebAssembly source]" and
> m_startChar and m_endChar are out of range. Come to think of it, maybe I
> shouldn't have m_startChar and m_endChar out of range in the first place.

Right: It is always an error for m_startChar and m_endChar to be out of range.
Comment 9 Geoffrey Garen 2015-07-28 12:58:17 PDT
> > > Source/JavaScriptCore/wasm/WASMFormat.h:33
> > > +static const uint32_t wasmMagicNumber = 0x6d736177;
> > 
> > This file should be named WASMMagicNumber.h.
> 
> This file will have more constants and enums that define the WASM file
> format. It will be very similar to
> <https://github.com/WebAssembly/polyfill-prototype-1/blob/master/src/shared.
> h> Should I rename it to WASMMagicNumber.h for now? Or maybe WASMConstants.h?

Which constants and enums do you plan to add?
Comment 10 Sukolsak Sakshuwong 2015-07-28 14:19:02 PDT
Created attachment 257686 [details]
rename WASMFormat.h to WASMMagicNumber.h
Comment 11 Sukolsak Sakshuwong 2015-07-28 14:34:48 PDT
Created attachment 257688 [details]
rename WASMFormat.h to WASMMagicNumber.h
Comment 12 Sukolsak Sakshuwong 2015-07-28 14:36:44 PDT
Per Geoff's suggestion on IRC, it would be good to separate the WASM representation in memory from the binary format, which includes the magic number. Put wasmMagicNumber in WASMMagicNumber.h for now. I will rename it later as more constants are added.
Comment 13 Geoffrey Garen 2015-07-28 17:05:05 PDT
Comment on attachment 257688 [details]
rename WASMFormat.h to WASMMagicNumber.h

r=me
Comment 14 WebKit Commit Bot 2015-07-28 17:56:45 PDT
Comment on attachment 257688 [details]
rename WASMFormat.h to WASMMagicNumber.h

Clearing flags on attachment: 257688

Committed r187531: <http://trac.webkit.org/changeset/187531>
Comment 15 WebKit Commit Bot 2015-07-28 17:56:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Simon Fraser (smfr) 2015-07-28 19:02:40 PDT
This broke the Windows build:
.\runtime\JSGlobalObject.cpp(95): fatal error C1083: Cannot open include file: 'JSWASMModule.h': No such file or directory [C:\cygwin\home\buildbot\slave\win-debug\build\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
Comment 17 WebKit Commit Bot 2015-07-28 20:24:30 PDT
Re-opened since this is blocked by bug 147397
Comment 18 Sukolsak Sakshuwong 2015-07-28 23:07:17 PDT
Created attachment 257738 [details]
Reupload the patch since r187539 should fix the JSWASMModule.h issue in the Windows build.
Comment 19 Mark Lam 2015-07-29 10:12:54 PDT
https://bugs.webkit.org/show_bug.cgi?id=147400 adds the wasm directory to the Windows projects files.  This fixes the condition that necessitated the rollout of this patch.
Comment 20 Mark Lam 2015-07-29 10:13:30 PDT
Comment on attachment 257738 [details]
Reupload the patch since r187539 should fix the JSWASMModule.h issue in the Windows build.

Rubber stamp to re-land this patch.
Comment 21 WebKit Commit Bot 2015-07-29 11:01:04 PDT
Comment on attachment 257738 [details]
Reupload the patch since r187539 should fix the JSWASMModule.h issue in the Windows build.

Rejecting attachment 257738 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 257738, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 187547 = 1fe8dd202bcea1baed52ec0873a511fdfcc32f45
r187548 = f10aaab6466f27e566d939dadd6f573fc55c4cf0
r187549 = 7bcaccefc8816a6065b6d627cb84cff6a20ae0e7
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/5948503670915072
Comment 22 Sukolsak Sakshuwong 2015-07-29 11:13:19 PDT
Created attachment 257758 [details]
Rebase to ToT.
Comment 23 Mark Lam 2015-07-29 11:14:25 PDT
Comment on attachment 257758 [details]
Rebase to ToT.

Try landing again via commit queue.
Comment 24 WebKit Commit Bot 2015-07-29 12:05:03 PDT
Comment on attachment 257758 [details]
Rebase to ToT.

Clearing flags on attachment: 257758

Committed r187550: <http://trac.webkit.org/changeset/187550>
Comment 25 WebKit Commit Bot 2015-07-29 12:05:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Commit Bot 2015-07-29 14:32:07 PDT
Re-opened since this is blocked by bug 147420
Comment 27 Sukolsak Sakshuwong 2015-07-30 19:03:41 PDT
Created attachment 257894 [details]
Patch
Comment 28 Saam Barati 2015-07-31 00:25:19 PDT
Comment on attachment 257894 [details]
Patch

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

> Source/JavaScriptCore/wasm/WASMReader.cpp:38
> +    result = m_cursor[0] | m_cursor[1] << 8 | m_cursor[2] << 16 | m_cursor[3] << 24;

Out of curiosity, why does byte order not matter here but matter below?
Also, does WASM follow network byte order?
Comment 29 Sukolsak Sakshuwong 2015-07-31 01:04:21 PDT
(In reply to comment #28)
> Comment on attachment 257894 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257894&action=review
> 
> > Source/JavaScriptCore/wasm/WASMReader.cpp:38
> > +    result = m_cursor[0] | m_cursor[1] << 8 | m_cursor[2] << 16 | m_cursor[3] << 24;
> 
> Out of curiosity, why does byte order not matter here but matter below?

Because the shift operator takes care of that. Say, m_cursor[0] = A, m_cursor[1] = B, m_cursor[2] = C, and m_cursor[3] = D.

On a little-endian machine,
A       = A 0 0 0
B << 8  = 0 B 0 0
C << 16 = 0 0 C 0
D << 24 = 0 0 0 D
result  = A B C D

On a big-endian machine,
A       = 0 0 0 A
B << 8  = 0 0 B 0
C << 16 = 0 C 0 0
D << 24 = D 0 0 0
result  = D C B A

which is as it should be.

> Also, does WASM follow network byte order?

The binary format hasn't been standardized yet. We are using the file format of an experimental WebAssembly polyfill <https://github.com/WebAssembly/polyfill-prototype-1>, which uses the little endian format.
Comment 30 Saam Barati 2015-07-31 01:26:32 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > Comment on attachment 257894 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=257894&action=review
> > 
> > > Source/JavaScriptCore/wasm/WASMReader.cpp:38
> > > +    result = m_cursor[0] | m_cursor[1] << 8 | m_cursor[2] << 16 | m_cursor[3] << 24;
> > 
> > Out of curiosity, why does byte order not matter here but matter below?
> 
> Because the shift operator takes care of that. Say, m_cursor[0] = A,
> m_cursor[1] = B, m_cursor[2] = C, and m_cursor[3] = D.
> 
> On a little-endian machine,
> A       = A 0 0 0
> B << 8  = 0 B 0 0
> C << 16 = 0 0 C 0
> D << 24 = 0 0 0 D
> result  = A B C D
> 
> On a big-endian machine,
> A       = 0 0 0 A
> B << 8  = 0 0 B 0
> C << 16 = 0 C 0 0
> D << 24 = D 0 0 0
> result  = D C B A
> 
> which is as it should be.
Makes sense. This is a cool illustration.
It's a little worrisome that this is wrong though
if it's decided that WASM binary format is big endian.
I'm not sure there is an alternative, though. 

> 
> > Also, does WASM follow network byte order?
> 
> The binary format hasn't been standardized yet. We are using the file format
> of an experimental WebAssembly polyfill
> <https://github.com/WebAssembly/polyfill-prototype-1>, which uses the little
> endian format.
Do we think it is likely that WASM binary format will stay
little endian? Would a lot of parsing code have to change if it's
big endian? It seems like it wouldn't be a lot but I haven't read
all the WASM code you've been working on.
Comment 31 Sukolsak Sakshuwong 2015-07-31 02:09:15 PDT
(In reply to comment #30)
> Do we think it is likely that WASM binary format will stay
> little endian?

I'm not sure, but my guess is yes. The design document says "WebAssembly portability assumes that execution environments offer the following characteristics: ... Little-endian byte ordering." <https://github.com/WebAssembly/design/blob/master/Portability.md> So, the little-endian ordering is probably preferred.

> Would a lot of parsing code have to change if it's
> big endian? It seems like it wouldn't be a lot but I haven't read
> all the WASM code you've been working on.

The final format will likely be very different from what we are using. So, a lot of code will have to change anyway. For the WASMReader class, I think there are only 5 methods would have to change, i.e., int16, int32, int64, float, and double.
Comment 32 Sukolsak Sakshuwong 2015-07-31 12:20:58 PDT
I have verified that https://bugs.webkit.org/show_bug.cgi?id=147443 fixes the build issue on Windows.
Comment 33 Mark Lam 2015-07-31 12:34:20 PDT
Comment on attachment 257894 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Reupload the patch, since r187591 should fix the "..\..\jsc.cpp(46):
> +        fatal error C1083: Cannot open include file: 'JSWASMModule.h'" issue.

I think you should include the original description here (from https://bugs.webkit.org/attachment.cgi?id=257688&action=review).  It's ok to prefix with it with a "Re-landing after fix for ..." but it is useful for the ChangeLog to actually describe what this change is about.  Please fix.
Comment 34 Sukolsak Sakshuwong 2015-07-31 12:41:33 PDT
Created attachment 257946 [details]
Fix ChangeLog
Comment 35 Mark Lam 2015-07-31 12:43:40 PDT
Comment on attachment 257946 [details]
Fix ChangeLog

Rubber stamp to re-land after Windows build fix.
Comment 36 WebKit Commit Bot 2015-07-31 13:34:57 PDT
Comment on attachment 257946 [details]
Fix ChangeLog

Clearing flags on attachment: 257946

Committed r187677: <http://trac.webkit.org/changeset/187677>
Comment 37 WebKit Commit Bot 2015-07-31 13:35:03 PDT
All reviewed patches have been landed.  Closing bug.