Add anyref feature and type #3109

dcodeIO · 2020-09-09T02:21:31Z

Adds a custom --enable-anyref feature enabling just the anyref type on top of reference types that allows us to test subtyping relationship (with externref, funcref and exnref) without having to enable the full set of GC features. Re-enables previously disabled tests.

dcodeIO · 2020-09-09T02:38:11Z

src/wasm/wasm-type.cpp

+    // FIXME: `anyref` is only valid here if the `anyref` feature is enabled,
+    // but this information is not available within `Type` alone.
+    return b.isRef() ? Type::anyref : Type::none;


Fixing this looks like it requires changing the signature of this function to getLeastUpperBound(Type a, Type b, FeatureSet features), with call sites aware of enabled features passing these in.

Only works this way currently because we are not generating, for example, an (if $condition $funcref $externref) where anyref is not enabled, but an API user can do so yielding an unexpected (otherwise not enabled) anyref here. Do you prefer to tackle this issue in this PR?

Unless I'm missing some use of this function, I believe it will only incorrectly return anyref for invalid code. As long as validation will separately catch that invalidity, I'm tempted to not do anything with this issue besides document it. WDYT?

Ah, it looks like this PR doesn't make any changes to validation yet. It would be good to fix that.

Have a feeling that this will resurface eventually, but since the fuzzer isn't complaining I guess it is fine for now. Is the comment ok, or would you prefer a little more detail?

Oh, going to check!

On a first glimpse it looks like the validator uses Type::isSubType for its checks. This then leads to a similar issue like this one for getLeastUpperBound, but for isSubType not knowing the enabled features I guess, where the check in isSubType is right == Type::anyref which can only happen in valid code if anyref is enabled.

Can you point me into the right direction what else needs to be updated in the validator?

On the other hand, there will be a(nother) validation error anyway if anyref is used but not enabled, via Type::getFeatures().

If this ends up generating anyref in other programs where anyref isn't enabled, eventually it will be caught by the validator, and that basically means one of Binaryen passes have a bug. So I think it's OK to keep this simple and not include types for now, but erroring out here might help Binaryen authors debug their code a little, or something. Anyway I think this should be OK for now.

I think isSubType will be even more OK, because it doesn't create anyref when it isn't enabled.

On the other hand, there will be a(nother) validation error anyway if anyref is used but not enabled, via Type::getFeatures().

Aha, I had forgotten that this wouldn't require any changes in the validator itself. I agree with @aheejin that this should be fine, then. We can probably replace the FIXME with a comment pointing out the issue but explaining that it will only happen with invalid code.

dcodeIO · 2020-09-09T02:46:40Z

src/tools/fuzzing.h

+        if (wasm.features.hasExceptionHandling()) {
+          options.push_back(Type::exnref);
+        }


Found that at most places, similar checks are not nested inside if (hasReferenceTypes). Not an urgent problem, but I'd imagine that this is going to lead to obscure issues where a user specifies --enable-anyref but not --enable-reference-types eventually.

One way to tackle this might be to implicitly enable reference types on setAnyref(true) respectively setExceptionHandling(true), and implicitly disable anyref and exception handling on setReferenceTypes(false)?

Yeah this is a tricky issue. So far we've avoided having any implicit dependencies between features to keep the mental model as simple and predictable as possible. One possibility would be to make it a validation error to have anyref enabled without having reference types enabled. That's a little more annoying for users, but on the other hand it keeps the features simple and there should not be real users of the --enable-anyref feature anyway.

Added a feature validation step, but naturally this now breaks the fuzzer because it doesn't know of implied features. Any suggestions?

Ended up with fb1a789 for now

I don't think the fuzzer will generate types that does not respect dependences. For example, we check for both FeatureSet::ExceptionHandling and FeatureSet::ReferenceTypes whenever we try to add exnref or other EH instructions (By the way I just realized my PR for generating EH instructions for the fuzzer hasn't landed and is pending for months, but anyway). So I guess the fuzzer error you've seen was just complaining about the feature combination itself, right?

