Add support for multiple tables
Created attachment 372080 [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 372080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372080&action=review Mostly LGTM, but some questions and I think I found a bug. > Source/JavaScriptCore/ChangeLog:9 > + existing users to give a table index. We also add some extra checks for table.get/set. extra checks. Why didn't we do this before? Can you explain? > Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:958 > + if (idx >= instance->table(tableIndex)->length()) > + return 0; why? We didn't do this before. > Source/JavaScriptCore/wasm/WasmInstance.cpp:96 > + *bitwise_cast<Table**>(bitwise_cast<char*>(this) + offsetOfTablePtr(m_numImportFunctions, i)) = &table.leakRef(); > + this->table(i)->ref(); this is wrong. this table is already +1, and you're leaking it, then ref() after. I think leakRef is all you need. > Source/JavaScriptCore/wasm/WasmInstance.h:147 > + return (offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions + sizeof(Table*) * numTables).unsafeGet(); I think you need to do rounding here after the number of import functions. Or add some static asserts that they have same alignment as ImportFunctionInfo
Comment on attachment 372080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372080&action=review >> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:958 >> + return 0; > > why? We didn't do this before. I never wrote a test case for out of bounds access until now. It should trap according to the semantics, but instead we just hit the release assert. I am moving these checks to the funcref patch since I responded to the EncodedJSValue comments there.
Created attachment 372175 [details] patch (requires funcref patch) This is the patch with the comments above addressed, and rebased to apply after the funcref patch
Created attachment 372266 [details] Patch
Comment on attachment 372266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372266&action=review r=me with comments. I found one bug, and the rest are nits. Also, I didn't read through all testing, but I commented in areas where it would be good to have tests. > Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1918 > + RELEASE_ASSERT(Arg::isValidAddrForm(Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex), B3::Width64)); this is now wrong. Before, it was guaranteed, but now it's not. On arm64, this may not fit in an immediate. Please add a test and make this a branch. > Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1923 > + append(Move, Arg::addr(instanceValue(), Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex)), callableFunctionBufferLength); this is where the branch should be. Basically, look for "isValidForm", and you need to branch on instruction selection based on that. Should be easy to make an arm64 test fail by having many, many, tables > Source/JavaScriptCore/wasm/WasmInstance.cpp:113 > + if (auto* t = this->table(i)) > + t->deref(); t => table > Source/JavaScriptCore/wasm/WasmInstance.h:157 > + return (offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions + sizeof(Table*) * numTables).unsafeGet(); I think this should be: roundUpToMultipleOf<sizeof(Table*)>((offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions) + sizeof(Table*)*numTables Or static assert > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:219 > WASM_PARSER_FAIL_IF(!parseVarUInt32(count), "can't get Table's count"); let's add a maximum number of tables here > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:379 > + WASM_PARSER_FAIL_IF(m_info->tables[tableIndex].type() != TableElementType::Funcref, "Table ", tableIndex, " must have type 'anyfunc' to have an element section"); Do we have a test? > Source/JavaScriptCore/wasm/WasmValidate.cpp:183 > + WASM_VALIDATOR_FAIL_IF(tableIndex >= m_module.tableCount(), "table index ", tableIndex, " is invalid, limit is ", m_module.tableCount()); Do we have a test? > Source/JavaScriptCore/wasm/WasmValidate.cpp:192 > + WASM_VALIDATOR_FAIL_IF(tableIndex >= m_module.tableCount(), "table index ", tableIndex, " is invalid, limit is ", m_module.tableCount()); Do we have a test? > Source/JavaScriptCore/wasm/WasmValidate.cpp:197 > + WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Anyref > + && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc"); do we have tests for this? > Source/JavaScriptCore/wasm/WasmValidate.cpp:387 > + WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call"); do we have tests for this? > JSTests/wasm/spec-tests/imports.wast.js:199 > +// These tests assert that we can only have one table. The references spec makes this incorrect, and this should be fixed when it gets merged into the main spec. > +//assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x02\x8d\x80\x80\x80\x00\x02\x00\x00\x01\x70\x00\x0a\x00\x00\x01\x70\x00\x0a"); > > // imports.wast:297 > -assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x02\x87\x80\x80\x80\x00\x01\x00\x00\x01\x70\x00\x0a\x04\x84\x80\x80\x80\x00\x01\x70\x00\x0a"); > +//assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x02\x87\x80\x80\x80\x00\x01\x00\x00\x01\x70\x00\x0a\x04\x84\x80\x80\x80\x00\x01\x70\x00\x0a"); > > // imports.wast:301 > -assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x04\x87\x80\x80\x80\x00\x02\x70\x00\x0a\x70\x00\x0a"); > +//assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x04\x87\x80\x80\x80\x00\x02\x70\x00\x0a\x70\x00\x0a"); just delete these lines.
Comment on attachment 372266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372266&action=review >> Source/JavaScriptCore/wasm/WasmInstance.h:157 >> + return (offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions + sizeof(Table*) * numTables).unsafeGet(); > > I think this should be: > > roundUpToMultipleOf<sizeof(Table*)>((offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions) + sizeof(Table*)*numTables > > Or static assert How does this differ from the static assert above? >> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:219 >> WASM_PARSER_FAIL_IF(!parseVarUInt32(count), "can't get Table's count"); > > let's add a maximum number of tables here I just picked 1000000 arbitrarily, but I will file a spec bug: https://webassembly.github.io/reference-types/js-api/index.html#limits says 1 still. >> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:379 >> + WASM_PARSER_FAIL_IF(m_info->tables[tableIndex].type() != TableElementType::Funcref, "Table ", tableIndex, " must have type 'anyfunc' to have an element section"); > > Do we have a test? Yes, one in the other patch as well as one in this patch for when the table index != 0 >> Source/JavaScriptCore/wasm/WasmValidate.cpp:197 >> + && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc"); > > do we have tests for this? It is currently impossible to do so, so I will turn this into a debug assert. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:387 >> + WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call"); > > do we have tests for this? It is a parse error, so I will turn this into a release assert.
Comment on attachment 372266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372266&action=review >> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1923 >> + append(Move, Arg::addr(instanceValue(), Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex)), callableFunctionBufferLength); > > this is where the branch should be. Basically, look for "isValidForm", and you need to branch on instruction selection based on that. Should be easy to make an arm64 test fail by having many, many, tables The test case I uploaded runs on iOS, and I added a static assert here to warn anyone who tries to increase the limit in the future. Is that OK?
Created attachment 372313 [details] Patch
Comment on attachment 372266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372266&action=review >>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1923 >>> + append(Move, Arg::addr(instanceValue(), Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex)), callableFunctionBufferLength); >> >> this is where the branch should be. Basically, look for "isValidForm", and you need to branch on instruction selection based on that. Should be easy to make an arm64 test fail by having many, many, tables > > The test case I uploaded runs on iOS, and I added a static assert here to warn anyone who tries to increase the limit in the future. Is that OK? Your test uses the maximum number of import functions too? What if you remove the limit on the number of tables, and use a large number for the table index? >>> Source/JavaScriptCore/wasm/WasmValidate.cpp:197 >>> + && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc"); >> >> do we have tests for this? > > It is currently impossible to do so, so I will turn this into a debug assert. Why is it impossible? Isn’t this the job of the validator? >>> Source/JavaScriptCore/wasm/WasmValidate.cpp:387 >>> + WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call"); >> >> do we have tests for this? > > It is a parse error, so I will turn this into a release assert. Why? Aren’t we validating here? The whole point is we can see arbitrary things, right?
Created attachment 372347 [details] Patch
Created attachment 372376 [details] Patch
(In reply to Saam Barati from comment #11) > >>> Source/JavaScriptCore/wasm/WasmValidate.cpp:197 > >>> + && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc"); > >> > >> do we have tests for this? > > > > It is currently impossible to do so, so I will turn this into a debug assert. > > Why is it impossible? Isn’t this the job of the validator? The type makes it impossible, but I wasn't sure how to put a static_assert to warn anyone who adds a new table type. > > >>> Source/JavaScriptCore/wasm/WasmValidate.cpp:387 > >>> + WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call"); > >> > >> do we have tests for this? > > > > It is a parse error, so I will turn this into a release assert. > > Why? Aren’t we validating here? The whole point is we can see arbitrary > things, right? It is a parse error if the table index is too big / the wrong type (the check was there before this patch), and I added a test to make sure it is a parse error.
Created attachment 372380 [details] Patch
Comment on attachment 372376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372376&action=review Still LGTM, two small comments > Source/JavaScriptCore/ChangeLog:9 > + Support multiple wasm tables. We turn tableInformation into a tables array, and update all of the > + existing users to give a table index. Sorry, I should have commented earlier, but you should have at least a sentence on the implementation details, e.g, that it hangs off the end of Instance. Feel free to add even more. > Source/JavaScriptCore/wasm/WasmInstance.cpp:113 > + if (auto* table = this->table(i)) > + table->deref(); shouldn't this be an assert that !table(i)? We have that assert in the JS code.
Created attachment 372381 [details] Patch
Comment on attachment 372381 [details] Patch Clearing flags on attachment: 372381 Committed r246571: <https://trac.webkit.org/changeset/246571>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51867688>