Skip to content

[reference-types] remove single table restriction in IR #3517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 52 commits into from
Feb 9, 2021

Conversation

martianboy
Copy link
Contributor

@martianboy martianboy commented Jan 26, 2021

This will remove the single table per module assumption everywhere, and
instead introduces a vector of tables, same as functions, globals, etc.
Support for parsing and writing multiple tables is added and C/JS APIs
have been updated accordingly. A table name argument is also added to
call_indirect.

(Fixes #3512)

@martianboy martianboy force-pushed the multi-table branch 2 times, most recently from 16a7a4b to 657e727 Compare January 26, 2021 18:19
@martianboy
Copy link
Contributor Author

Reviews appreciated @kripken @aheejin @sbc100

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Haven't read all the PR yet, but the very initial comments:
I don't think we have a test case that uses multiple tables and call_indirects with a table index yet. Can we have one? Maybe multi-table.wast or something in test/ directory. You can run ./auto_update_tests.py to update other files after adding it.
(Also we would need tests for other added parts such as tests for the added C API)

@martianboy
Copy link
Contributor Author

Thanks, @aheejin.

I don't think we have a test case that uses multiple tables and call_indirects with a table index yet. Can we have one? Maybe multi-table.wast or something in test/ directory.

Right, I started adding tests today. Hopefully I can push some by tomorrow.

@martianboy
Copy link
Contributor Author

@aheejin

I don't think we have a test case that uses multiple tables and call_indirects with a table index yet. Can we have one? Maybe multi-table.wast or something in test/ directory. You can run ./auto_update_tests.py to update other files after adding it.

I don't think we can test modules with multiple tables yet, because in this PR I tried to keep it minimal and not implement the new changes from the spec as much as possible. For instance, we cannot parse an elem section in wast files that belong to tables with higher indexes. It does parse elems embedded inside a table declaration, but roundtrip runs fail as the elem section is separated from the table, and is therefore added to table 0. I did add a test/example for the new c apis though.

My plan was to get this PR reviewed and merged, to make sure that everything is working as before and I haven't missed anything. And then, continue with the remaining parts of the reference types spec.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review the entire thing yet, but the wasm.h and wasm-delegates-fields.h changes look like the right approach. Thanks for the PR!

Will try to review the rest tomorrow.

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aheejin

I don't think we have a test case that uses multiple tables and call_indirects with a table index yet. Can we have one? Maybe multi-table.wast or something in test/ directory. You can run ./auto_update_tests.py to update other files after adding it.

I don't think we can test modules with multiple tables yet, because in this PR I tried to keep it minimal and not implement the new changes from the spec as much as possible. For instance, we cannot parse an elem section in wast files that belong to tables with higher indexes. It does parse elems embedded inside a table declaration, but roundtrip runs fail as the elem section is separated from the table, and is therefore added to table 0. I did add a test/example for the new c apis though.

My plan was to get this PR reviewed and merged, to make sure that everything is working as before and I haven't missed anything. And then, continue with the remaining parts of the reference types spec.

Thanks. Sorry I was OOO and I have still read only a fraction of the PR so far; I think this PR is one of the fundamental changes, but the only test additions + changes I see are from metrics, wasm-split (which I don't understand well why they are necessary; I asked questions there), and some table name changes.

I understand it is sometimes hard to add tests for every single part of code changes, especially in this kind of big PRs that change one of the very fundamental assumptions in the code base, so I really appreciate your work! But I still think this kind of big PRs need at least some tests that cover the very backbone of changes, which I think should include wast reading/writing and binary reading/writing. Also I think it is more desirable for a PR to have a smaller change with tests that matches the change, but as I said, I know it is not always easy in this kind of PRs.. 😅

I think maybe we can defer implementations + tests for the new call_indirect for the next PR, but I still would like to see at least some tests that show basic reading/writing of multiple tables working. Can we add those parts to the PR and maybe defer some other parts of the implementation that are not essential to the reading/writing to the next PR? I think C-API also could have been deferred if you wanted to make this minimal but you already added them so that looks good.

Anyway I'll probably take a more close look within a few days but please don't think me as a blocker for LGTM as long as others are happy with it.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a lot of code could be simplified by requiring call_indirect to have a real table name rather than defaulting to the first table if the table name is empty.

@martianboy
Copy link
Contributor Author

@tlively Thanks a lot for the review! I will try to address the points by tomorrow.

@martianboy
Copy link
Contributor Author

@tlively I think I've addressed most of the review points here (I hope I haven't missed any), except for adding support for multiple tables in the fuzzer.

After reading fuzzing.h a little bit, I feel like anything I do in the fuzzer now to support multiple tables will have to be changed very soon when we add data type to tables and element segments, and convert segment data items from function names into (ref) expressions. If you agree, we can put that part for a later PR, but I'm totally open to suggestions.

Other than that, please do let me know if you have other comments about this PR. Hopefully we can finalize it very soon!

@tlively
Copy link
Member

tlively commented Feb 4, 2021

Neat, the latest changes look good to me. Can you summarize the C/JS API changes in the change log? Beyond that, I want to just do another review pass over everything and then it should be good!

@martianboy
Copy link
Contributor Author

@tlively I added a few missing C/JS APIs and updated the change log. Not sure if the change log update is good, we may want to make it shorter. I also sent PR #3548 to add feature options to wasm-dis.

Comment on lines 219 to 223
if (tableName.is()) {
return wasm->getTableOrNull(tableName);
} else if (!wasm->tables.empty()) {
return wasm->tables.front().get();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function can be removed and its use replaced with just wasm->getTableOrNull(tableName) now that we are assuming that the table name is always present.

Comment on lines 565 to 567
o << U32LEB(0x0);
} else {
o << U32LEB(0x2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to use the enum values here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I used constants, although using ?: made it look a bit ugly! I can try several if blocks if you prefer. I've seen both styles in wasm-binary.cpp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the if blocks would be better in this case, but I'm going to merge this as-is because it is so big and I don't want it to get out of sync with upstream. If you wouldn't mind fixing this in a follow-up PR, that would be great :)

@tlively
Copy link
Member

tlively commented Feb 9, 2021

After those final two comments are addressed, I will be happy to merge this :)

@martianboy
Copy link
Contributor Author

@tlively Thanks a lot for catching those! I've already started working on the next PR to add a ref type to Table. Hopefully it will be smaller and easier to review!

@tlively
Copy link
Member

tlively commented Feb 9, 2021

Thanks for your great work on this, @martianboy!

@tlively tlively merged commit 3da8b08 into WebAssembly:main Feb 9, 2021
@martianboy
Copy link
Contributor Author

Yaay! Finally! Thanks again @tlively. :)

