Add initial support for anyref as an opaque type by jayphelps · Pull Request #2294 · WebAssembly/binaryen

jayphelps · 2019-08-10T05:39:07Z

Another round of trying to push upstream things from my fork.

This PR only adds support for anyref itself as an opaque type. It does NOT implement the full reference types proposal--so no table.get/set/grow/etc or ref.null, ref.func, etc.

I'll also be adding a few github comments to places I want to point out/question.

jayphelps · 2019-08-10T05:42:03Z

    }
    // There's no binary ops for exnref
-    if (type == exnref) {
+    if (type == exnref || type == anyref) {


This file is particularly unfamiliar to me, so I'm not entirely sure what I should be doing. I'm familiar with fuzzing, just not this code and how it goes about it. Pointed suggestions are very welcome, if I did the wrong thing.

Reference types are practically not supported by the fuzzer at this point. I added only a bit of boilerplate for exnref just to make it compile, but we don't really generate any instructions that deal with reference types yet. I guess at this point we can leave this and mark it as TODO or something here, and add real fuzzer support when we are going to generate real instructions in the fuzzer.

This line is not really correct; I just made it compile, becasue we can't generate binary instructions that produce a reftype value. (This is true for other types of instructions, such as unary, load, store, ...) I guess we have to refactor the fuzzer to correctly support reference types at some point in the future.

Anyway, this line can be

if (isReferenceType(type)) {

jayphelps · 2019-08-10T05:43:57Z

  AnyFunc = -0x10, // 0x70
-  // reference type
+  // opaque reference type
+  anyref = -0x11, // 0x6f


I boosted this from wabt because I couldn't find the values in the proposal doc.

This value looks correct, but anyway just in case you'd like to see the current version of the reference types spec, it is here: https://webassembly.github.io/reference-types/core/_download/WebAssembly.pdf
And the repo is here: https://github.com/WebAssembly/reference-types/tree/master/document

jayphelps · 2019-08-10T05:47:18Z

  def test_emit_all_features(self):
    p = run_process(WASM_OPT + ['--emit-target-features', '-all', '-o', '-'],
-                    input="(module)", check=False, capture_output=True)
+                    input="(module)", check=False, capture_output=True, decode_output=False)


This is for a problem that stumped me for a while. What seems to have happened is that previously the code below would always try to decode as utf-8 however with the new addition of one more feature flag, it pushes the binary representation of the user section size over 127 (it was literally 127 exactly before) causing it to be "malformed" utf-8. Correct me if I'm wrong.

I'm not familiar enough with all the moving pieces of the testing suite to know if there's a more elegant solution. I'm all ears!

Yes, I think this method was written assuming the output is text. While I'm not sure if adding the option is the best way to fix this, we actually have another similar method run_command elsewhere, so I guess future refactoring should combine these methods or something and also take care of both text and binary output. I don't think that should be included in this PR, of course.

So, I'm not very sure about what the best quick fix should be :) I don't mind much about the current option, but maybe cc @tlively

I would prefer a solution that just never decodes the output instead of adding another parameter. I’m fine with doing that cleanup in a separate PR, though, if it touches a bunch of unrelated tests.

jayphelps · 2019-08-10T06:41:59Z

  f32,
  f64,
  v128,
+  anyref,


does this need to be moved to be the last leg of the enum so the values of the existing legs don't shift or non-issue?

I don't think so..? Why should it matter?

It might matter to someone using the C API in whatever language and they create their own enum with these values instead of using the BinaryenTypeInt32() functions since some languages (e.g. C++) can't switch through non-constants. FWIW this is what I'm doing (using C API from a new language, with my own enum), so this could have broken me if this wasn't already coming from my fork. Whether what I'm doing is kosher or not is debatable 🤡

Wasn't sure how careful binaryen tries to be with breaking things for folks. I imagine it's not worth the effort at this point, the number of compilers using Binaryen I assume isn't massive.

I think people shouldn't rely on the values of enums. But just in case if you are worried about it, you can append anyref at the end of the list I think, rather than inserting it in the middle..?

Friendly ping

oops sorry. I don't care enough to move it because I'm pretty confident it will happen many many many times in the future and won't be thought about because it's super easy to not realize might be depending on it implicitly. I'd rather have the enum arm order make more logical sense internally. (in this case, exnref being after anyref makes more sense)

I mostly wanted to point it out in case you or the other maintainers already had decided to make this a stable, public API.

I did mention my use case, but it was less "keep this stable for me" and more just an example of what others might do too, now or eventually. Personally I look at binaryen as signing up for a little bleeding edge since the Wasm spec is rapidly changing (relatively speaking as far as specs go)

I see, let's leave this as is then.

jayphelps · 2019-08-10T06:51:10Z

binaryen.js updated. Still some flake8 issues, but I'm not sure how to fix them. The line:column it shows don't make sense to me or match up with the lint errors. There are also errors for code I haven't changed. Is there a --fix flag or something I can run to autoformat? (doesn't look like it, without other tools)

./auto_update_tests.py:18:1: F401 'shutil' imported but unused
./auto_update_tests.py:230:7: E265 block comment should start with '# '
./auto_update_tests.py:231:9: E116 unexpected indentation (comment)
./auto_update_tests.py:232:9: E116 unexpected indentation (comment)
./auto_update_tests.py:232:9: E265 block comment should start with '# '
./test/unit/test_features.py:169:1: E302 expected 2 blank lines, found 1
./scripts/test/shared.py:269:1: E302 expected 2 blank lines, found 1

jayphelps · 2019-08-10T21:15:12Z

All green.

aheejin

Nice! While you're at it, how about adding functype too, which should be similar?

aheejin · 2019-08-11T14:11:50Z

  f32,
  f64,
  v128,
+  anyref,


I don't think so..? Why should it matter?

aheejin · 2019-08-11T14:21:15Z

    }
    // There's no binary ops for exnref
