[RT] Add type to tables and element segments #3763

martianboy · 2021-03-31T15:17:01Z

This will add support for other reference types to tables and elems,
which was introduced in reference-types proposal and extended in typed
function references proposal. Non-nullable types are not supported yet,
since the spec only mentions the idea of using an initializer, but
it has remained a TODO item to this date.
Spec tests have also been partially updated to reflect the new changes
from upstream.

martianboy · 2021-03-31T15:32:10Z

This includes and is based on the changes from #3753, so I marked this a draft until that one is merged.

@tlively @kripken @aheejin It's possible that I have some misunderstandings regarding the spec, specially when it comes to the new types introduced in GC proposal. Please take a look and let me know if something is incorrect. Thanks!

martianboy · 2021-03-31T21:03:13Z

src/ir/module-utils.h

+  }
+  for (auto& curr : wasm.elementSegments) {
+    counts.maybeNote(curr->type);
+  }


@tlively is this enough for making sure table & elem types are counted correctly everywhere, or have I missed something?

This looks good to me!

martianboy · 2021-03-31T21:04:24Z

This is now ready for review, I've rebased it on top of the current main after #3753 was merged.

tlively

Nice work! I only have a few questions :)

tlively · 2021-03-31T22:01:31Z

src/passes/Print.cpp

      } else {
-        TypeNamePrinter(o, currModule).print(Type::funcref);
+        printType(o, curr->type, currModule);


Is this supposed to print the value type (e.g. anyref) or the heap type (e.g. any)?

This should be a value type. My understanding is that the only heap type allowed here is func which is used in the abbreviated form that doesn't use expressions. In all other cases, it should be a value type and items should be expressions.

tlively · 2021-03-31T22:08:45Z

src/shell-interface.h

+        table->initial = 10;
+        table->max = 20;


Where do these numbers come from?

From the reference interpreter! This is an old mechanism for to link a few default global when running spec tests. More recent tests have (register ... instruction for this purpose, although we don't support it yet in wasm-shell.

I was trying to get more spec tests to pass, so I decided to dig into the differences between wasm-shell and the reference interpreter. Looks like table initialization from element segments is fully handled at module instantiation, including the bounds check. We check bounds at validation, which still makes this change useless. There are test cases like:

(module (import "spectest" "table" (table 0 funcref)) (func $f) (elem (i32.const 0) $f) )

that are a validation error in wasm-shell but pass in the reference interpreter, because the former (incorrectly) takes the table's initial to be 0, whereas the latter imports the table instance and sees that it has an initial of 10.

Again, to be clear, these difference haven't been fully addressed in this PR, so I may remove these from this PR as well if you think that's cleaner.

tlively · 2021-03-31T22:43:21Z

src/wasm/wasm-binary.cpp

-    if (elemType != BinaryConsts::EncodedType::funcref) {
-      throwError("Non-funcref tables not yet supported");
+    auto elemType = getType();
+    if (!elemType.isRef()) {


Does this handle MVP tables correctly, too?

In MVP all tables should be funcref, right? We defer checking features until validation.

tlively · 2021-03-31T22:54:02Z

src/wasm/wasm-s-parser.cpp

@@ -3205,7 +3205,7 @@ void SExpressionWasmBuilder::parseTable(Element& s, bool preParseImport) {
      table->module = inner[1]->str();
      table->base = inner[2]->str();
      i++;
-    } else {
+    } else if (!elementStartsWith(inner, REF)) {


Is this meant to ensure that tables have reference types? It seems like this wouldn't support abbreviations like funcref or anyref. I assume that those should be supported?

No, we are here after we saw s[i] to be a list. In that case, we still don't want to parse the type yet, just making sure it passes over the parse exception.

tlively · 2021-03-31T22:56:33Z

test/spec/elem_reftypes.wast

+;; (module
+;;   (import "spectest" "table" (table 0 funcref))
+;;   (func $f)
+;;   (elem (i32.const 0) $f)
+;; )


Why are these tests commented out? It would be good to add a comment.

I explained the reasons above, but sure I'll add a comment. Thanks!

I added a comment explaining the reason.

tlively · 2021-03-31T23:02:02Z

src/wasm/wasm-validator.cpp

+    info.shouldBeTrue(segment->type != Type::externref,
+                      "elem",
+                      "element segment type cannot be externref");


This is surprising!

Again, this is only my understanding of the spec but haven't seen the spec text mention it anywhere, nor any spec tests that expects this to be invalid. But, here's my reasoning:

Element segments are only used to initialize tables and cannot be imported from outside. Given the fact that they cannot include function call expressions, there's no way for them to hold externref values. The only way for a wasm module to change items on an externref table is via the table instructions like table.set and table.copy that are allowed at runtime.

This is now changed to only allow function types for element segments. I added a comment block explaining the reason.

src/wasm/wasm-validator.cpp

This will add support for other reference types to tables and elems.

kripken

Nice work! No more comments from me.

tlively · 2021-04-05T20:09:37Z

I fuzzed this a bit and it looks like the fuzzer is producing invalid modules for seed 7344750093000028152. Can you reproduce that? (Besides fuzzing this LGTM, too!)

martianboy · 2021-04-05T20:20:33Z

@tlively thanks I will look into it!

modules with more specific function type segments weren't handled correctly. We now ensure that a funcref element segments and and table exist in `setupTables`. Also when adding functions to a segment, first we find one that has a compatible type.

martianboy · 2021-04-05T21:42:00Z

@tlively Thanks for the catch! The fuzzer wasn't correctly handling modules that only have element segments with more specific function types. I fixed it by making sure at least one funcref element segment and table exist. See my last commit. On my local machine, the fuzzer can now run a few hundred iterations without a problem.

tlively · 2021-04-06T19:33:15Z

Excellent, thanks @martianboy!

martianboy force-pushed the table-types branch 2 times, most recently from d1cabad to e8206b1 Compare March 31, 2021 20:31

martianboy marked this pull request as ready for review March 31, 2021 20:49

martianboy commented Mar 31, 2021

View reviewed changes

tlively reviewed Mar 31, 2021

View reviewed changes

kripken reviewed Apr 1, 2021

View reviewed changes

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

martianboy commented Apr 1, 2021

View reviewed changes

src/wasm/wasm-validator.cpp Show resolved Hide resolved

kripken reviewed Apr 1, 2021

View reviewed changes

src/wasm/wasm-validator.cpp Outdated Show resolved Hide resolved

martianboy mentioned this pull request Apr 2, 2021

Reorder global definitions in Print pass #3770

Merged

[RT] Add type to tables and elem segments

7f4bc4c

This will add support for other reference types to tables and elems.

martianboy force-pushed the table-types branch from 3961dfb to 7f4bc4c Compare April 2, 2021 17:18

martianboy added 2 commits April 2, 2021 22:03

update tests

bde8f94

a little bit of polish

fb2abda

martianboy requested review from tlively and kripken April 2, 2021 19:45

martianboy added 2 commits April 3, 2021 01:04

wasm2js: error out if there's a non-function table

a5da3d4

make FlatTable invalid if it's non-function type

eb76258

kripken approved these changes Apr 3, 2021

View reviewed changes

fix the fuzzer

2b07dc9

modules with more specific function type segments weren't handled correctly. We now ensure that a funcref element segments and and table exist in `setupTables`. Also when adding functions to a segment, first we find one that has a compatible type.

martianboy added 3 commits April 6, 2021 02:15

trigger another build

8c3e93e

and another...

900b176

Merge remote-tracking branch 'upstream/main' into table-types

af98cc6

tlively merged commit e9fcf44 into WebAssembly:main Apr 6, 2021

[RT] Add type to tables and element segments #3763

[RT] Add type to tables and element segments #3763

Uh oh!

Conversation

martianboy commented Mar 31, 2021

Uh oh!

martianboy commented Mar 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martianboy commented Mar 31, 2021

Uh oh!

tlively 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!

Choose a reason for hiding this comment

Uh oh!

martianboy Apr 1, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

tlively commented Apr 5, 2021

Uh oh!

martianboy commented Apr 5, 2021

Uh oh!

martianboy commented Apr 5, 2021

Uh oh!

tlively commented Apr 6, 2021

Uh oh!

Uh oh!

martianboy Apr 1, 2021 •

edited

Loading