So I'm wondering, why do we need to impose the dependences in validator? Unless giving --enable-exception-handling without --enable-reference-types itself in the command line of a VM is an error, I don't think we should treat it as an error.

I think it is a subtle difference; using exnref without enabling --enable-reference-types might be an error. But Wasm spec doesn't say giving --enable-exception-handling without also giving --enable-reference-types is an error, so..

From a user's perspective:

If making missing required features validation errors: "Oh, I see"

If refactoring all code locations: "Why isn't this option picked up, even though it's specified?"

If enabling features implicitly: "Why does my binary suddenly utilize reference types, even though it isn't specified?"

From our perspective:

If making missing required features validation errors: -

If refactoring all code locations: "Look, a new issue"

If enabling features implicitly: "Look, a new issue"

The initial concern was that at most places we have code like

if (hasReferenceTypes) { add(funcref) add(externref) } if (hasExceptionHandling) { add(exnref); } if (hasAnyref) { add(anyref); }

instead of

if (hasReferenceTypes) { add(funcref) add(externref) if (hasExceptionHandling) { add(exnref); } if (hasAnyref) { add(anyref); } }

so it seems possible to yield an exnref or anyref without reference types being enabled, or to otherwise get false positives, and a validation check lets us keep the code as-is.

I wonder where we do something like the first? The only place I can think of that creates exnref or anyref is the fuzzer, and I think it handles them correctly (or you handled them correctly in this PR). Validation or checking wouldn't be as problematic as creating them because they don't bother with anyref of exnref when they don't exist. I could be mistaken; if so please let me know.

Checking again, there is one place where we are doing the former

binaryen/src/passes/InstrumentLocals.cpp

Lines 170 to 193 in 916ce6f

if (curr->features.hasReferenceTypes()) {

addImport(curr,

get_funcref,

{Type::i32, Type::i32, Type::funcref},

Type::funcref);

addImport(curr,

set_funcref,

{Type::i32, Type::i32, Type::funcref},

Type::funcref);

addImport(curr,

get_externref,

{Type::i32, Type::i32, Type::externref},

Type::externref);

addImport(curr,

set_externref,

{Type::i32, Type::i32, Type::externref},

Type::externref);

}

if (curr->features.hasExceptionHandling()) {

addImport(

curr, get_exnref, {Type::i32, Type::i32, Type::exnref}, Type::exnref);

addImport(

curr, set_exnref, {Type::i32, Type::i32, Type::exnref}, Type::exnref);

}

and I somehow assumed that this is some sort of convention, even though it probably isn't. Would suggest to move the hasExceptionHandling into hasReferenceTypes there as well so there can't be instrumented get_exnref/set_exnref functions taking an exnref type if reference types is not enabled, but exception handling is. Validating separately still seems fine to me, though, for clarity.

