Bug 147293

Created attachment 257511 [details] Patch

Created attachment 257558 [details] Patch

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.

*fixups

Created attachment 257632 [details] remove isStringSource()

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.

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?

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

> > > 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?

Created attachment 257686 [details] rename WASMFormat.h to WASMMagicNumber.h

Created attachment 257688 [details] rename WASMFormat.h to WASMMagicNumber.h

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 on attachment 257688 [details] rename WASMFormat.h to WASMMagicNumber.h r=me

Comment on attachment 257688 [details] rename WASMFormat.h to WASMMagicNumber.h Clearing flags on attachment: 257688 Committed r187531: <http://trac.webkit.org/changeset/187531>

All reviewed patches have been landed. Closing bug.

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]

Re-opened since this is blocked by bug 147397

Created attachment 257738 [details] Reupload the patch since r187539 should fix the JSWASMModule.h issue in the Windows build.

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

Created attachment 257758 [details] Rebase to ToT.

Comment on attachment 257758 [details] Rebase to ToT. Try landing again via commit queue.

Comment on attachment 257758 [details] Rebase to ToT. Clearing flags on attachment: 257758 Committed r187550: <http://trac.webkit.org/changeset/187550>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 147420

Created attachment 257894 [details] Patch

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?

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

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

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

I have verified that https://bugs.webkit.org/show_bug.cgi?id=147443 fixes the build issue on Windows.

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.

Created attachment 257946 [details] Fix ChangeLog

Comment on attachment 257946 [details] Fix ChangeLog Rubber stamp to re-land after Windows build fix.

Comment on attachment 257946 [details] Fix ChangeLog Clearing flags on attachment: 257946 Committed r187677: <http://trac.webkit.org/changeset/187677>

All reviewed patches have been landed. Closing bug.