Bug 177473 – WebAssembly: no VM / JS version of everything but Instance

• Component: WebAssembly
• Status: RESOLVED FIXED

[JF Bastien]

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.

[JF Bastien]

Created attachment 321775 [details]
patch

[JF Bastien]

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!

[Build Bot]

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.

[Filip Pizlo]

Comment on attachment 321775 [details]
patch

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

[JF Bastien]

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.

[Build Bot]

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.

[JF Bastien]

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.

[Build Bot]

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.

[JF Bastien]

Created attachment 321822 [details]
patch

Add missing forwarding header.

[Build Bot]

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.

[JF Bastien]

Created attachment 321828 [details]
patch

Missing export. Linker was sad.

[Build Bot]

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.

[JF Bastien]

Created attachment 321831 [details]
patch

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

[Build Bot]

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.

[JF Bastien]

Created attachment 321837 [details]
patch

More 32-bit build fix...

[Build Bot]

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.

[JF Bastien]

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.

[Build Bot]

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.

[Filip Pizlo]

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?

[JF Bastien]

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.

[Build Bot]

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.

[JF Bastien]

Created attachment 322012 [details]
patch

Fix 32-bit build.

[Build Bot]

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.

[JF Bastien]

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

[JF Bastien]

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

[JF Bastien]

Created attachment 322103 [details]
patch

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

[Build Bot]

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.

[Filip Pizlo]

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?

[JF Bastien]

Created attachment 322458 [details]
patch

Address all of Fil's comments.

[Saam Barati]

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?

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 322458 [details]
patch

Clearing flags on attachment: 322458

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34794915>

[JF Bastien]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 178031

[Ryan Haddad]

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

[JF Bastien]

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.

[JF Bastien]

Created attachment 324307 [details]
change since last patch

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

[Build Bot]

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.

[Saam Barati]

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.

[Saam Barati]

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.

[JF Bastien]

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())>();`.

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 324328 [details]
patch

Clearing flags on attachment: 324328

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[JF Bastien]

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