@tlively
Copy link
Member

tlively commented Feb 9, 2021

You're welcome :D

@sbc100
Copy link
Member

sbc100 commented Feb 9, 2021

It looks like this change broke a bunch of emscirpten tests. At first glance it looks like any test related to dyanmic linking:

e.g.:

./tests/runner.py wasm2.test_relocatable_void_function
runner:WARNING: use EMTEST_ALL_ENGINES=1 in the env to run against all JS engines, which is slower but provides more coverage
Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_relocatable_void_function (test_core.wasm2) ... wasm-emscripten-finalize: /usr/local/google/home/sbc/dev/wasm/binaryen/src/wasm/wasm-binary.cpp:498: uint32_t wasm::WasmBinaryWriter::getTableIndex(wasm::Name) const: Assertion `it != indexes.tableIndexes.end()' failed.
emcc: error: '/usr/local/google/home/sbc/dev/wasm/binaryen-out/bin/wasm-emscripten-finalize --detect-features --minimize-wasm-changes --dyncalls-i64 --pass-arg=legalize-js-interface-export-originals test_relocatable_void_function.wasm -o test_relocatable_void_function.wasm' failed (-6)
FAIL

@martianboy
Copy link
Contributor Author

@sbc100 I will look into this as soon as I can.

@tlively
Copy link
Member

tlively commented Feb 10, 2021

I fixed it in #3560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple tables support from the reference-types proposal
5 participants