wasm2c: implement the reference-types proposal #1887

keithw · 2022-04-04T11:31:59Z

~~(This PR is sequenced behind #1875, #1814, #1877 and #1999.)~~

This PR adds support for the reference-types proposal (per #1853 (comment)). This makes it possible to run all of the non-SIMD spec tests in the current testsuite.

Closes #1737. Closes #1985.

sbc100

Awesome! Just a few more nits and we good to land this I think.

sbc100 · 2022-10-01T15:46:26Z

wasm2c/wasm-rt.h

+ */
+void wasm_rt_free_funcref_table(wasm_rt_funcref_table_t*);
+
+/** Initialize an externref Table object with an element count of `elements` and


Can you start these javadoc-style comments with a /** on a line by itself?

wasm2c/wasm-rt-impl.c

sbc100 · 2022-10-01T15:54:41Z

wasm2c/examples/fac/fac.c

@@ -485,58 +485,96 @@ static inline void memory_init(wasm_rt_memory_t* dest,

 typedef struct {
  uint32_t func_type_index;
-  wasm_rt_funcref_t func;
+  wasm_rt_function_ptr_t func;
  size_t module_offset;
 } wasm_elem_segment_expr_t;


It seems like this type could/should be unified with wasm_rt_funcref_t... what happens if an elem segment contains funcrefs to imported functions? (or is that not possible?). It seems like this would only be able to represent module-local functions in elem segments?

Re: what happens when an elem segment contains funcrefs to imported functions, I believe this is tested in the testsuite (e.g. in linking.wast) so we definitely have to handle it. The module_offset field is what makes this possible. (FWIW this code was added already in handling bulk memory ops.)

Re: unifying the types, the idea behind having these separate was that a wasm_rt_funcref_t includes a pointer to an instance of the originating module, which is what you want at runtime when calling a funcref. But when CWriter is representing the elem segment's initializing expr, we don't have the instance of the originating module because the module hasn't been instantiated yet. So instead the elem segment expr contains basically a pointer-to-member; the offset within the module instance where that function's originating module instance will be found, so that at runtime, funcref_table_init (previously table_init) can properly initialize the table.

Also, the wasm_elem_segment_expr_t stores the function's type index within the module, whereas a wasm_rt_funcref_t contains the global opaque type ID returned by wasm_rt_register_func_type (only known at runtime).

wasm2c/examples/fac/fac.c

test/wasm2c/old-spec/README.md

src/c-writer.cc

sbc100 · 2022-10-01T16:10:55Z

src/c-writer.cc

-        Write("table_init(", ExternalInstancePtr(dest_table->name), ", ");
+        if (dest_table->elem_type != src_segment->elem_type) {
+          WABT_UNREACHABLE;
+        }


Should this be an assert instead? (I guess it would be validation error if this were true.. so it should be impossible for that to happen here?)

I agree this should be a validation error, but my understanding is that the validator doesn't currently catch this one. :-( Per #1612 . So it seems like this should abort (ideally with a helpful error message) even on a Release build.

Still, why not use assert here?

My thinking was that an assert isn't strong enough here, because cmake disables asserts on Release builds. :-/ If we made this an assert, then a user could take an invalid module like this:

(module (table 1 externref) (func $f) (elem (i32.const 0) func $f) )

and successfully pass it through wat2wasm and wasm2c (release build) and all they would get is a warning from the C compiler when compiling the generated code (warning: passing argument 1 of ‘funcref_table_init’ from incompatible pointer type; note: expected ‘wasm_rt_funcref_table_t *’ but argument is of type ‘wasm_rt_externref_table_t *’), and unless they compile everything with -Werror, the module would end up with the ability to create a garbage externref that the host never gave it. That felt kind of icky to have as a known bug that's only caught on non-Release builds.

True. but UNREACHABLE isn't much better as an end user experience is it? Wouldn't it manifest as a crash, which then would require debugger to figure out what happens? IIRC it doesn't even given a line number.

I guess the real fix is to update the validator. How we add a TODO that says something like "This should really be and assert since the validator should catch this, but currently it doesn't?"

sbc100 · 2022-10-01T16:11:04Z

src/c-writer.cc

-        Write("table_copy(", ExternalInstancePtr(dest_table->name), ", ",
+        if (dest_table->elem_type != src_table->elem_type) {
+          WABT_UNREACHABLE;
+        }


assert instead?

(same as previous)

sbc100 · 2022-10-01T16:13:05Z

src/c-writer.cc

+const char* CWriter::GetReferenceNullValue(const Type& type) {
+  switch (type) {
+    case Type::FuncRef:
+      return "(wasm_rt_funcref_t){0, NULL, 0}";


Can we define wasm_rt_funcref_null_value too? Also, can we avoid having wasm_rt_externref_null_value in the wasm-rt.h header file if its only used here in the generated code?

Sure, done (wasm_rt_funcref_null_value). We wanted wasm_rt_externref_null_value to be in the wasm-rt.h header so that if the embedder wants to choose a different type for wasm_rt_externref_t (something other than a void*), then the embedder can also specify the appropriate null value. (In Fix we're using externref = m256 instead of void*.)

Restores current versions of all non-SIMD tests in the core testsuite and multi-memory and exception-handling proposals.

sbc100 · 2022-10-02T21:16:55Z

wasm2c/wasm-rt.h

-void wasm_rt_free_table(wasm_rt_table_t*);
+void wasm_rt_free_externref_table(wasm_rt_externref_table_t*);
+
+/** Grow a Table object by `delta` elements (giving the new elements the value


Can you put opening and closing comment marks on lines by themsevles.

sbc100

lgtm with a final comments

Restores current versions of all non-SIMD tests in the core testsuite and multi-memory and exception-handling proposals.

keithw force-pushed the reference-types branch 2 times, most recently from 06a6ac5 to f939c50 Compare April 4, 2022 11:38

keithw changed the title ~~wasm2c: implement reference types~~ wasm2c: implement the reference-types proposal Apr 4, 2022

keithw requested a review from sbc100 April 4, 2022 11:40

keithw mentioned this pull request Apr 4, 2022

wasm2c: support final (non-SIMD) spec tests by adding an unsafe "ignore validation failure" flag #1888

Closed

keithw requested a review from kripken April 4, 2022 17:18

keithw force-pushed the reference-types branch from ef394d8 to 86c3ac4 Compare April 5, 2022 04:10

keithw mentioned this pull request Apr 5, 2022

wasm2c: implement the bulk memory operations proposal #1877

Merged

keithw force-pushed the reference-types branch 2 times, most recently from a5b4343 to bc1fa1a Compare April 6, 2022 11:34

This was referenced Apr 6, 2022

wasm2c: add support for module instancing #1814

Merged

Remove signature mangling from wasm2c output #1896

Merged

keithw force-pushed the reference-types branch from bc1fa1a to b90ca98 Compare April 28, 2022 05:50

keithw force-pushed the reference-types branch from b90ca98 to 1a16165 Compare May 15, 2022 06:05

keithw mentioned this pull request Jun 1, 2022

wasm2c: implement the exception-handling proposal #1930

Merged

keithw mentioned this pull request Jun 30, 2022

wasm2c: run tests with -O2 on non-Windows #1939

Merged

sbc100 mentioned this pull request Jul 12, 2022

wasm2c: Support table.size #1944

Closed

keithw force-pushed the reference-types branch 4 times, most recently from 9e2d69b to ea15fd5 Compare July 15, 2022 06:40

keithw force-pushed the reference-types branch 5 times, most recently from 62aaba6 to 1e23777 Compare August 17, 2022 15:48

keithw force-pushed the reference-types branch from 1e23777 to acb6ed0 Compare August 24, 2022 15:17

keithw force-pushed the reference-types branch 2 times, most recently from a6c6be3 to 99d3b78 Compare September 16, 2022 16:58

keithw mentioned this pull request Sep 17, 2022

Aborted in CWriter::Write at wasm2c #1985

Closed

keithw mentioned this pull request Sep 20, 2022

Implement Relaxed SIMD proposal #1994

Merged

keithw force-pushed the reference-types branch 4 times, most recently from 81db43d to 703a9c7 Compare September 21, 2022 08:23

keithw mentioned this pull request Sep 21, 2022

Finish updating testsuite #2001

Merged

keithw force-pushed the reference-types branch 2 times, most recently from 6114337 to 2f352ab Compare October 1, 2022 05:25

sbc100 reviewed Oct 1, 2022

View reviewed changes

keithw mentioned this pull request Oct 2, 2022

wasm2c templated code: simplify table_copy to use memmove #2012

Merged

keithw force-pushed the reference-types branch 2 times, most recently from a9a0a7b to 028c068 Compare October 2, 2022 06:50

keithw added 4 commits October 2, 2022 10:22

wasm2c: implement the reference-types proposal

43de8f5

wasm2c: restore current versions of non-SIMD tests

858edef

Restores current versions of all non-SIMD tests in the core testsuite and multi-memory and exception-handling proposals.

Respond to review

a64e485

c-writer.cc: Fix formatting on single-line switch cases

87f8cd7

keithw force-pushed the reference-types branch from 028c068 to 87f8cd7 Compare October 2, 2022 20:34

sbc100 reviewed Oct 2, 2022

View reviewed changes

sbc100 approved these changes Oct 2, 2022

View reviewed changes

keithw added 2 commits October 2, 2022 15:59

wasm-rt.h: Fix javadoc-style comment

a3bde82

c-writer.cc: Add clarifying TODO to WABT_UNREACHABLEs

2100a9f

keithw enabled auto-merge (squash) October 3, 2022 06:08

keithw merged commit cbc5684 into WebAssembly:main Oct 3, 2022

keithw deleted the reference-types branch October 4, 2022 00:42

wasm2c: implement the reference-types proposal #1887

wasm2c: implement the reference-types proposal #1887

Uh oh!

Conversation

keithw commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithw Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithw Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithw Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keithw commented Apr 4, 2022 •

edited

Loading

keithw Oct 2, 2022 •

edited

Loading

keithw Oct 2, 2022 •

edited

Loading

keithw Oct 3, 2022 •

edited

Loading