-
Notifications
You must be signed in to change notification settings - Fork 779
Support table initializer expressions; completes function references support #2593
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
Conversation
|
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.
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: #2348. If you would like to help maintain wabt going forward, and potentially work on major additions such as GC please join that discussion. |
|
Thank you for the suggestion! |
6492586 to
e92be0b
Compare
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? |
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.
I am open to any ideas. https://github.com/WebAssembly/function-references/blob/main/proposals/function-references/Overview.md#tables Btw I found a case, which has no spec test, and the specification is unclear:
However, if I do the inlining here, I should get a missing initializer error: |
|
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. |
ec270d6 to
e6dff8a
Compare
|
The last test is done, now all spec test are pass. |
22728a7 to
a4d9b47
Compare
dda079f to
bec3c3b
Compare
30e7b37 to
a7701de
Compare
zherczeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbc100 this is the last patch of the function references proposal.
| bool has_init_expr) override; | ||
| Result BeginTableInitExpr(Index index) override; | ||
| Result EndTableInitExpr(Index index) override; | ||
| Result EndTable(Index index) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since tables can have an initializer expressions now, this is a breaking change. I don't know how these are handled in this project.
| | [extended-const][] | `--enable-extended-const` | | ✓ | ✓ | ✓ | ✓ | ✓ | | ||
| | [relaxed-simd][] | `--enable-relaxed-simd` | | ✓ | ✓ | ✓ | ✓ | | | ||
| | [custom-page-sizes][] | `--enable-custom-page-sizes`| | ✓ | ✓ | ✓ | ✓ | ✓ | | ||
| | [function-references][] | `--enable-function-references` | | ✓ | ✓ | ✓ | ✓ | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used update-spec-tests.py and updated README before, so these are just speculative changes.
| ) | ||
|
|
||
| (elem declare funcref (ref.func $square)) | ||
| (elem declare func $square) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are similar changes below. The reason is a backward incompatible change. As for elem types 0-3, they have changed the result type from (ref null func) (same as funcref) to (ref func). This change affects table type matches.
sbc100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for all the work on this!
Can you update the PR description to match what you would like in the final commit?
include/wabt/ir.h
Outdated
| struct ElemSegment { | ||
| explicit ElemSegment(std::string_view name) : name(name) {} | ||
| uint8_t GetFlags(const Module*) const; | ||
| uint8_t GetFlags(const Module*, bool) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give this new argument a name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Since a bool (options_.features.function_references_enabled()) is passed, I would not introduce an enum for this.
include/wabt/shared-validator.h
Outdated
|
|
||
| Result OnFunction(const Location&, Var sig_var); | ||
| Result OnTable(const Location&, Type elem_type, const Limits&); | ||
| Result OnTable(const Location&, Type elem_type, const Limits&, bool, bool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you name these new arguments? (seems especially important for bool)
src/binary-reader-objdump.cc
Outdated
| Result BeginTable(Index index, | ||
| Type elem_type, | ||
| const Limits* elem_limits, | ||
| bool) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name here too please
src/binary-reader.cc
Outdated
| bool has_init_expr = false; | ||
|
|
||
| if (options_.features.function_references_enabled() && | ||
| state_.offset < read_end_ && state_.data[state_.offset] == 0x40) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this 0x40 value? Maybe at least deserves a comment, if not a name.
It looks like you are peeking at the next byte here.. is there perhaps a helper function for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the comment. I could not find a helper for peaking a byte, probably this is something new as well. I will add something then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose a different direction at the end. It does not need peak, but some code duplication is needed. However, it is clearer, that the 0x40 is the leb32 representation of Type::Void.
|
|
||
| uint8_t ElemSegment::GetFlags(const Module* module) const { | ||
| uint8_t ElemSegment::GetFlags(const Module* module, | ||
| bool function_references_enabled) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a little odd that that we don't have any other features in ir.cc, I guess we have gotten away to a feature-agnostic IR up until this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, new features are backward compatible, so you don't need flags for them (the parser will check the feature). This is probably the first exception.
src/shared-validator.cc
Outdated
| !options_.features.reference_types_enabled()) { | ||
| if (options_.features.reference_types_enabled()) { | ||
| if (!elem_type.IsRef()) { | ||
| result |= PrintError(loc, "tables must have reference types"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "tables may only contain reference types"? i'm not sure its any better actually :)
src/validator.cc
Outdated
|
|
||
| if (actual_type == Type::FuncRef && expected_type == Type::RefNull) { | ||
| // Specification tests pass expected functions | ||
| // directly, their type is not relevant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this comment here, does it need updating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the description. There is a test: (assert_return (invoke "ref") (ref.null)), where the value is null without any actual type. Unfortunately we only get the types, not the values, so this check is not perfect, but I have no idea how to do this differently.
src/validator.cc
Outdated
| result_ |= | ||
| validator_.OnTable(field.loc, table.elem_type, table.elem_limits); | ||
| result_ |= validator_.OnTable(field.loc, table.elem_type, | ||
| table.elem_limits, true, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to have Enums to avoid all these true / false arguments, but not a huge deal, and I think we have a lot of them already probably.
src/wast-parser.cc
Outdated
|
|
||
| if (type->GetReferenceIndex() != kInvalidIndex) { | ||
| // Resolved earlier. | ||
| // Index or resolved earlier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment doesn't seem to maker sense. Is there a typo?
|
What do you think about re-titling: "Support table initializer expressions; completes function references support" |
22c1d56 to
518626e
Compare
|
Thank you for the review. I changed the title, and I hope I fixed every request. Please check the patch again. |
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.