wabt icon indicating copy to clipboard operation
wabt copied to clipboard

Support table initializers

Open zherczeg opened this issue 6 months ago • 8 comments

Another large patch on the top of the other two. Now there are 3-4000 lines of new code is waiting for review, and it looks like the end is still far.

The painful part so far: https://webassembly.github.io/function-references/core/binary/modules.html#element-section

Since type 0-3 is reserved for (ref func), all (elem ... funcref ...) must be encoded with 4-7, which increase binary size. This affect several tests, and probably real word programs :(

There are spec tests to check this behavior.

zherczeg avatar Apr 29 '25 13:04 zherczeg

Function references is a Phase 5 (The Feature is Standardized) proposal for WebAssembly. It is a requirement for the Garbage Collection proposal. It is part of the WebAssembly 3.0 draft as well:

https://webassembly.github.io/spec/versions/core/WebAssembly-3.0-draft.pdf

The overview of the proposal isn't that long, but it turned out that it affects all parts of the project, since types are everywhere and type comparisons / validations are frequently used. Nullable / non-nullable references (e.g. tables with initializers and/or active elem initializers, etc.) requires many extra checks as well. Imports are on another level, since you need to validate them across modules. I have no idea how a native function can define references though, there is no module assigned for them.

This amount of work this feature needs was unexpected for me. Unfortunately, this is unavoidable. I made many patches, and these cover most specification tests, although some of them are still just partly supported. However, I don't get any reviews for them. I would like to understand why.

  • Is this a feature which does not align with the aims of wabt?
  • Are these changes too big / too risky?
  • Any other reasons?

It is very hard to make progress when I don't get any feedback, and I need to maintain several patches. Is there a chance these patches will land?

zherczeg avatar May 01 '25 07:05 zherczeg

This amount of work this feature needs was unexpected for me. Unfortunately, this is unavoidable. I made many patches, and these cover most specification tests, although some of them are still just partly supported. However, I don't get any reviews for them. I would like to understand why.

  • Is this a feature which does not align with the aims of wabt?
  • Are these changes too big / too risky?
  • Any other reasons?

It is very hard to make progress when I don't get any feedback, and I need to maintain several patches. Is there a chance these patches will land?

Sorry for the lack of feedback on some of your patches. Unfortunately there are very few folks to who have any time to work on wabt these days (or review patches).

Please expect fairly long feedback cycles. If you are waiting for a review from me, feel free to ping me a every couple of days, but please understand that I have very few cycles to spend on wabt myself.

For some background on where some folks see wabt heading see this issue: https://github.com/WebAssembly/wabt/issues/2348. If you would like to help maintain wabt going forward, and potentially work on major additions such as GC please join that discussion.

sbc100 avatar May 06 '25 06:05 sbc100

Thank you for the suggestion!

zherczeg avatar May 06 '25 13:05 zherczeg

The patch set (#2562, #2567, and this patch) covers 25 tests from the 26 spec tests in testsuite/proposals/function-references. That is 96% pass rate. The missing one is ref.wast, which needs removing the invalid reference test during parsing.

zherczeg avatar May 08 '25 12:05 zherczeg

The patch set (#2562, #2567, and this patch) covers 25 tests from the 26 spec tests in testsuite/proposals/function-references. That is 96% pass rate. The missing one is ref.wast, which needs removing the invalid reference test during parsing.

Nice! Thanks for all the hard work on this.

I assume #2562 is the first in the series and the one we should look at first?

BTW can you have this PR a better title? Support table initializers sounds odd because that is obviously something that wabt already supports. Perhaps you mean a certain type of new table intializer?

sbc100 avatar May 08 '25 15:05 sbc100

I assume #2562 is the first in the series and the one we should look at first?

Yes. It focuses on extending the existing features, e.g. tables / globals now supports refs. Recursive refs, and not initalized non nullable refs are detected as well. The next patch focuses on adding new byte codes. The last focuses on a new initializer value for tables. It breaks the current API.

BTW can you have this PR a better title? Support table initializers sounds odd because that is obviously something that wabt already supports. Perhaps you mean a certain type of new table intializer?

I am open to any ideas.

https://github.com/WebAssembly/function-references/blob/main/proposals/function-references/Overview.md#tables Tables can have an optional initializer value, and all elements of the table is set to that value when the table is constructed (before active elems are processed). If ref.null is not possible for a table (e.g. it has (ref func) type), the initializer is mandatory.

Btw I found a case, which has no spec test, and the specification is unclear:

(table (ref func) (elem 1)) -> since all elements of the table is initialized, it seems the initializer is unnecessary.

However, if I do the inlining here, I should get a missing initializer error: https://webassembly.github.io/function-references/core/text/modules.html#tables And I have no idea how the binary should be constructed. Maybe the first function could be the initializer.

zherczeg avatar May 08 '25 17:05 zherczeg

FWIW, when I hit a case where the prose spec is unclear, I usually just go try it in the reference interpreter and in wasm-tools. If those two disagree, it's an even better opportunity to submit a new test case. :-) Andreas is usually very quick about accepting new test cases into the spec repo.

keithw avatar May 08 '25 17:05 keithw

The last test is done, now all spec test are pass.

zherczeg avatar May 09 '25 11:05 zherczeg

What do you think about re-titling: "Support table initializer expressions; completes function references support"

sbc100 avatar Nov 13 '25 18:11 sbc100

Thank you for the review. I changed the title, and I hope I fixed every request. Please check the patch again.

zherczeg avatar Nov 14 '25 11:11 zherczeg