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

Justin Michaud
Reported 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.
Attachments
Patch (63.38 KB, patch)
2019-05-30 17:05 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (62.94 KB, patch)
2019-05-30 17:22 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews103 for mac-highsierra (3.14 MB, application/zip)
2019-05-30 18:32 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews117 for mac-highsierra (2.95 MB, application/zip)
2019-05-30 19:12 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (3.98 MB, application/zip)
2019-05-30 21:23 PDT, EWS Watchlist 		no flags
Details
Patch (70.41 KB, patch)
2019-06-03 15:44 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (71.65 KB, patch)
2019-06-03 20:40 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-highsierra (3.17 MB, application/zip)
2019-06-03 21:48 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.84 MB, application/zip)
2019-06-03 21:55 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews114 for mac-highsierra (2.99 MB, application/zip)
2019-06-03 22:30 PDT, EWS Watchlist 		no flags
Details
Patch (69.82 KB, patch)
2019-06-04 11:06 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (71.19 KB, patch)
2019-06-05 19:29 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.
Justin Michaud
Comment 1 2019-05-30 17:05:44 PDT
Created attachment 370997 [details] Patch
Justin Michaud
Comment 2 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.
Justin Michaud
Comment 3 2019-05-30 17:22:41 PDT
Created attachment 371001 [details] Patch
EWS Watchlist
Comment 4 2019-05-30 17:26:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2019-05-30 18:32:08 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2019-05-30 18:32:09 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2019-05-30 19:12:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 8 2019-05-30 19:12:49 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 9 2019-05-30 19:21:54 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 10 2019-05-30 21:23:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 11 2019-05-30 21:23:50 PDT Comment hidden (obsolete)
Saam Barati
Comment 12 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)?
Justin Michaud
Comment 13 2019-06-03 15:44:12 PDT
Created attachment 371221 [details] Patch
Saam Barati
Comment 14 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.
Justin Michaud
Comment 15 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>
Justin Michaud
Comment 16 2019-06-03 20:40:01 PDT
Created attachment 371242 [details] Patch
EWS Watchlist
Comment 17 2019-06-03 21:48:40 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 18 2019-06-03 21:48:42 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 19 2019-06-03 21:55:40 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 20 2019-06-03 21:55:42 PDT Comment hidden (obsolete)
Saam Barati
Comment 21 2019-06-03 22:16:13 PDT
Seems like you have some real test failures
EWS Watchlist
Comment 22 2019-06-03 22:28:31 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 23 2019-06-03 22:30:14 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 24 2019-06-03 22:30:16 PDT Comment hidden (obsolete)
Saam Barati
Comment 25 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
Justin Michaud
Comment 26 2019-06-04 11:06:39 PDT
Created attachment 371303 [details] Patch
Justin Michaud
Comment 27 2019-06-04 13:11:57 PDT
The minibrowser entitlements were by mistake, I will remove them after review
Saam Barati
Comment 28 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?
Justin Michaud
Comment 29 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.
Justin Michaud
Comment 30 2019-06-05 19:29:26 PDT
Created attachment 371467 [details] Patch
Justin Michaud
Comment 31 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.
WebKit Commit Bot
Comment 32 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>
WebKit Commit Bot
Comment 33 2019-06-05 20:14:56 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 34 2019-06-05 20:15:41 PDT
<rdar://problem/51467542>
Note You need to log in before you can comment on or make changes to this bug.