Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174123002:
[wasm] Add support for multiple indirect function tables (Closed)
Description[wasm] Add support for multiple indirect function tables
This patch updates internal data structures used by V8 to support
multiple indirect function tables (WebAssembly/design#682). But, since
this feature is post-MVP, the functionality is not directly exposed and
parsing/generation of WebAssembly is left unchanged. Nevertheless, it
is being used in an experiment to implement fine-grained control flow
integrity based on C/C++ types.
BUG=
Committed: https://crrev.com/0a9d4003c78316dcbf366db718f9f6285d1c8a74
Cr-Commit-Position: refs/heads/master@{#38110}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Fix loop bug, cleanups, style #
Total comments: 44
Patch Set 3 : Update for readability #Patch Set 4 : Rename indirect tables #Patch Set 5 : Rebase #Patch Set 6 : Fix assertions, add casts #Patch Set 7 : Fix more MSVC type checks #Patch Set 8 : One last MSVC type fix #Patch Set 9 : More MSVC type fixes #Patch Set 10 : Fix GC issue #
Messages
Total messages: 63 (52 generated)
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== [wasm] Add support for multiple tables This patch updates internal data structures used by V8 to support multiple tables. Parsing/generation of WebAssembly is unchanged and uses the default table at index zero. BUG= ========== to ========== [wasm] Add support for multiple tables This patch updates internal data structures used by V8 to support multiple tables. Parsing/generation of WebAssembly is unchanged and uses the default table at index zero. BUG= ==========
ddchen@chromium.org changed reviewers: + ahaas@chromium.org, bradnelson@chromium.org, mtrofin@chromium.org
https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2062: // Assume only one table for now. Could you add a DCHECK for this assumption? https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2064: ((table_size = module_->GetTable(0)->max_size) > 0)) { I personally don't like assignments in conditions. https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2135: // Assume only one table for now. same here. https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2819: DCHECK(!module_->instance->function_table[index].is_null()); Shouldn't this be {i} instead of {index}? If you really meant to check function_table[index], then I think the DCHECK should be outside the for loop. https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2820: function_table_.push_back( Does {push_back} actually add the element at the right location if {i < function_table_.size()} is true and {function_table_[i]} is false? https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-interpreter.cc File src/wasm/wasm-interpreter.cc (right): https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-interpreter.c... src/wasm/wasm-interpreter.cc:1389: // Assume only one table for now. I think we should have a DCHECK for this assumption. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1006: temp_instance_for_compilation.function_table[i] = values; Why do you store the function table in the temp_instance_for_compilation? Why does temp_instance_for_compilation exist in the first place? https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1187: Handle<FixedArray> clone_table = factory->CopyFixedArray(orig_table); clone_table is already used above, could you use a different variable name here? https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1265: std::vector<Handle<Code>> functions( Why did you make this change? https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1460: Handle<FixedArray> BuildFunctionTable(Isolate* isolate, uint32_t index, Why did you move this function? https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.h#newc... src/wasm/wasm-module.h:249: std::vector<Handle<FixedArray>> function_table; // indirect function tables. Maybe we should call the field {function_tables} now that it contains multiple tables? https://codereview.chromium.org/2174123002/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm.cc (right): https://codereview.chromium.org/2174123002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm.cc:2411: module.AddIndirectFunctionTable(table, 2); could you use arraysize(table) here instead of the hardcoded 2? https://codereview.chromium.org/2174123002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm.cc:2442: module.AddIndirectFunctionTable(table, 2); same here. https://codereview.chromium.org/2174123002/diff/1/test/mjsunit/wasm/wasm-modu... File test/mjsunit/wasm/wasm-module-builder.js (right): https://codereview.chromium.org/2174123002/diff/1/test/mjsunit/wasm/wasm-modu... test/mjsunit/wasm/wasm-module-builder.js:230: if (wasm.pad != null) { Do you intentionally use != instead of !== here?
https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2062: // Assume only one table for now. On 2016/07/25 19:23:44, ahaas wrote: > Could you add a DCHECK for this assumption? Done. https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2064: ((table_size = module_->GetTable(0)->max_size) > 0)) { On 2016/07/25 19:23:44, ahaas wrote: > I personally don't like assignments in conditions. Done. https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2135: // Assume only one table for now. On 2016/07/25 19:23:44, ahaas wrote: > same here. Done. https://codereview.chromium.org/2174123002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2820: function_table_.push_back( On 2016/07/25 19:23:44, ahaas wrote: > Does {push_back} actually add the element at the right location if {i < > function_table_.size()} is true and {function_table_[i]} is false? This code was incorrect, it should be fixed now. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-interpreter.cc File src/wasm/wasm-interpreter.cc (right): https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-interpreter.c... src/wasm/wasm-interpreter.cc:1389: // Assume only one table for now. On 2016/07/25 19:23:44, ahaas wrote: > I think we should have a DCHECK for this assumption. Done. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1006: temp_instance_for_compilation.function_table[i] = values; On 2016/07/25 19:23:44, ahaas wrote: > Why do you store the function table in the temp_instance_for_compilation? Why > does temp_instance_for_compilation exist in the first place? I'm not entirely familiar with this code, but I believe that modules are first compiled (sequentially or in parallel), and are then separately instantiated. The reason that they are stored is that the tables need to be accessible from wasm-compiler.cc; if a table is used by an indirect call, then it is cached as a heap constant and referenced by the graph builder. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1187: Handle<FixedArray> clone_table = factory->CopyFixedArray(orig_table); On 2016/07/25 19:23:45, ahaas wrote: > clone_table is already used above, could you use a different variable name here? Done. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1265: std::vector<Handle<Code>> functions( On 2016/07/25 19:23:45, ahaas wrote: > Why did you make this change? The reason I moved the {BuildFunctionTable} and {PopulateFunctionTable} functions was to expose them for usage in for wasm-run-utils.h. Previously, the implementations were copied into that file. But for {BuildFunctionTable}, one implementation used the {std::vector} representation, while the other used the {FixedArray} implementation. I standardized on the {std::vector} representation, so I exposed it here. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:1460: Handle<FixedArray> BuildFunctionTable(Isolate* isolate, uint32_t index, On 2016/07/25 19:23:45, ahaas wrote: > Why did you move this function? See above. https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2174123002/diff/1/src/wasm/wasm-module.h#newc... src/wasm/wasm-module.h:249: std::vector<Handle<FixedArray>> function_table; // indirect function tables. On 2016/07/25 19:23:45, ahaas wrote: > Maybe we should call the field {function_tables} now that it contains multiple > tables? Done. https://codereview.chromium.org/2174123002/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm.cc (right): https://codereview.chromium.org/2174123002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm.cc:2411: module.AddIndirectFunctionTable(table, 2); On 2016/07/25 19:23:45, ahaas wrote: > could you use arraysize(table) here instead of the hardcoded 2? Done. https://codereview.chromium.org/2174123002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm.cc:2442: module.AddIndirectFunctionTable(table, 2); On 2016/07/25 19:23:45, ahaas wrote: > same here. Done. https://codereview.chromium.org/2174123002/diff/1/test/mjsunit/wasm/wasm-modu... File test/mjsunit/wasm/wasm-module-builder.js (right): https://codereview.chromium.org/2174123002/diff/1/test/mjsunit/wasm/wasm-modu... test/mjsunit/wasm/wasm-module-builder.js:230: if (wasm.pad != null) { On 2016/07/25 19:23:45, ahaas wrote: > Do you intentionally use != instead of !== here? Nope, good catch!
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2174123002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2816: if (index >= function_tables_.size()) { I think it'd be a bit more readable if you either: - checked if function_tables_.empty() and filled it all - so the invariant is that function_tables_ is either empty or constructed), OR - reserved the appropriate size for function_tables_ in the constructor of WasmGraphBuilder, and lazily instantiated the value at index. Then you can just check if function_tables_[index] is null and fill it, if needed. https://codereview.chromium.org/2174123002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2817: for (size_t i = function_tables_.size(); i <= index; i++) { ++i https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:264: uint32_t table_count = 1; static const uint32_t kSupportedTableCount = 1 https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:273: &module->function_tables.back()); should it be module->function_tables[i], for consistency? https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:515: for (uint32_t i = 0; i < table->size; i++) { ++i https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:518: error(pc_ - 2, "invalid function index"); what's "2"? (sizeof(uint16_t) ?) https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-interpret... File src/wasm/wasm-interpreter.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-interpret... src/wasm/wasm-interpreter.cc:1392: if (!target) { target == nullptr https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:258: MaybeHandle<FixedArray> BuildFunctionTable(Isolate* isolate, why did this move? https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:155: kIndirectFunctionTable, // maybe FixedArray of FixedArray of is this one or more tables? kIndirectFunctionTables or kIndirectFunctionTablePrototypes? (prototypes because they aren't populated) https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:193: enum WasmTableMetadata { WasmIndirectFunctionTableMetadata long name, but avoids confusion and simplifies maintainability https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1004: for (uint32_t i = 0; i < function_tables.size(); i++) { ++i https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1173: MaybeHandle<FixedArray> maybe_indirect_table = _tables (plural) https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1175: Handle<FixedArray> indirect_table; _tables https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1177: Handle<FixedArray> clone_indirect_table = plural here, too https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1297: Handle<FixedArray> indirect_table; tables https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:151: struct WasmTable { WasmIndirectFunctionTable. WasmTable is very ambiguous: import table? function table? etc. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:290: bool IsValidTable(uint32_t index) { const if you didn't mind, while at it, could you please const the above APIs, too? (the Isxxx) Thanks! https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:309: const WasmTable* GetTable(uint32_t index) { this one's const, too https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/test-r... File test/cctest/wasm/test-run-wasm.cc (right): https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm.cc:2411: module.AddIndirectFunctionTable(table, arraysize(table)); rename to "indirect_function_table" for clarity https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm.cc:2441: uint16_t table[] = {0, 1}; "indirect_function_table" https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/wasm-r... File test/cctest/wasm/wasm-run-utils.h (right): https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/wasm-r... test/cctest/wasm/wasm-run-utils.h:226: for (int i = 0; i < table_size; i++) { ++i
https://codereview.chromium.org/2174123002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2816: if (index >= function_tables_.size()) { On 2016/07/26 03:37:26, Mircea Trofin wrote: > I think it'd be a bit more readable if you either: > - checked if function_tables_.empty() and filled it all - so the invariant is > that function_tables_ is either empty or constructed), OR > - reserved the appropriate size for function_tables_ in the constructor of > WasmGraphBuilder, and lazily instantiated the value at index. Then you can just > check if function_tables_[index] is null and fill it, if needed. I'll take the former approach; the module isn't available until {set_module} is called. https://codereview.chromium.org/2174123002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2817: for (size_t i = function_tables_.size(); i <= index; i++) { On 2016/07/26 03:37:26, Mircea Trofin wrote: > ++i Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:264: uint32_t table_count = 1; On 2016/07/26 03:37:27, Mircea Trofin wrote: > static const uint32_t kSupportedTableCount = 1 Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:273: &module->function_tables.back()); On 2016/07/26 03:37:26, Mircea Trofin wrote: > should it be module->function_tables[i], for consistency? Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:515: for (uint32_t i = 0; i < table->size; i++) { On 2016/07/26 03:37:26, Mircea Trofin wrote: > ++i Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/module-decoder... src/wasm/module-decoder.cc:518: error(pc_ - 2, "invalid function index"); On 2016/07/26 03:37:26, Mircea Trofin wrote: > what's "2"? (sizeof(uint16_t) ?) Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-interpret... File src/wasm/wasm-interpreter.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-interpret... src/wasm/wasm-interpreter.cc:1392: if (!target) { On 2016/07/26 03:37:27, Mircea Trofin wrote: > target == nullptr Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:258: MaybeHandle<FixedArray> BuildFunctionTable(Isolate* isolate, On 2016/07/26 03:37:27, Mircea Trofin wrote: > why did this move? In order to expose this function and eliminate the duplicate implementation in wasm-run-utils.h, I needed to move it out of the anonymous namespace. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:155: kIndirectFunctionTable, // maybe FixedArray of FixedArray of On 2016/07/26 03:37:27, Mircea Trofin wrote: > is this one or more tables? > > kIndirectFunctionTables or kIndirectFunctionTablePrototypes? (prototypes because > they aren't populated) One table that contains all of the other tables. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:193: enum WasmTableMetadata { On 2016/07/26 03:37:27, Mircea Trofin wrote: > WasmIndirectFunctionTableMetadata > > long name, but avoids confusion and simplifies maintainability Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1004: for (uint32_t i = 0; i < function_tables.size(); i++) { On 2016/07/26 03:37:27, Mircea Trofin wrote: > ++i Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1173: MaybeHandle<FixedArray> maybe_indirect_table = On 2016/07/26 03:37:27, Mircea Trofin wrote: > _tables (plural) Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1175: Handle<FixedArray> indirect_table; On 2016/07/26 03:37:27, Mircea Trofin wrote: > _tables Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1177: Handle<FixedArray> clone_indirect_table = On 2016/07/26 03:37:27, Mircea Trofin wrote: > plural here, too Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1297: Handle<FixedArray> indirect_table; On 2016/07/26 03:37:27, Mircea Trofin wrote: > tables Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:151: struct WasmTable { On 2016/07/26 03:37:27, Mircea Trofin wrote: > WasmIndirectFunctionTable. > > WasmTable is very ambiguous: import table? function table? etc. Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:290: bool IsValidTable(uint32_t index) { On 2016/07/26 03:37:27, Mircea Trofin wrote: > const > > if you didn't mind, while at it, could you please const the above APIs, too? > (the Isxxx) > > Thanks! Done. https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:309: const WasmTable* GetTable(uint32_t index) { On 2016/07/26 03:37:27, Mircea Trofin wrote: > this one's const, too Done. https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/test-r... File test/cctest/wasm/test-run-wasm.cc (right): https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm.cc:2411: module.AddIndirectFunctionTable(table, arraysize(table)); On 2016/07/26 03:37:27, Mircea Trofin wrote: > rename to "indirect_function_table" for clarity Done. https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm.cc:2441: uint16_t table[] = {0, 1}; On 2016/07/26 03:37:27, Mircea Trofin wrote: > "indirect_function_table" Done. https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/wasm-r... File test/cctest/wasm/wasm-run-utils.h (right): https://codereview.chromium.org/2174123002/diff/20001/test/cctest/wasm/wasm-r... test/cctest/wasm/wasm-run-utils.h:226: for (int i = 0; i < table_size; i++) { On 2016/07/26 03:37:27, Mircea Trofin wrote: > ++i Done.
https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:155: kIndirectFunctionTable, // maybe FixedArray of FixedArray of On 2016/07/26 05:44:44, ddchen wrote: > On 2016/07/26 03:37:27, Mircea Trofin wrote: > > is this one or more tables? > > > > kIndirectFunctionTables or kIndirectFunctionTablePrototypes? (prototypes > because > > they aren't populated) > > One table that contains all of the other tables. OK, but the name may cause confusion. How about kTableOfIndirectFunctionTables? Long, but clear
https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2174123002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:155: kIndirectFunctionTable, // maybe FixedArray of FixedArray of On 2016/07/26 05:52:47, Mircea Trofin wrote: > On 2016/07/26 05:44:44, ddchen wrote: > > On 2016/07/26 03:37:27, Mircea Trofin wrote: > > > is this one or more tables? > > > > > > kIndirectFunctionTables or kIndirectFunctionTablePrototypes? (prototypes > > because > > > they aren't populated) > > > > One table that contains all of the other tables. > > OK, but the name may cause confusion. How about kTableOfIndirectFunctionTables? > Long, but clear Done.
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9734) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9719) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/9680) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5706) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20269)
On 2016/07/26 16:11:46, ddchen wrote: lgtm, but one more thing: please update the checkin comment to capture sufficient context, specifically: - replace "tables" with "indirect function tables" - either succinctly explain the motivation (support separation of function types different in the language being compiled to wasm, which may be lowered to same wasm function types) and / or link to wasm spec or discussion
Description was changed from ========== [wasm] Add support for multiple tables This patch updates internal data structures used by V8 to support multiple tables. Parsing/generation of WebAssembly is unchanged and uses the default table at index zero. BUG= ========== to ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= ==========
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/11253) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/11268)
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
Description was changed from ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= ========== to ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= ==========
The CQ bit was checked by ddchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ddchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2174123002/#ps170001 (title: "Fix GC issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= ========== to ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= ========== to ========== [wasm] Add support for multiple indirect function tables This patch updates internal data structures used by V8 to support multiple indirect function tables (WebAssembly/design#682). But, since this feature is post-MVP, the functionality is not directly exposed and parsing/generation of WebAssembly is left unchanged. Nevertheless, it is being used in an experiment to implement fine-grained control flow integrity based on C/C++ types. BUG= Committed: https://crrev.com/0a9d4003c78316dcbf366db718f9f6285d1c8a74 Cr-Commit-Position: refs/heads/master@{#38110} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0a9d4003c78316dcbf366db718f9f6285d1c8a74 Cr-Commit-Position: refs/heads/master@{#38110} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
