Summary: | [WASM-References] Add support for Anyref tables, Table.get and Table.set (for Anyref only). | ||
---|---|---|---|
Product: | WebKit | Reporter: | Justin Michaud <justin_michaud> |
Component: | JavaScriptCore | Assignee: | 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
Justin Michaud
2019-05-30 16:57:51 PDT
Created attachment 370997 [details]
Patch
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. Created attachment 371001 [details]
Patch
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json". Comment on attachment 371001 [details] Patch Attachment 371001 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12333859 New failing tests: workers/wasm-hashset.html Created attachment 371009 [details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 371001 [details] Patch Attachment 371001 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12333957 New failing tests: workers/wasm-hashset.html Created attachment 371019 [details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 371001 [details] Patch Attachment 371001 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12333884 New failing tests: wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/table.js.wasm-no-air wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/table.js.wasm-slow-memory wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/jsapi.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/basic-element.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-slow-memory wasm.yaml/wasm/js-api/element.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/exceptions.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-slow-memory wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-collect-continuously wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-air wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-air wasm.yaml/wasm/js-api/element.js.wasm-collect-continuously wasm.yaml/wasm/js-api/element.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/table.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/context-switch.js.wasm-slow-memory wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table.js.default-wasm wasm.yaml/wasm/js-api/call-indirect.js.wasm-collect-continuously wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/linking.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-air wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/basic-element.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-call-ic wasm.yaml/wasm/js-api/element.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-call-ic wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-air wasm.yaml/wasm/js-api/call-indirect.js.default-wasm wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/table-basic.js.wasm-no-call-ic wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-call-ic wasm.yaml/wasm/modules/js-wasm-table-namespace.js.default-wasm wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.default-wasm wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.default-wasm wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-collect-continuously wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-air wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/context-switch.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/nameSection.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/exceptions.js.wasm-eager-jettison wasm.yaml/wasm/modules/js-wasm-table.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/jsapi.js.wasm-no-tls-context wasm.yaml/wasm/js-api/call-indirect.js.wasm-slow-memory wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-collect-continuously wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-collect-continuously wasm.yaml/wasm/js-api/table.js.wasm-no-tls-context wasm.yaml/wasm/js-api/call-indirect.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/left-to-right.wast.js.default-wasm wasm.yaml/wasm/function-tests/basic-element.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/element.js.wasm-eager-jettison wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-tls-context wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-call-ic wasm.yaml/wasm/js-api/table.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/jsapi.js.default-wasm wasm.yaml/wasm/js-api/element.js.default-wasm wasm.yaml/wasm/js-api/table.js.wasm-no-call-ic wasm.yaml/wasm/modules/js-wasm-table.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/table.js.wasm-collect-continuously wasm.yaml/wasm/js-api/element.js.wasm-no-air wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-air wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/table.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-air wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/jsapi.js.wasm-collect-continuously wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/context-switch.js.wasm-eager-jettison wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-eager-jettison wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-collect-continuously wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-air wasm.yaml/wasm/spec-tests/imports.wast.js.default-wasm wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-collect-continuously wasm.yaml/wasm/js-api/element.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-collect-continuously apiTests Comment on attachment 371001 [details] Patch Attachment 371001 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12335438 New failing tests: workers/wasm-hashset.html Created attachment 371029 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
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)? Created attachment 371221 [details]
Patch
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 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> Created attachment 371242 [details]
Patch
Comment on attachment 371242 [details] Patch Attachment 371242 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12369202 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html workers/wasm-hashset.html Created attachment 371245 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 371242 [details] Patch Attachment 371242 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12369208 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html workers/wasm-hashset.html Created attachment 371246 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Seems like you have some real test failures Comment on attachment 371242 [details] Patch Attachment 371242 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12369185 New failing tests: wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/nameSection.js.wasm-no-call-ic wasm.yaml/wasm/js-api/wrapper-function.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/table-basic.js.default-wasm wasm.yaml/wasm/spec-tests/call_indirect.wast.js.default-wasm wasm.yaml/wasm/function-tests/exceptions.js.wasm-no-air wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-tls-context wasm.yaml/wasm/js-api/call-indirect.js.default-wasm wasm.yaml/wasm/function-tests/context-switch.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/basic-element.js.wasm-no-air wasm.yaml/wasm/function-tests/context-switch.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-slow-memory wasm.yaml/wasm/regress/regress-191056.js.default-wasm wasm.yaml/wasm/js-api/element.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-air wasm.yaml/wasm/spec-tests/func.wast.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/exceptions.js.default-wasm wasm.yaml/wasm/function-tests/exceptions.js.wasm-slow-memory wasm.yaml/wasm/function-tests/exceptions.js.wasm-no-tls-context stress/js-to-wasm-callee-has-correct-prototype.js.default wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-slow-memory wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-no-tls-context wasm.yaml/wasm/js-api/wrapper-function.js.wasm-slow-memory wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-collect-continuously wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-air wasm.yaml/wasm/function-tests/exceptions.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/br.wast.js.default-wasm wasm.yaml/wasm/spec-tests/br_table.wast.js.wasm-eager-jettison wasm.yaml/wasm/regress/regress-191056.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.default-wasm wasm.yaml/wasm/js-api/element.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/grow-memory-2.js.default-wasm wasm.yaml/wasm/js-api/element.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/regress/regress-191056.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/call_indirect.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/context-switch.js.wasm-slow-memory wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/unreachable.wast.js.wasm-collect-continuously wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/modules/js-wasm-table.js.default-wasm wasm.yaml/wasm/spec-tests/br_table.wast.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/table-basic.js.wasm-collect-continuously wasm.yaml/wasm/js-api/wrapper-function.js.wasm-no-air wasm.yaml/wasm/js-api/call-indirect.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/unreachable.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-slow-memory wasm.yaml/wasm/function-tests/nameSection.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.default-wasm wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/exceptions.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/call_indirect.wast.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-air wasm.yaml/wasm/function-tests/nameSection.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/basic-element.js.wasm-slow-memory wasm.yaml/wasm/function-tests/basic-element.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-no-tls-context wasm.yaml/wasm/regress/regress-191056.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/br.wast.js.wasm-no-air wasm.yaml/wasm/js-api/element.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-call-ic wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-air wasm.yaml/wasm/js-api/unique-signature.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-slow-memory wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/table-basic.js.wasm-no-call-ic wasm.yaml/wasm/js-api/wrapper-function.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-air wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/call_indirect.wast.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/unique-signature.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.default-wasm wasm.yaml/wasm/js-api/element.js.default-wasm wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.default-wasm wasm.yaml/wasm/function-tests/basic-element.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/unreachable.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-call-ic wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-collect-continuously wasm.yaml/wasm/regress/regress-191056.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/table-basic.js.wasm-no-air wasm.yaml/wasm/spec-tests/br.wast.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/context-switch.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/exceptions.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/func.wast.js.default-wasm wasm.yaml/wasm/function-tests/nameSection.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/br.wast.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/exceptions.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/call_indirect.wast.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/table-basic.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/return.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/br.wast.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/br_table.wast.js.wasm-no-tls-context wasm.yaml/wasm/js-api/call-indirect.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/return.wast.js.wasm-no-call-ic wasm.yaml/wasm/js-api/wrapper-function.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/return.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/stack-overflow.js.default-wasm wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/func.wast.js.wasm-no-air wasm.yaml/wasm/spec-tests/return.wast.js.default-wasm wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/return.wast.js.wasm-eager-jettison wasm.yaml/wasm/regress/regress-191056.js.wasm-slow-memory wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-tls-context wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-collect-continuously wasm.yaml/wasm/js-api/call-indirect.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/left-to-right.wast.js.default-wasm wasm.yaml/wasm/spec-tests/linking.wast.js.default-wasm wasm.yaml/wasm/function-tests/basic-element.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/basic-element.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/unreachable.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/br_table.wast.js.default-wasm wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/func.wast.js.wasm-collect-continuously wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/call_indirect.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.default-wasm wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/modules/js-wasm-table-namespace.js.wasm-eager-jettison wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/basic-element.js.wasm-no-call-ic wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-collect-continuously wasm.yaml/wasm/js-api/wrapper-function.js.default-wasm wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/func.wast.js.wasm-no-tls-context wasm.yaml/wasm/modules/js-wasm-table.js.wasm-eager-jettison wasm.yaml/wasm/regress/regress-191056.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/linking.wast.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/call_indirect.wast.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/context-switch.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/basic-element.js.default-wasm wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/nameSection.js.wasm-no-air wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-eager-jettison wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-slow-memory wasm.yaml/wasm/spec-tests/return.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/func.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/table-basic.js.wasm-eager-jettison wasm.yaml/wasm/js-api/element.js.wasm-no-air wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/nameSection.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/nameSection.js.default-wasm wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-air wasm.yaml/wasm/spec-tests/imports.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/unique-signature.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/br.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/context-switch.js.wasm-no-air wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-collect-continuously wasm.yaml/wasm/js-api/wrapper-function.js.wasm-eager-jettison wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.default-wasm wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-no-air wasm.yaml/wasm/js-api/element.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/unreachable.wast.js.wasm-no-call-ic wasm.yaml/wasm/modules/js-wasm-table.js.wasm-collect-continuously wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-no-air wasm.yaml/wasm/spec-tests/unreachable.wast.js.wasm-eager-jettison wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/br_table.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/unique-signature.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-no-call-ic wasm.yaml/wasm/js-api/unique-signature.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/context-switch.js.wasm-eager-jettison wasm.yaml/wasm/regress/regress-191056.js.wasm-no-air wasm.yaml/wasm/js-api/call-indirect.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/table-basic.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/func.wast.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/table-basic.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/modules/wasm-imports-wasm-exports.js.wasm-slow-memory wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-no-air wasm.yaml/wasm/function-tests/table-basic-2.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/nameSection.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-collect-continuously wasm.yaml/wasm/js-api/unique-signature.js.wasm-slow-memory wasm.yaml/wasm/js-api/unique-signature.js.wasm-eager-jettison wasm.yaml/wasm/modules/js-wasm-table.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-air wasm.yaml/wasm/spec-tests/imports.wast.js.default-wasm wasm.yaml/wasm/js-api/wrapper-function.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-eager-jettison wasm.yaml/wasm/js-api/unique-signature.js.default-wasm wasm.yaml/wasm/js-api/element.js.wasm-eager-jettison wasm.yaml/wasm/modules/js-wasm-table.js.wasm-slow-memory wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-collect-continuously wasm.yaml/wasm/spec-tests/return.wast.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-eager-jettison wasm.yaml/wasm/modules/wasm-imports-js-re-exports-wasm-exports.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/unreachable.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/grow-memory-2.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-eager-jettison wasm.yaml/wasm/spec-tests/br_table.wast.js.wasm-no-call-ic wasm.yaml/wasm/spec-tests/br.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/left-to-right.wast.js.wasm-no-tls-context wasm.yaml/wasm/spec-tests/br_table.wast.js.wasm-no-air wasm.yaml/wasm/function-tests/context-switch.js.wasm-no-call-ic apiTests Comment on attachment 371242 [details] Patch Attachment 371242 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12369241 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html workers/wasm-hashset.html Created attachment 371248 [details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
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 Created attachment 371303 [details]
Patch
The minibrowser entitlements were by mistake, I will remove them after review 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? > 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.
Created attachment 371467 [details]
Patch
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 on attachment 371467 [details] Patch Clearing flags on attachment: 371467 Committed r246139: <https://trac.webkit.org/changeset/246139> All reviewed patches have been landed. Closing bug. |