(Haven't checked other locations where just hasExceptionHandling is checked, but hasReferenceTypes is not)

Thanks for finding this. That is a special pass that generates imports, and I agree that moving hasExceptionHandling condition into hasReferenceTypes is the right call.. Can you add that to this PR if possible?

But I don't think there are many, if at all, passes that generates anyref of exnref out of thin air. (Except for the fuzzer, but it is not a pass) One function that can generate them and can be called from Binaryen passes is getLeastUpperBound, as you pointed out, and if that function creates a type that's not valid that means we have a bug, and that will be caught by the validator. So I don't think there will be mass refactoring necessary.

I just checked what V8 and wabt do for feature dependencies, and actually they are both doing the implication thing, so if you only enable EH that automatically enables reference types too, even if you don't explicitly enable it. Maybe we should consider doing a similar thing in future.

dcodeIO · 2020-09-09T04:37:51Z

Some early fuzzing:

Invocations so far:
   FuzzExec: 5875
   CompareVMs: 1441
   CheckDeterminism: 510
   Wasm2JS: 1351
   Asyncify: 1412

ITERATION: 6816

tlively · 2020-09-09T06:27:25Z

This LGTM once those two conversations are resolved 👍

aheejin · 2020-09-09T06:36:07Z

Thanks for doing this. I quickly glanced over a few re-enabled subtype tests and I am a little confused; some tests seem to have their supertype-subtype order reversed compared to the original tests; and it is hard to check that because, disabled tests in #3084 already have their types changed.

The one I discovered was done by comparing my local code (without changes from #3084) and this PR. I just started looking at this code just now, but can you hold off on landing this?

dcodeIO · 2020-09-09T06:40:05Z

Yeah, these changes can be confusing. Previously we had

nullref <: externref | funcref | exnref

with nullref being a common subtype, but now we have

externref | funcref | exnref <: anyref

wit anyref being a common supertype, effectively reversing the order.

aheejin · 2020-09-09T06:52:39Z

Oh sorry. You're right, and I was aware of that. I thought the case I meant was not that and really had a relationship reversed, but I just realized that the line wrapping changed and I was confused by that. Anyway, the case I discovered didn't have any problem. Sorry for the confusion and thanks!

aheejin · 2020-09-09T07:46:57Z

src/wasm/literal.cpp

+    case Type::anyref:
+      assert(literal.isNull() && "TODO: non-null anyref values");
+      o << "anyref(null)";
+      break;
    case Type::externref:
      assert(literal.isNull() && "TODO: non-null externref values");
      o << "externref(null)";


In which case do we need non-null anyref and externref literals?

This came up earlier in that these are currently not supported by Literal, which only can represent nulls of any reference type, but Thomas mentioned that we'd want to support these eventually, analogous to ExceptionPackage for exnref, so the interpreter can run code by representing non-null anyref etc. at least abstractly.

test/passes/flatten_local-cse_all-features.wast

aheejin · 2020-09-09T09:36:23Z

src/tools/fuzzing.h

+        if (wasm.features.hasExceptionHandling()) {
+          options.push_back(Type::exnref);
+        }


I don't think the fuzzer will generate types that does not respect dependences. For example, we check for both FeatureSet::ExceptionHandling and FeatureSet::ReferenceTypes whenever we try to add exnref or other EH instructions (By the way I just realized my PR for generating EH instructions for the fuzzer hasn't landed and is pending for months, but anyway). So I guess the fuzzer error you've seen was just complaining about the feature combination itself, right?

So I'm wondering, why do we need to impose the dependences in validator? Unless giving --enable-exception-handling without --enable-reference-types itself in the command line of a VM is an error, I don't think we should treat it as an error.

I think it is a subtle difference; using exnref without enabling --enable-reference-types might be an error. But Wasm spec doesn't say giving --enable-exception-handling without also giving --enable-reference-types is an error, so..

Co-authored-by: Heejin Ahn <[email protected]>

scripts/fuzz_opt.py

Co-authored-by: Thomas Lively <[email protected]>

aheejin

LGTM, and thanks!

dcodeIO · 2020-09-10T00:58:11Z

Please let me know whether the recent series of commits sufficiently addresses your concerns or if I misunderstood something :)

aheejin · 2020-09-10T11:05:26Z

CHANGELOG.md

@@ -16,6 +16,9 @@ Current Trunk
 -------------
 - Remove asm2wasm, which supported Emscripten's fastcomp backend, after fastcomp
  was removed.
+- Enabling the exception handling feature now requires enabling the reference
+  types feature as well since `exnref` depends on it. The same applies to the
+  new anyref feature where `anyref` depends on reference types.


This has been the case before (that you have to specify dependent features too), so nothing has changed here. If we later add feature implying feature then we need to write something here.

But it'd be worth noting that --enable-anyref is added, and what it does.

Added this because it is a validation error now (as of the current state of this PR), but wasn't before. Previously one would only get a validation error further down the road if --enable-exception-handling was specified, an exnref emitted somehow but --enable-reference-types omitted.

Split the entry into two parts, with one mentioning the new feature and one mentioning that not enabling implied features is a validation error now.

Oh sorry, I misinterpreted the comment. I actually forgot that this PR makes it a validation error if dependent flags aren't specified. I was not in favor of that, but maybe we should do feature implication later.

src/wasm/wasm-type.cpp

