Bug 198398

Summary: [WASM-References] Add support for Anyref tables, Table.get and Table.set (for Anyref only).
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: JavaScriptCoreAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 198506    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Patch
none
Patch none

Description Justin Michaud 2019-05-30 16:57:51 PDT
Add support for Anyref tables, Table.get and Table.set (for Anyref only). Support for making Anyrefs from Funcrefs is out of scope for this patch.
Comment 1 Justin Michaud 2019-05-30 17:05:44 PDT
Created attachment 370997 [details]
Patch
Comment 2 Justin Michaud 2019-05-30 17:15:31 PDT
Comment on attachment 370997 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:956
> +    ASSERT(idx.tmp());

I was hoping to avoid emitting the assembly for this until we nail down how it is going to work with funcrefs. Thoughts?

> Source/JavaScriptCore/wasm/WasmTable.cpp:97
> +        auto& subThis = *static_cast<FuncRefTable*>(this);

Not sure how to make this cleaner.

> Source/JavaScriptCore/wasm/WasmTable.cpp:118
> +        auto& subThis = *static_cast<FuncRefTable*>(this);
> +        subThis.m_importableFunctions.get()[index & m_mask] = WasmToWasmImportableFunction();
> +        ASSERT(subThis.m_importableFunctions.get()[index & m_mask].signatureIndex == Wasm::Signature::invalidIndex); // We rely on this in compiled code.
> +        subThis.m_instances.get()[index & m_mask] = nullptr;

same

> Source/JavaScriptCore/wasm/WasmTable.cpp:127
> +    RELEASE_ASSERT(m_type == Wasm::Anyref);

I feel like there should be a more obvious way for the interface to explain this to the caller.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:67
> +    m_table->setOwner(this);

