Bug 177473

Summary:

WebAssembly: no VM / JS version of everything but Instance

Product:

WebKit

Reporter:

JF Bastien <jfbastien>

Component:

WebAssembly

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alecflett, beidson, benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, fpizlo, jfbastien, jsbell, keith_miller, mark.lam, msaboff, rmorisset, ryanhaddad, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=177568

Bug Depends on:

178031

Bug Blocks:

177472, 177887, 177892

Attachments:

Description	Flags
patch	fpizlo: review-
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none
patch	fpizlo: review-
patch	none
patch	none
patch	fpizlo: review+, fpizlo: commit-queue-
patch	none
patch	saam: review+
change since last patch	none
patch	none

Description JF Bastien 2017-09-25 16:18:52 PDT

It turns out that Instance is the central part of our implementation, and it touches everything. I'm starting #177472 with all the other things, and then doing a follow-up to untangle Instance. This means we'll have a few hacks floating around temporarily (mostly pointer copies of objects), but it'll make my work more incremental.

Comment 1 JF Bastien 2017-09-25 17:11:21 PDT

Created attachment 321775 [details]
patch

Comment 2 JF Bastien 2017-09-25 17:13:14 PDT

Comment on attachment 321775 [details]
patch

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

> Source/JavaScriptCore/API/WebAssembly.h:37
> +#include <JavaScriptCore/WATableDescriptor.h>

I need to remove this!

Comment 3 Build Bot 2017-09-25 19:23:33 PDT

Attachment 321775 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:213:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:254:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:257:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:258:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:259:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:266:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:273:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:274:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:275:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:300:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:409:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:94:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmBBQPlan.h:53:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmBBQPlan.h:54:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmBBQPlan.h:57:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.h:104:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.h:105:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.h:106:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/wasm/WasmCodeBlock.h:57:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmCodeBlock.h:57:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmCodeBlock.h:57:  The parameter name "moduleInformation" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:560:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmModule.h:52:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmModule.h:53:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmThunks.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/WebAssembly.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
Total errors found: 35 in 105 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 4 Filip Pizlo 2017-09-25 19:34:42 PDT

Comment on attachment 321775 [details]
patch

R- since it seems like you set the bots on fire.

Comment 5 JF Bastien 2017-09-25 21:56:28 PDT

Created attachment 321790 [details]
patch

- Fix include with should have been private.
- Fix WebKit style where sensible.
- Remove API header. That's for a later date.

Comment 6 Build Bot 2017-09-25 21:59:58 PDT

Attachment 321790 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:410:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 7 JF Bastien 2017-09-25 23:28:28 PDT

Created attachment 321800 [details]
patch

Fix build issues again. Turns out some of our headers are included in places I didn't expect! Just building JSC isn't sufficient.

Comment 8 Build Bot 2017-09-25 23:30:04 PDT

Attachment 321800 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 108 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 9 JF Bastien 2017-09-26 09:07:21 PDT

Created attachment 321822 [details]
patch

Add missing forwarding header.

Comment 10 Build Bot 2017-09-26 09:10:05 PDT

Attachment 321822 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 11 JF Bastien 2017-09-26 09:56:19 PDT

Created attachment 321828 [details]
patch

Missing export. Linker was sad.

Comment 12 Build Bot 2017-09-26 09:58:33 PDT

Attachment 321828 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 13 JF Bastien 2017-09-26 10:09:07 PDT

Created attachment 321831 [details]
patch

Fix 32-bit build, it has WebAssembly disabled and I'd added an unguarded field in VM.

Comment 14 Build Bot 2017-09-26 10:11:45 PDT

Attachment 321831 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 15 JF Bastien 2017-09-26 10:29:11 PDT

Created attachment 321837 [details]
patch

More 32-bit build fix...

Comment 16 Build Bot 2017-09-26 10:33:00 PDT

Attachment 321837 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 17 JF Bastien 2017-09-26 10:42:53 PDT

Created attachment 321839 [details]
patch

Another 32-bit build fix.


