[WASM-References] Add support for active mods in element section
Created attachment 414651 [details] Patch
Created attachment 414655 [details] Patch
Haven't supported passive & declarative segments because there is no table.init instruction for it in JSC yet. I'm going to add table.init with passive segments in next patches. Then support declarative segments and add all tests from the ref-types spec repo.
Created attachment 414666 [details] Patch
Created attachment 414886 [details] Patch
<rdar://problem/71758493>
Comment on attachment 414886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414886&action=review overall looks good, but I think we should address the Optional vs sentinel value issue. > Source/JavaScriptCore/ChangeLog:7 > + Adjust wasm parser to parse new form of element section. Nit: We usually put a new line here. > Source/JavaScriptCore/wasm/WasmFormat.h:250 > + Vector<Optional<uint32_t>> functionIndices; Since this is long lived, and could be very large (many modules I've seen have 10k+ functions), we should probably use a sentinel value that's not realistic like UINT_MAX. Right now I think this is doubling the memory of this vector. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:611 > + if (!element.functionIndices[i].hasValue()) { Nit: I think you can just do `if (!element.functionIndices[I])` > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:-624 > - ++elementIndex; lol we have quality code...
Comment on attachment 414886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414886&action=review > Source/JavaScriptCore/ChangeLog:10 > + Add support for vec(exp) in element section: can you explain what it used t be and how this is different? > Source/JavaScriptCore/ChangeLog:12 > + Just transformed (ref.func idx) -> idx and (ref.null func) -> null. I don't know what this means. > Source/JavaScriptCore/ChangeLog:13 > + For null values call table->clear to zero value. I don't understand the significance here. Can you explain it more > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:386 > + const uint32_t tableIndex = 0; nit: can be constexpr > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:412 > + case 0x01: { > + WASM_PARSER_FAIL_IF(!Options::useWebAssemblyReferences(), "references are not enabled"); > + WASM_PARSER_FAIL_IF(true, "unsupported ", elementNum, "th Element reserved byte"); > + break; > + } What do we need to do to support this? Maybe we should have a bug? > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:432 > + Optional<uint32_t> funcIdx; Why optional here? Seems like a weird choice since we fail unless we parse a real uint32_t > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:443 > + case 0x03: { > + WASM_PARSER_FAIL_IF(!Options::useWebAssemblyReferences(), "references are not enabled"); > + WASM_PARSER_FAIL_IF(true, "unsupported ", elementNum, "th Element reserved byte"); > + break; Should we have a bug on implementing this? > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:472 > + WASM_PARSER_FAIL_IF(true, "unsupported ", elementNum, "th Element reserved byte"); What do we need to do to support this? Maybe we should have a bug? > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:487 > + WASM_PARSER_FAIL_IF(refType == Externref, "reftype in element section should be funcref"); I think the error message and the condition here are a bit worrisome if anyone adds another enum value. Why can't we just directly check if != Funcref?
Created attachment 415097 [details] Patch
Comment on attachment 414886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414886&action=review >> Source/JavaScriptCore/ChangeLog:7 >> + Adjust wasm parser to parse new form of element section. > > Nit: We usually put a new line here. Fixed. >> Source/JavaScriptCore/ChangeLog:10 >> + Add support for vec(exp) in element section: > > can you explain what it used t be and how this is different? Removed implementation details from high-level description. >> Source/JavaScriptCore/wasm/WasmFormat.h:250 >> + Vector<Optional<uint32_t>> functionIndices; > > Since this is long lived, and could be very large (many modules I've seen have 10k+ functions), we should probably use a sentinel value that's not realistic like UINT_MAX. Right now I think this is doubling the memory of this vector. Fixed this and all other issues. Now design of this is just one-one copy from SpiderMonkey :)
Comment on attachment 415097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415097&action=review r=me with suggestions. > Source/JavaScriptCore/wasm/WasmFormat.h:233 > + constexpr static uint32_t NullFuncIndex = UINT32_MAX; Let's define it as `nullFuncIndex`, which is WebKit's naming convention for constants. > Source/JavaScriptCore/wasm/WasmFormat.h:235 > + enum class Kind { Let's make it `enum class Kind : uint8_t`. > Source/JavaScriptCore/wasm/WasmFormat.h:248 > + bool active() const { return kind == Kind::Active; } isActive() would be better. > Source/JavaScriptCore/wasm/WasmFormat.h:253 > + TableElementType elemType; Let's name it elementType. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:417 > + // https://bugs.webkit.org/show_bug.cgi?id=219297 Add FIXME with comment about what is missing. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:428 > + uint8_t elemKind; => elementKind. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:450 > + // https://bugs.webkit.org/show_bug.cgi?id=219385 Ditto. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:480 > + // https://bugs.webkit.org/show_bug.cgi?id=219297 Ditto. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:516 > + // https://bugs.webkit.org/show_bug.cgi?id=219385 Ditto. > Source/JavaScriptCore/wasm/WasmSectionParser.h:75 > + PartialResult WARN_UNUSED_RETURN parseElemKind(uint8_t& elemKind); > + PartialResult WARN_UNUSED_RETURN parseIndexCountForElemSection(uint32_t&, const unsigned); > + PartialResult WARN_UNUSED_RETURN parseFuncIdxFromRefExpForElemSection(uint32_t&, const unsigned, const unsigned); > + PartialResult WARN_UNUSED_RETURN parseFuncIdxForElemSection(uint32_t&, const unsigned, const unsigned); Let's rename Elem to Element. WebKit prefers non-abbreviated names :) And since we have parseElement, using "Element" is good for consistency. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:558 > + if (element.active()) { if (!element.isActive()) continue; can be cleaner. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:619 > // for the import. Let's move `uint32_t functionIndex = element.functionIndices[i];` to the prologue of this loop body and use `functionIndex` instead of `element.functionIndices[i]`.
Created attachment 415193 [details] Patch
Created attachment 415195 [details] Patch
Comment on attachment 415195 [details] Patch r=me
Committed r270344: <https://trac.webkit.org/changeset/270344> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415195 [details].