wabt icon indicating copy to clipboard operation
wabt copied to clipboard

Add support for function references proposal

Open zherczeg opened this issue 8 months ago • 20 comments
trafficstars

This large patch adds function references support for those parts of the wabt library, which already present in the code. The current code is designed for an older (outdated) variant of the function references proposal.

Key changes:

  • Var variables can optionally store type data. This way there is no need for a Var/Type field pair in many cases.
  • CallRef now needs to explicitly provide its reference type (no auto detection)
  • Tracking local reference initialization (non nullable refs must be set before using them)
  • Supporting type equality comparisons in the validator and in the interpreter
  • Adds a code for reading/writing 33 bit integers (leb format)
  • Remove EndTypeSection in shared validator, types can be validated earlier
  • Improve named reference resolving in the text parser

zherczeg avatar Mar 11 '25 10:03 zherczeg

@sbc100 @keithw since I don't know enough about the project, these are just some random changes in the parser.

The aim is parsing https://github.com/WebAssembly/function-references/blob/main/test/core/call_ref.wast

It is doing something (parsing correctly is surely an exaggeration) with call_ref $ii and ref null $ii. The next item is ref.null $ii. It looks like it will need API changes, since a token type will not be enough. Would adding an index to it be a good idea?

Could you check that these changes makes sense? There is a Type::Reference enum in the code, but function reference introduces two other enums, and does not use Reference. This is strange to me.

Thank you for the help.

zherczeg avatar Mar 11 '25 11:03 zherczeg

This patch goes to a new direction. The 32 bit type filed of Var is split into two 16 bit fields. The second can be used to store WebAssembly types (negative 7 bit numbers). This way Var can be used in a more generic way.

zherczeg avatar Mar 12 '25 05:03 zherczeg

There is something I don't understand. There is a Reference = -0x15 in type.h. However, in https://github.com/WebAssembly/function-references/blob/main/proposals/function-references/Overview.md (ref ht) is defined as -0x1c and (ref null ht) is -0x1d. Is the old code obsolete?

The key change of the patch is that Var can also behave like Type, and transformation between the two is possible after names are resolved.

Btw, may I ask how actively maintained the wabt project?

zherczeg avatar Mar 13 '25 16:03 zherczeg

I believe ht refers to Heap Type which would not be needed for function references. I believe heap types refer to things like structs and arrays in the GC proposal.

sbc100 avatar Mar 13 '25 17:03 sbc100

That is possible. Unfortunately not everything is described clearly in the text. For example, ref.null ht: [] -> [(ref null ht)] Which encoding is used for ht here?

https://github.com/WebAssembly/function-references/blob/main/proposals/function-references/Overview.md

zherczeg avatar Mar 13 '25 18:03 zherczeg

I have tried to track down the v8 implementation for ref.null.

https://github.com/v8/v8/blob/main/src/wasm/function-body-decoder-impl.h#L386

It calls read_heap_type. It is in the same file.

https://github.com/v8/v8/blob/main/src/wasm/function-body-decoder-impl.h#L217

Starts with: decoder->read_i33v<ValidationTag>(pc, "heap type");

So it looks like it uses the s33 decoding which the specification mentioned.

zherczeg avatar Mar 14 '25 00:03 zherczeg

I have some good news! This test is fully working now (including the interpreter): https://github.com/WebAssembly/function-references/blob/main/test/core/call_ref.wast

Only 9 fails remained, they all look like CallRef syntax change related, so some tests need to be updated.

The patch is quite large. There are some parts which are questionable. I will comment about these parts later to help review.

zherczeg avatar Mar 14 '25 17:03 zherczeg

I think the patch is ready. There are lots of follow-up work ahead.

zherczeg avatar Mar 16 '25 04:03 zherczeg

Who should I cc to get review for this patch? Thank you!

zherczeg avatar Mar 18 '25 09:03 zherczeg

This patch and #2567 covers the majority of function reference proposal. There are still missing pieces, such as compile time checking that all non-null references are set before use, and typed tables.

Is there anybody here who can review these patches or how reviewing works here?

zherczeg avatar Mar 21 '25 07:03 zherczeg

I got no reply for my previous comment. This patch perfectly aligns with the aims of the project, it improves the current implementation, and still there is no feedback. I suspect there is something I don't know. Another project was mentioned in #2551 . Is this project in some kind of a maintenance mode, and improvements are not accepted anymore? There is no problem with negative news as long as it is clearly communicated.

zherczeg avatar Mar 24 '25 08:03 zherczeg

expect slow responses, we don't know about other maintainers but we've been a bit busy... will review when we have time