The Windows and WPE problems are due to a file move, and unified build's Sources.txt not getting rebuilt. Keith is looking into that.

Comment 18 Build Bot 2017-09-26 10:45:38 PDT

Attachment 321839 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 19 Filip Pizlo 2017-09-27 09:33:31 PDT

Comment on attachment 321839 [details]
patch

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

I think it needs a few stylistic changes.

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:155
> +    m_frame.m_CallerEntryFrame = m_frame.m_entryFrame;

Seems like Caller should be caller here.

> Source/JavaScriptCore/interpreter/StackVisitor.h:117
> +        EntryFrame* m_CallerEntryFrame;

caller

> Source/JavaScriptCore/jit/AssemblyHelpers.h:370
> +    void copyCalleeSavesToEntryFrameCalleeSavesBuffer(EntryFrame** topEntryFrame, const TempRegisterSet& usedRegisters = { RegisterSet::stubUnavailableRegisters() })

Strongly recommend making this EntryFrame*&.  JSC is fuzzy about when we do * and when we do &.  But this checks off some & boxes for me:

- Never null.
- Nobody is going to ever repoint this pointer.
- There is no existing convention that we should point to EntryFrame* instead of referencing it (for example we prefer to point to ExecState rather than reference it, therefore we almost universally say ExecState* to be consistent)