-    if (type == exnref) {
+    if (type == exnref || type == anyref) {


Reference types are practically not supported by the fuzzer at this point. I added only a bit of boilerplate for exnref just to make it compile, but we don't really generate any instructions that deal with reference types yet. I guess at this point we can leave this and mark it as TODO or something here, and add real fuzzer support when we are going to generate real instructions in the fuzzer.

This line is not really correct; I just made it compile, becasue we can't generate binary instructions that produce a reftype value. (This is true for other types of instructions, such as unary, load, store, ...) I guess we have to refactor the fuzzer to correctly support reference types at some point in the future.

Anyway, this line can be

if (isReferenceType(type)) {

jayphelps · 2019-08-11T18:10:27Z

Thanks for the review! I've made all the requested changes.

Nice! While you're at it, how about adding functype too, which should be similar?

Unfortunately I don't have a large number of free cycles at the moment and this code was mostly coming from a fork I have which doesn't contain functype yet. I'd need to fully grok the spec on them, which I'd definitely plan to do but wanted to at least get something in as I imagine anyref alone is helpful to some folks like it was me.

Lmk if this PR is blocked on them and I'll try and get to them in the coming weeks. 🤙

aheejin

LGTM to me, but maybe it'd be better to wait for a bit before landing this in case other people have comments.

jayphelps · 2019-08-16T02:43:53Z

rebased to resolve conflicts

jayphelps · 2019-08-16T23:32:10Z

rebased to resolve conflicts

aheejin · 2019-08-20T09:06:05Z

Thank you! By the way, have you seen #2294 (comment)?

jayphelps commented Aug 10, 2019

View reviewed changes

jayphelps force-pushed the anyref branch from e46df67 to 1cb732d Compare August 10, 2019 06:39

jayphelps commented Aug 10, 2019

View reviewed changes

jayphelps force-pushed the anyref branch 3 times, most recently from e32e35b to c4613d1 Compare August 10, 2019 17:59

jayphelps force-pushed the anyref branch 2 times, most recently from 5af9f11 to 9bfcbca Compare August 11, 2019 06:49

aheejin reviewed Aug 11, 2019

View reviewed changes

jayphelps force-pushed the anyref branch from 9bfcbca to ff05dc1 Compare August 11, 2019 18:02

aheejin approved these changes Aug 12, 2019

View reviewed changes

jayphelps force-pushed the anyref branch from ff05dc1 to 74e531d Compare August 16, 2019 01:32

Add initial support for anyref as an opaque type

f62cd22

jayphelps force-pushed the anyref branch from 74e531d to f62cd22 Compare August 16, 2019 22:53

aheejin merged commit dce1fe8 into WebAssembly:master Aug 20, 2019

dcodeIO mentioned this pull request Nov 16, 2019

Reference types for phase 4? WebAssembly/reference-types#61

Closed

Conversation

jayphelps commented Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayphelps Aug 10, 2019 • 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!

jayphelps Aug 10, 2019 • 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!

jayphelps Aug 11, 2019 • 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!

jayphelps Aug 20, 2019 • 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!

jayphelps commented Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayphelps commented Aug 10, 2019

Uh oh!

aheejin left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayphelps commented Aug 11, 2019

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

jayphelps commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayphelps commented Aug 16, 2019

Uh oh!

aheejin commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jayphelps commented Aug 10, 2019 •

edited

Loading

jayphelps Aug 10, 2019 •

edited

Loading

jayphelps Aug 10, 2019 •

edited

Loading

jayphelps Aug 11, 2019 •

edited

Loading

jayphelps Aug 20, 2019 •

edited

Loading

jayphelps commented Aug 10, 2019 •

edited

Loading

jayphelps commented Aug 16, 2019 •

edited

Loading

aheejin commented Aug 20, 2019 •

edited

Loading