SoniEx2 avatar Mar 24 '25 10:03 SoniEx2

Patch is rebased on the top of #2571 Thanks to the fixes, it is now < 900 lines of new code including test changes.

zherczeg avatar Apr 04 '25 10:04 zherczeg

This patch now fails with a fuzzer error. Reason:

https://github.com/WebAssembly/wabt/blob/main/fuzzers/read_binary_interp_fuzzer.cc#L37 The comment explicitly says that stop_on_first_error must be false.

However: https://github.com/WebAssembly/wabt/blob/main/src/interp/binary-reader-interp.cc#L634 This check can fail now. The failed fuzzer test has a (ref 5) global type, and there is no type 5, so this is correct. Then the global is not added to module_.globals. Furthermore, the binary parsing does not stop as well.

This time the exports are parsed and this line crashes: https://github.com/WebAssembly/wabt/blob/main/src/interp/binary-reader-interp.cc#L634

The validator_.OnExport does not fail, since it thinks 21 globals are available, although the parsing is failed after the first global, and all globals are skipped.

zherczeg avatar Apr 18 '25 08:04 zherczeg

I have a question. In this variant of the patch, Globals / Tables / etc. uses Type structures, and the parser may update the named references, and validate the index references. However, call_ref and ref.null uses Var structures. I don't know how types should be handled in general, since many WebAssembly structures uses Var, but types are optionally use references.

zherczeg avatar Apr 25 '25 06:04 zherczeg

This is the next patch for supporting the function references proposal. It focuses on improving the wabt supported elements of WebAssembly. It is a feature patch now, the bugfixes were extracted and landed as separate patches.

The key concept changes of this patch:

  • Type comparison (type-checker) requires the types hashmap (shared-validator). This circular dependency can be broken by a new type comparison class, which is used by both type-checker and shared-validator.

  • Vars can optionally store types now without increasing its structure size. This simplify passing information, but does not necessary fit for the design concepts of the wabt project.

  • RefNull and CallRef use (the new) Vars, instead of types. Many structures uses Vars in the internal representation, so this is not necessary new. However, using Vars for types is something new and debatable.

Please let me know your opinion about these design concepts. Since the parser can store pointers to types, and check/resolve them later, it is possible to use types only. But using Vars directly is not necessary a problem.

zherczeg avatar Apr 28 '25 05:04 zherczeg

I have updated the description

zherczeg avatar May 21 '25 19:05 zherczeg

I hope I did all the requests. Thank you for the review!

zherczeg avatar May 21 '25 19:05 zherczeg

I have found a bug in RecursiveMatch and fixed it. It seems there will be no other comments to this patch, @sbc100 you are my only hope.

zherczeg avatar Jun 03 '25 10:06 zherczeg

I have found a bug in RecursiveMatch and fixed it. It seems there will be no other comments to this patch, @sbc100 you are my only hope.

I think this change is mostly looking good. I'm still trying to find some time to really understand what is going on with the Var / opt_type thing as it seems a little odd to me still.

sbc100 avatar Jun 03 '25 14:06 sbc100

I'm on vacation for the next couple of week, but I will make a concerted effort to review this change when I get back in July.

sbc100 avatar Jun 19 '25 13:06 sbc100

Thank you. The GC support will also likely completed by that time. Only 5 instructions remained, all type cast related.

zherczeg avatar Jun 19 '25 14:06 zherczeg

It looks like WebAssembly 3.0 is released, and GC is part of it: https://webassembly.org/news/2025-09-17-wasm-3.0/ May I ask what is the plans for these patches?

zherczeg avatar Sep 24 '25 06:09 zherczeg

The title was misleading, I am sorry. I thought I can do this in one patch, but the full support is divided into three patches. I have updated the title.

zherczeg avatar Nov 07 '25 08:11 zherczeg

I have rebased and updated the patch. Please let me know if you need more changes.

zherczeg avatar Nov 07 '25 09:11 zherczeg

lgtm!

(What about update-spec-tests.py?)

sbc100 avatar Nov 07 '25 18:11 sbc100

(What about update-spec-tests.py?)

I think we should update it in the third patch when the feature is complete.

zherczeg avatar Nov 07 '25 18:11 zherczeg

Thank you Zoltan, and again sorry for the absurd review time.

sbc100 avatar Nov 07 '25 20:11 sbc100

No problem. These changes are rather complex which means higher risk from project perspective, and needs more time to review. There are two more patches for function references: #2567 implements new opcodes (I have rebased it) and #2593 implements table initializers. I will add README and update-spec-tests.py changes to the last one. Could you review them as well? Thank you!

zherczeg avatar Nov 08 '25 03:11 zherczeg