I don't know if these three rules are something we all agree upon, but it seems that these three rules most easily explain JSC's existing uses of & and *.  So, I think you should EntryFrame*& here.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:555
> +    int32_t (*growMemory)(JSWebAssemblyInstance*, int32_t) = [] (JSWebAssemblyInstance* instance, int32_t delta) -> int32_t {

I think this is an opportunity to say "auto growMemory" here instead.  I think it's important to do that, because that means fewer churn if we decide to make this capture things.

> Source/JavaScriptCore/wasm/WasmTable.cpp:104
> +    Checked newSizeChecked = size();
> +    newSizeChecked += delta;
> +    uint32_t newSize;
> +    if (newSizeChecked.safeGet(newSize) == CheckedState::DidOverflow)
> +        return std::nullopt;
> +
> +    if (maximum() && newSize > *maximum())
> +        return std::nullopt;
> +    if (!isValidSize(newSize))
> +        return std::nullopt;
> +
> +    {
> +        Checked reallocFunctionsSizeChecked = newSizeChecked;
> +        reallocFunctionsSizeChecked *= sizeof(CallableFunction);
> +        uint32_t reallocFunctionsSize;
> +        if (reallocFunctionsSizeChecked.safeGet(reallocFunctionsSize) == CheckedState::DidOverflow)
> +            return std::nullopt;
> +        m_functions.realloc(reallocFunctionsSize);
> +        for (uint32_t i = m_size; i < newSize; ++i)
> +            new (&m_functions.get()[i]) Wasm::CallableFunction();
> +    }
> +    
> +    {
> +        Checked reallocInstancesSizeChecked = newSizeChecked;
> +        reallocInstancesSizeChecked *= sizeof(JSWebAssemblyInstance*);
> +        uint32_t reallocInstancesSize;
> +        if (reallocInstancesSizeChecked.safeGet(reallocInstancesSize) == CheckedState::DidOverflow)
> +            return std::nullopt;
> +        m_instances.realloc(reallocInstancesSize);
> +        for (uint32_t i = m_size; i < newSize; ++i)
> +            m_instances.get()[i] = nullptr;
> +    }
> +
> +    m_size = newSize;

Can all of this be Vector<>?

If not Vector<>, can you do something to reduce the deja vu?

Comment 20 JF Bastien 2017-09-27 12:23:54 PDT

Created attachment 321995 [details]
patch

Fixed style comments.

> > Source/JavaScriptCore/jit/AssemblyHelpers.h:370
> > +    void copyCalleeSavesToEntryFrameCalleeSavesBuffer(EntryFrame** topEntryFrame, const TempRegisterSet& usedRegisters = { RegisterSet::stubUnavailableRegisters() })
> 
> Strongly recommend making this EntryFrame*&.  JSC is fuzzy about when we do
> * and when we do &.  But this checks off some & boxes for me:
> 
> - Never null.
> - Nobody is going to ever repoint this pointer.
> - There is no existing convention that we should point to EntryFrame*
> instead of referencing it (for example we prefer to point to ExecState
> rather than reference it, therefore we almost universally say ExecState* to
> be consistent)
> 
> I don't know if these three rules are something we all agree upon, but it
> seems that these three rules most easily explain JSC's existing uses of &
> and *.  So, I think you should EntryFrame*& here.

Yeah that makes a bunch of sense! Done.

> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:555
> > +    int32_t (*growMemory)(JSWebAssemblyInstance*, int32_t) = [] (JSWebAssemblyInstance* instance, int32_t delta) -> int32_t {
> 
> I think this is an opportunity to say "auto growMemory" here instead.  I
> think it's important to do that, because that means fewer churn if we decide
> to make this capture things.

Usually I would, but here we want a function pointer instead of a lambda's closure, because the address is passed to the JIT. We could use auto and coerce to function pointer with unary plus, but that's kinda obscure C++ and I like the explicit signature check this version adds.

> > Source/JavaScriptCore/wasm/WasmTable.cpp:104
[snip]
> > +
> > +    m_size = newSize;
> 
> Can all of this be Vector<>?
> 
> If not Vector<>, can you do something to reduce the deja vu?

I'm not sure vector is great here, unless we pre-allocate and never touch it again, because of the small buffer (we could disable...) and the fact that the JIT reaches in to get the buffer address.

I refactored to remove copy-pasta, but it's also kinda ugly. Less code though!

Especially ugly:
  new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();

WDYT?

I could just add a `value_type` typedef to MallocPtr to avoid the ugly stuff there.

Comment 21 Build Bot 2017-09-27 12:27:02 PDT

Attachment 321995 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 11 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 22 JF Bastien 2017-09-27 13:35:23 PDT

Created attachment 322012 [details]
patch

Fix 32-bit build.

Comment 23 Build Bot 2017-09-27 13:37:56 PDT

Attachment 322012 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 11 in 110 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 24 JF Bastien 2017-09-27 14:48:27 PDT

FWIW this would make the ugly code nicer: https://bugs.webkit.org/show_bug.cgi?id=177568

Comment 25 JF Bastien 2017-09-28 11:34:43 PDT

*** Bug 177568 has been marked as a duplicate of this bug. ***

Comment 26 JF Bastien 2017-09-28 11:44:24 PDT

Created attachment 322103 [details]
patch

Update patch to contain the cleanup I suggested, instead of adding it to WTF separately. Looks much nicer now.

Comment 27 Build Bot 2017-09-28 11:47:53 PDT

Attachment 322103 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ChangeLog:26:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:28:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:34:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:36:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:39:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:41:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:43:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:48:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:50:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:53:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:61:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:63:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:71:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:78:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:80:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:90:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:91:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:100:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:101:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:121:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StdLibExtras.h:164:  default_construct_at is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 32 in 113 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 28 Filip Pizlo 2017-10-02 10:30:58 PDT

Comment on attachment 322103 [details]
patch

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

r=me with comments

> Source/JavaScriptCore/wasm/WasmMemory.cpp:302
> -        return adoptRef(new Memory(initial, maximum));
> +        return adoptRef(new Memory(initial, maximum, std::forward<WTF::Function<void()>>(notifyMemoryPressure), std::forward<WTF::Function<void()>>(syncTryToReclaimMemory), std::forward<WTF::Function<void(PageCount, PageCount)>>(growSuccessCallback)));

I think it's weird that you're using std::forward.  Shouldn't this be WTFMove?

> Source/JavaScriptCore/wasm/WasmTable.cpp:93
> +    auto checkedGrow = [&] (auto& container) {
> +        Checked reallocSizeChecked = newSizeChecked;
> +        reallocSizeChecked *= sizeof(*container.get());
> +        uint32_t reallocSize;
> +        if (reallocSizeChecked.safeGet(reallocSize) == CheckedState::DidOverflow)
> +            return false;
> +        container.realloc(reallocSize);
> +        for (uint32_t i = m_size; i < newSize; ++i)
> +            default_construct_at(&container.get()[i]);
> +        return true;
> +    };

Nice

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344
> +            RefPtr<Wasm::Memory> memory = Wasm::Memory::create(moduleInformation.memory.initial(), moduleInformation.memory.maximum(),
> +                [&vm] () { vm.heap.collectAsync(CollectionScope::Full); },
> +                [&vm] () { vm.heap.collectSync(CollectionScope::Full); },
> +                [&vm, jsMemory] (Wasm::PageCount oldPageCount, Wasm::PageCount newPageCount) { jsMemory->growSuccessCallback(vm, oldPageCount, newPageCount); });

That's pretty cool!

The one downside is that it's hard to tell what each callback is for.  One way to make this easy to read is to introduce dummy enums for each WTF::Function:

enum NotifyLowMemory { NotifyLowMemoryTag }
enum SyncTryToReclaim { SyncTryToReclaimTag }
enum GrowMemory { GrowMemoryTag }

And then this would do:

RefPtr<...> memory = ...(...,
    [&vm] (NotifyLowMemory) { vm.heap.collectAsync(...); },
    [&vm] (SyncTryToRedlaim) ...

I don't feel strongly about it, but it might be nice.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:445
> +                jit.purifyNaN(fprReg);
> +                jit.moveDoubleTo64(fprReg, scratch);
> +                materializeDoubleEncodeOffset(doubleEncodeOffsetGPRReg);
> +                jit.add64(doubleEncodeOffsetGPRReg, scratch);
> +                jit.store64(scratch, calleeFrame.withOffset(calleeFrameOffset));
> +                calleeFrameOffset += sizeof(Register);

Looks very similar to the code above.  Can you create a helper lambda or something?

Comment 29 JF Bastien 2017-10-02 15:28:42 PDT

Created attachment 322458 [details]
patch

Address all of Fil's comments.

Comment 30 Saam Barati 2017-10-02 18:13:31 PDT

Comment on attachment 322103 [details]
patch

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

r=me too
My meta comment here is I think this patch would be a lot easier to review if you had split it up.

> Source/JavaScriptCore/ChangeLog:30
> +            purpose: one to notify of memory pressure, and the other to ask for

what do you mean by memory pressure here?

> Source/JavaScriptCore/ChangeLog:80
> +          * WasmMemory doens't need to know about fault handling thunks. Only the IR

typo: doens't

> Source/JavaScriptCore/ChangeLog:104
> +            entrypoint. This triples the space allocated per instance's imported
> +            function, but there shouldn't be that many imports. This has two upsides: it

Triples seems like a lot if there are a lot of these. I thought Emscripten generates a *ton* of imports?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:372
> +        // Most memory accesses in signaling mode don't do an explicit exception check because they can rely on fault handling to detect out-of-bounds accesses. FaultSignalHandler nonetheless needs the thunk to exist so that it can jump to that thunk.

Style nit: this comment should be on more than one line

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:388
> +            // FIXME: Because WasmToWasm call clobbers wasmContextInstance register and does not restore it, we need to restore it in the caller side.

wasmContextInstance? Isn't that redundant? Ditto elsewhere you do this.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:477
> +            jit.loadPtr(CCallHelpers::Address(baseMemory, Wasm::Memory::offsetOfSize()), sizeRegs[0].sizeRegister);
> +            jit.loadPtr(CCallHelpers::Address(baseMemory, Wasm::Memory::offsetOfMemory()), baseMemory);

Aren't we in the Wasm namespace here? If so, no need for Wasm::

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1206
> +    // FIXME: when we have trap handlers, we can just let the call fail because Signature::invalidIndex is 0. https://bugs.webkit.org/show_bug.cgi?id=177210

How does this have anything to do with signature index? It seems like this only has to do with the code pointer we load.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1272
> +            jit.loadPtr(CCallHelpers::Address(baseMemory, Wasm::Memory::offsetOfSize()), sizeRegs[0].sizeRegister); // Memory size.
> +            jit.loadPtr(CCallHelpers::Address(baseMemory, Wasm::Memory::offsetOfMemory()), baseMemory); // Wasm::Memory::void*.

ditto about namespace

> Source/JavaScriptCore/wasm/WasmContext.cpp:57
> +void Context::store(JSWebAssemblyInstance* i, void* softStackLimit)

style: We usually don't use 1 letter variable names in code like this.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:302
>> +        return adoptRef(new Memory(initial, maximum, std::forward<WTF::Function<void()>>(notifyMemoryPressure), std::forward<WTF::Function<void()>>(syncTryToReclaimMemory), std::forward<WTF::Function<void(PageCount, PageCount)>>(growSuccessCallback)));
> 
> I think it's weird that you're using std::forward.  Shouldn't this be WTFMove?

agreed

> Source/JavaScriptCore/wasm/WasmMemory.cpp:331
> +        return adoptRef(new Memory(fastMemory, initial, maximum, Memory::fastMappedBytes(), MemoryMode::Signaling, std::forward<WTF::Function<void()>>(notifyMemoryPressure), std::forward<WTF::Function<void()>>(syncTryToReclaimMemory), std::forward<WTF::Function<void(PageCount, PageCount)>>(growSuccessCallback)));

ditto here

> Source/JavaScriptCore/wasm/WasmMemory.cpp:346
> +    return adoptRef(new Memory(slowMemory, initial, maximum, initialBytes, MemoryMode::BoundsChecking, std::forward<WTF::Function<void()>>(notifyMemoryPressure), std::forward<WTF::Function<void()>>(syncTryToReclaimMemory), std::forward<WTF::Function<void(PageCount, PageCount)>>(growSuccessCallback)));

and here

> Source/JavaScriptCore/wasm/WasmMemory.h:78
> +    enum class GrowFailReason {
> +        InvalidDelta,
> +        InvalidGrowSize,
> +        WouldExceedMaximum,
> +        OutOfMemory,
> +    };
> +    Expected<PageCount, GrowFailReason> grow(PageCount);

nice

> Source/JavaScriptCore/wasm/WasmMemory.h:99
> +    WTF::Function<void()> m_notifyMemoryPressure;
> +    WTF::Function<void()> m_syncTryToReclaimMemory;
> +    WTF::Function<void(PageCount, PageCount)> m_growSuccessCallback;

Do we really want to tie these to a Memory and not a call to grow()?

> Source/JavaScriptCore/wasm/WasmMemoryMode.h:34
> +namespace JSC {
> +
> +namespace Wasm {

This can go on one line if you want

> Source/JavaScriptCore/wasm/WasmTable.h:47
> +    enum class CreateFail {
> +        InvalidSize,
> +    };

This isn't used

> Source/JavaScriptCore/wasm/WasmThunks.cpp:94
> +    typedef void (*Run)(JSWebAssemblyInstance*, uint32_t);
> +    Run run = OMGPlan::runForIndex;

Is this just for documenting the type?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:62
> +    for (unsigned i = 0; i < m_numImportFunctions; ++i)
> +        new (importFunctionInfo(i)) ImportFunctionInfo();

You can use default_construct_at if you'd like

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344
>> +                [&vm, jsMemory] (Wasm::PageCount oldPageCount, Wasm::PageCount newPageCount) { jsMemory->growSuccessCallback(vm, oldPageCount, newPageCount); });
> 
> That's pretty cool!
> 
> The one downside is that it's hard to tell what each callback is for.  One way to make this easy to read is to introduce dummy enums for each WTF::Function:
> 
> enum NotifyLowMemory { NotifyLowMemoryTag }
> enum SyncTryToReclaim { SyncTryToReclaimTag }
> enum GrowMemory { GrowMemoryTag }
> 
> And then this would do:
> 
> RefPtr<...> memory = ...(...,
>     [&vm] (NotifyLowMemory) { vm.heap.collectAsync(...); },
>     [&vm] (SyncTryToRedlaim) ...
> 
> I don't feel strongly about it, but it might be nice.

Or you could just give all these lambdas names. That's usually what I do in this situation. The nice thing about Fil's suggestion is the type checker will catch a bug if you provide the wrong lambda.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:82
> +    ImportFunctionInfo* importFunctionInfo(size_t importFunctionNum) { return &bitwise_cast<ImportFunctionInfo*>(bitwise_cast<char*>(this) + offsetOfTail())[importFunctionNum]; }
> +    static size_t offsetOfTargetInstance(size_t importFunctionNum) { return offsetOfTail() + importFunctionNum * sizeof(ImportFunctionInfo) + OBJECT_OFFSETOF(ImportFunctionInfo, targetInstance); }
> +    static size_t offsetOfWasmEntrypoint(size_t importFunctionNum) { return offsetOfTail() + importFunctionNum * sizeof(ImportFunctionInfo) + OBJECT_OFFSETOF(ImportFunctionInfo, wasmEntrypoint); }
> +    static size_t offsetOfImportFunction(size_t importFunctionNum) { return offsetOfTail() + importFunctionNum * sizeof(ImportFunctionInfo) + OBJECT_OFFSETOF(ImportFunctionInfo, importFunction); }
> +    JSObject* importFunction(unsigned importFunctionNum) { RELEASE_ASSERT(importFunctionNum < m_numImportFunctions); return importFunctionInfo(importFunctionNum)->importFunction.get(); }

oh boy

> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:65
> +

please remove

> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:124
> +const Wasm::ModuleInformation& JSWebAssemblyModule::moduleInformation() const
> +{
> +    return m_module->moduleInformation();
> +}
> +
> +SymbolTable* JSWebAssemblyModule::exportSymbolTable() const
> +{
> +    return m_exportSymbolTable.get();
> +}
> +
> +Wasm::SignatureIndex JSWebAssemblyModule::signatureIndexFromFunctionIndexSpace(unsigned functionIndexSpace) const
> +{
> +    return m_module->signatureIndexFromFunctionIndexSpace(functionIndexSpace);
> +}
> +
> +WebAssemblyToJSCallee* JSWebAssemblyModule::callee() const
> +{
> +    return m_callee.get();
> +}
> +
> +JSWebAssemblyCodeBlock* JSWebAssemblyModule::codeBlock(Wasm::MemoryMode mode)
> +{
> +    return m_codeBlocks[static_cast<size_t>(mode)].get();
> +}
> +
> +Wasm::Module& JSWebAssemblyModule::module()
> +{
> +    return m_module.get();
> +}

these all seem like the should be inlined. Are they only called from the cpp file?

Comment 31 Build Bot 2017-10-02 22:34:47 PDT

Attachment 322458 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StdLibExtras.h:164:  default_construct_at is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 12 in 114 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 32 WebKit Commit Bot 2017-10-03 11:33:19 PDT

Comment on attachment 322458 [details]
patch

Clearing flags on attachment: 322458

Committed r222791: <http://trac.webkit.org/changeset/222791>

Comment 33 WebKit Commit Bot 2017-10-03 11:33:22 PDT

All reviewed patches have been landed.  Closing bug.

Comment 34 Radar WebKit Bug Importer 2017-10-03 11:34:42 PDT

<rdar://problem/34794915>

Comment 35 JF Bastien 2017-10-04 11:21:55 PDT

Will follow-up Saam's comments in https://bugs.webkit.org/show_bug.cgi?id=177887

Comment 36 WebKit Commit Bot 2017-10-06 15:10:29 PDT

Re-opened since this is blocked by bug 178031

Comment 37 Ryan Haddad 2017-10-06 15:14:16 PDT

*** Bug 177901 has been marked as a duplicate of this bug. ***

Comment 38 JF Bastien 2017-10-19 15:48:10 PDT

Created attachment 324306 [details]
patch

Fix the bug that caused a revert: moving the wasm->JS entries to the Wasm::CodeBlock (from the JSWebAssemblyCodeBlock) was invalid because they have the address of the VM baked into them. They're now on the JSWebAssemblyInstance instead.

Comment 39 JF Bastien 2017-10-19 15:48:53 PDT

Created attachment 324307 [details]
change since last patch

Changes since last patch, so it's easier to read.

Comment 40 Build Bot 2017-10-19 15:52:02 PDT

Attachment 324306 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:349:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StdLibExtras.h:164:  default_construct_at is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 12 in 115 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 41 Saam Barati 2017-10-19 16:04:02 PDT

Comment on attachment 324307 [details]
change since last patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:79
> +    ImportFunctionInfo* importFunctionInfo(size_t importFunctionNum) { RELEASE_ASSERT(importFunctionNum < m_numImportFunctions); return &bitwise_cast<ImportFunctionInfo*>(bitwise_cast<char*>(this) + offsetOfTail())[importFunctionNum]; }

Let's put this on two lines.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:55
> +static Expected<MacroAssemblerCodeRef, BindingFailure> handleBadI64Use(VM* vm, JIT& jit, const Signature& signature, unsigned importIndex)

why add this function? I think it just complicates reading this code for no reason.

Comment 42 Saam Barati 2017-10-19 16:05:49 PDT

Comment on attachment 324306 [details]
patch

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

r=me too

> Source/WTF/wtf/StdLibExtras.h:167
> +template<typename T>
> +inline T& default_construct_at(T* addr)
> +{
> +    return *new (addr) T();
> +}

I'd vote for removing this since nobody seemed to like it in the other patch you opened.

Comment 43 JF Bastien 2017-10-19 18:42:19 PDT

Created attachment 324328 [details]
patch

(In reply to Saam Barati from comment #41)
> Comment on attachment 324307 [details]
> change since last patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324307&action=review
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:79
> > +    ImportFunctionInfo* importFunctionInfo(size_t importFunctionNum) { RELEASE_ASSERT(importFunctionNum < m_numImportFunctions); return &bitwise_cast<ImportFunctionInfo*>(bitwise_cast<char*>(this) + offsetOfTail())[importFunctionNum]; }
> 
> Let's put this on two lines.

Done.

> > Source/JavaScriptCore/wasm/js/WasmToJS.cpp:55
> > +static Expected<MacroAssemblerCodeRef, BindingFailure> handleBadI64Use(VM* vm, JIT& jit, const Signature& signature, unsigned importIndex)
> 
> why add this function? I think it just complicates reading this code for no
> reason.

That function is just too huge, and it really does 3 things. I split off this one because I was having a hard time paging through it and that bit was irrelevant to what I was doing. I really should split what's left in 2... I really only removed 50 lines from a 600 line function.

> > Source/WTF/wtf/StdLibExtras.h:167
> > +template<typename T>
> > +inline T& default_construct_at(T* addr)
> > +{
> > +    return *new (addr) T();
> > +}
> 
> I'd vote for removing this since nobody seemed to like it in the other patch
> you opened.

Good point, removed. I put back my ugly / amazing `new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();`.

Comment 44 Build Bot 2017-10-19 18:44:02 PDT

Attachment 324328 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:400:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ForwardingHeaders/wasm/WasmModule.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/wasm/WasmEmbedder.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:349:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTable.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSToWasm.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:67:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:82:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 11 in 113 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 45 WebKit Commit Bot 2017-10-19 19:23:35 PDT

Comment on attachment 324328 [details]
patch

Clearing flags on attachment: 324328

Committed r223738: <https://trac.webkit.org/changeset/223738>

Comment 46 WebKit Commit Bot 2017-10-19 19:23:37 PDT

All reviewed patches have been landed.  Closing bug.

Comment 47 JF Bastien 2017-10-19 21:20:31 PDT

*** Bug 177887 has been marked as a duplicate of this bug. ***