aheejin · 2020-09-10T11:12:08Z

src/tools/fuzzing.h

-          options.push_back(Type::externref);
-        }
+        options.push_back(Type::funcref);
+        options.push_back(Type::externref);


I'm not too opinionated about this, but the fuzzer still does not assume feature dependencies have been completely resolved; it checks that here and there. How about restoring this condition and maybe we can remove it when we add feature implication feature.

Gone ahead and restored the condition. Hope it's correct now :)

dcodeIO · 2020-09-11T00:18:39Z

Feel free to merge if it looks ok to you now. Not quite confident that your concerns are addressed. Meanwhile looking for something useful to do, now eyeballing the name section, i.e. module and local names.

Add anyref feature and type

ac55818

dcodeIO commented Sep 9, 2020

View reviewed changes

remove a left-over comment

d888eae

aheejin reviewed Sep 9, 2020

View reviewed changes

dcodeIO added 4 commits September 9, 2020 10:39

add implied feature validation, update unit tests accordingly

add5e7d

lint

67a139b

fix js event test

1b65a5e

teach the fuzzer about implied feature options

fb1a789

aheejin reviewed Sep 9, 2020

View reviewed changes

Update test/passes/flatten_local-cse_all-features.wast

1003b31

Co-authored-by: Heejin Ahn <[email protected]>

tlively reviewed Sep 9, 2020

View reviewed changes

scripts/fuzz_opt.py Outdated Show resolved Hide resolved

Update scripts/fuzz_opt.py

2809e61

Co-authored-by: Thomas Lively <[email protected]>

aheejin approved these changes Sep 9, 2020

View reviewed changes

dcodeIO added 3 commits September 10, 2020 02:31

move hasY checks into hasX checks where Y requires X

bc2d522

concretize comment on getLeastUpperBound returning anyref

4efa482

add CHANGELOG entry

c2033c1

aheejin reviewed Sep 10, 2020

View reviewed changes

dcodeIO added 3 commits September 10, 2020 20:11

update getLeastUpperBound comment

6c76c1f

update CHANGELOG

f877ef9

restore condition in TranslateToFuzzReader::getSubType

74be0b6

tlively approved these changes Sep 11, 2020

View reviewed changes

tlively merged commit 1927577 into WebAssembly:master Sep 11, 2020

MaxGraey mentioned this pull request Sep 11, 2020

Issue found by fuzzer #3119

Closed

	if (curr->features.hasReferenceTypes()) {
	addImport(curr,
	get_funcref,
	{Type::i32, Type::i32, Type::funcref},
	Type::funcref);
	addImport(curr,
	set_funcref,
	{Type::i32, Type::i32, Type::funcref},
	Type::funcref);
	addImport(curr,
	get_externref,
	{Type::i32, Type::i32, Type::externref},
	Type::externref);
	addImport(curr,
	set_externref,
	{Type::i32, Type::i32, Type::externref},
	Type::externref);
	}
	if (curr->features.hasExceptionHandling()) {
	addImport(
	curr, get_exnref, {Type::i32, Type::i32, Type::exnref}, Type::exnref);
	addImport(
	curr, set_exnref, {Type::i32, Type::i32, Type::exnref}, Type::exnref);
	}

Add anyref feature and type #3109

Add anyref feature and type #3109

Conversation

dcodeIO commented Sep 9, 2020 • edited Loading

dcodeIO Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

dcodeIO commented Sep 9, 2020

tlively commented Sep 9, 2020

aheejin commented Sep 9, 2020

dcodeIO commented Sep 9, 2020

aheejin commented Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

dcodeIO commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

dcodeIO commented Sep 11, 2020

dcodeIO commented Sep 9, 2020 •

edited

Loading

dcodeIO Sep 9, 2020 •

edited

Loading

aheejin Sep 9, 2020 •

edited

Loading

aheejin Sep 9, 2020 •

edited

Loading

aheejin Sep 9, 2020 •

edited

Loading

aheejin commented Sep 9, 2020 •

edited

Loading

aheejin Sep 9, 2020 •

edited

Loading