It seems to me that we always have a JSWasmTable wrapping the table (and the patch relies on this) if we are in a JS embedding. Can anyone confirm?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:107
>  {

The vm param is no longer needed, although I was wondering if there are any cases where this vm is different from the one in the function JSObject.

> JSTests/wasm/references/validation.js:89
> +    assert.throws(() => new WebAssembly.Module(bin.get()), WebAssembly.CompileError, "WebAssembly.Module doesn't validate: table.get expects the table to have type Anyref, in function at index 0 (evaluating 'new WebAssembly.Module(bin.get())')");

This is temporary, until we support funcrefs.
Comment 3 Justin Michaud 2019-05-30 17:22:41 PDT
Created attachment 371001 [details]
Patch
Comment 4 EWS Watchlist 2019-05-30 17:26:48 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-05-30 18:32:08 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-05-30 18:32:09 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-05-30 19:12:48 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-05-30 19:12:49 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-05-30 19:21:54 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-05-30 21:23:48 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-05-30 21:23:50 PDT Comment hidden (obsolete)
Comment 12 Saam Barati 2019-06-03 14:19:14 PDT
Comment on attachment 371001 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmTable.cpp:72
> +        return adoptRef(new (NotNull, fastMalloc(sizeof(Table))) FuncRefTable(initial, maximum));

This is your bug probably. sizeof(FuncRefTable)?
Comment 13 Justin Michaud 2019-06-03 15:44:12 PDT
Created attachment 371221 [details]
Patch
Comment 14 Saam Barati 2019-06-03 16:25:58 PDT
Comment on attachment 371221 [details]
Patch

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

Mostly LGTM. Some comments and suggestions below

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:570
> +{

let's also open a bugzilla about emitting this code inline.

> Source/JavaScriptCore/wasm/WasmFormat.h:217
> +    TableInformation(uint32_t initial, Optional<uint32_t> maximum, bool isImport, int8_t type)

weird to use uint8_t here when type is int32. Can we just use a bit to represent isFuncRef vs isAnyRef. Or maybe use an enum?

> Source/JavaScriptCore/wasm/WasmTable.cpp:62
> +        new(&m_jsValues.get()[i]) WriteBarrier<Unknown>();

style: space between "new" and "("

> Source/JavaScriptCore/wasm/WasmTable.cpp:74
> +        return adoptRef(new (NotNull, fastMalloc(sizeof(FuncRefTable))) FuncRefTable(initial, maximum));
> +    if (type == Wasm::Anyref)
> +        return adoptRef(new (NotNull, fastMalloc(sizeof(Table))) Table(initial, maximum, Wasm::Anyref));

Why aren't these just WTF_MAKE_FAST_ALLOCATED classes? It's weird you're using placement new here, since WTF_MAKE_FAST_ALLOCATED does this for you. This can just be
adoptRef(new FuncRefTable(initial, maximum)

> Source/JavaScriptCore/wasm/WasmTable.cpp:117
> +        auto& subThis = *static_cast<FuncRefTable*>(this);

not a fan of the name "subThis". Can we just use a name based on the type? Maybe funcRefTable?

> Source/JavaScriptCore/wasm/WasmTable.h:62
> +    template<typename T>
> +    static inline bool checkedGrow(uint32_t length, uint32_t newLength, T& container) WARN_UNUSED_RETURN {
> +        if (newLength > allocatedLength(length)) {
> +            Checked<uint32_t, RecordOverflow> reallocSizeChecked = allocatedLength(newLength);
> +            reallocSizeChecked *= sizeof(*container.get());
> +            uint32_t reallocSize;
> +            if (reallocSizeChecked.safeGet(reallocSize) == CheckedState::DidOverflow)
> +                return false;
> +            // FIXME this over-allocates and could be smarter about not committing all of that memory https://bugs.webkit.org/show_bug.cgi?id=181425
> +            container.realloc(reallocSize);
> +        }
> +        for (uint32_t i = length; i < allocatedLength(newLength); ++i)
> +            new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();
> +        return true;
> +    }

I don't think this needs to be in the header since it's just called from the cpp file.

Also, why did the lambda stop working?

> Source/JavaScriptCore/wasm/WasmTable.h:75
> +    void setOwner(JSObject* instance) { m_owner = instance; }

Can we assert that m_owner is null prior?

> Source/JavaScriptCore/wasm/WasmTable.h:78
> +    JSValue get(uint32_t);

const?

> Source/JavaScriptCore/wasm/WasmTable.h:86
> +    WTF_MAKE_NONCOPYABLE(Table);

We tend to put this as the first thing in a class declaration. Also, as mentioned in other comments, you should just make this WTF_MAKE_FAST_ALLOCATED

> Source/JavaScriptCore/wasm/WasmTable.h:96
> +    JSObject* m_owner;

initialized to nullptr?

> Source/JavaScriptCore/wasm/WasmTable.h:98
> +    ConcurrentJSLock m_lock;

We could just hold the lock in the owner FWIW.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:83
> +    thisObject->table()->visitChildren(visitor);

You could hold cellLock here instead of having a lock in the non-cell class.

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:69
> +        if (elementString != "anyfunc" && elementString != "anyref")
> +            return JSValue::encode(throwException(exec, throwScope, createTypeError(exec, "WebAssembly.Table expects its 'element' field to be the string 'anyfunc' or 'anyref'"_s)));

Do we have tests for this?

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:70
> +        type = elementString == "anyfunc" ? Wasm::Anyfunc : Wasm::Anyref;

nit: I don't think we will actually run this string compare twice, but maybe it's worth caching the result in local variable anyways.

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:131
> +    if (table->table()->type() == Wasm::Anyfunc) {

Can we abstract this in a function like "isAnyFuncTable()" or something, so we don't compare against this type constant in various places (which easily allows us to change how it's stored).

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:149
> +    } else {
> +        table->set(vm, index, value);
>      }

style nit: no braces.
Comment 15 Justin Michaud 2019-06-03 17:08:19 PDT
Comment on attachment 371221 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:570
>> +{
> 
> let's also open a bugzilla about emitting this code inline.

<https://bugs.webkit.org/show_bug.cgi?id=198506>
Comment 16 Justin Michaud 2019-06-03 20:40:01 PDT
Created attachment 371242 [details]
Patch
Comment 17 EWS Watchlist 2019-06-03 21:48:40 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-06-03 21:48:42 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-06-03 21:55:40 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-06-03 21:55:42 PDT Comment hidden (obsolete)
Comment 21 Saam Barati 2019-06-03 22:16:13 PDT
Seems like you have some real test failures
Comment 22 EWS Watchlist 2019-06-03 22:28:31 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2019-06-03 22:30:14 PDT Comment hidden (obsolete)
Comment 24 EWS Watchlist 2019-06-03 22:30:16 PDT Comment hidden (obsolete)
Comment 25 Saam Barati 2019-06-04 00:04:44 PDT
Comment on attachment 371242 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmFormat.h:57
> +enum class TableElementType {

Should be: enum class TableElementType : uint8_t

> Source/JavaScriptCore/wasm/WasmTable.cpp:115
> +    if (isFuncrefTable()) {

What about abstracting this pattern into downcast form? Like adding this to Table

FuncRedTable* asFuncRefTable() { return m_type == funcRed? cast(this) : nullptr }

Then, call sites can be:
if (auto* funcRefTable = table->asFuncRefTable()) ...

> Source/JavaScriptCore/wasm/WasmTable.cpp:133
> +    if (isFuncrefTable()) {

That pattern could be used here too

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:50
> +    auto table = Wasm::Table::tryCreate(initial, maximum, (JSObject*) cell, type);

Style nit: static_cast instead of C style cast

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:53
> +        return nullptr;

My bet is this leaves cell in a non desirable state from GC perspective. It would be good to have a test. I’d recommend just setting owner after fully constructing the JSWasmTable. Test might be slightly tricky as this will only be pointed to via a conservative GC scan.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:114
> +    RELEASE_ASSERT(m_table->isAnyrefTable());

Maybe this is why you’re crashing. I think you mean the opposite, you want  is anyFuncTable
Comment 26 Justin Michaud 2019-06-04 11:06:39 PDT
Created attachment 371303 [details]
Patch
Comment 27 Justin Michaud 2019-06-04 13:11:57 PDT
The minibrowser entitlements were by mistake, I will remove them after review
Comment 28 Saam Barati 2019-06-05 11:25:03 PDT
Comment on attachment 371303 [details]
Patch

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

r=me with comments.

Can you also try to add a test where we'd crash if we didn't hold the lock in table.grow? Seems like it could be a pure JS test, where we repeatedly call grow on new tables in a loop, and we collect continuously.

> Source/JavaScriptCore/wasm/WasmTable.h:60
> +    void setOwner(JSObject* owner) { ASSERT(!m_owner); ASSERT(owner); m_owner = owner; }

style: let's put each statement on its own line of code.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:304
> +            RefPtr<Wasm::Table> wasmTable = Wasm::Table::tryCreate(moduleInformation.tableInformation.initial(), moduleInformation.tableInformation.maximum(), moduleInformation.tableInformation.type());

Don't you need to type check the tableImport somewhere here? E.g, if we say we're importing an Anyfunc table, but it's really an Anyref, we should throw, right?
Comment 29 Justin Michaud 2019-06-05 19:23:15 PDT
> Can you also try to add a test where we'd crash if we didn't hold the lock
> in table.grow? Seems like it could be a pure JS test, where we repeatedly
> call grow on new tables in a loop, and we collect continuously.

It crashes with DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib Malloc=1, collectContinuously is enabled, and the lock is missing.
Comment 30 Justin Michaud 2019-06-05 19:29:26 PDT
Created attachment 371467 [details]
Patch
Comment 31 Justin Michaud 2019-06-05 19:30:05 PDT
Comment on attachment 371303 [details]
Patch

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:304
>> +            RefPtr<Wasm::Table> wasmTable = Wasm::Table::tryCreate(moduleInformation.tableInformation.initial(), moduleInformation.tableInformation.maximum(), moduleInformation.tableInformation.type());
> 
> Don't you need to type check the tableImport somewhere here? E.g, if we say we're importing an Anyfunc table, but it's really an Anyref, we should throw, right?

Yep, good catch. Added a test.
Comment 32 WebKit Commit Bot 2019-06-05 20:14:54 PDT
Comment on attachment 371467 [details]
Patch

Clearing flags on attachment: 371467

Committed r246139: <https://trac.webkit.org/changeset/246139>
Comment 33 WebKit Commit Bot 2019-06-05 20:14:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Radar WebKit Bug Importer 2019-06-05 20:15:41 PDT
<rdar://problem/51467542>