Skip to content

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Oct 4, 2016

Also skip binary roundtrip on stacky spec tests

@kripken
Copy link
Member

kripken commented Oct 4, 2016

Wait, why did you add skipping here? Those aren't "stacky" tests, there should only be stacky stuff in stack.wast (which we ignore entirely)?

@dschuff
Copy link
Member Author

dschuff commented Oct 4, 2016

func.wast has

;; TODO(stack): move these somewhere else
(module (func $type-return-void-vs-enpty (return (nop))))
(module (func $type-return-num-vs-enpty (return (i32.const 0))))

and return.wast has

;; TODO(stack): move these somewhere else
(module (func $type-value-void-vs-empty (return (nop))))
(module (func $type-value-num-vs-empty (return (i32.const 0))))

Are my tests out of date?

@kripken
Copy link
Member

kripken commented Oct 4, 2016

The tests look up to date, and yes they have the "stack" comment on them, but they were not "too" stacky before - I guess that now they are, after removing return arity?

I'm ok to skip these then, for now, but the proper thing is to move them to stack.wast in WebAssembly/spec#351, and to update our spec tests to that version, so we don't need to skip. But we can do that later (might be more such things?).

@dschuff
Copy link
Member Author

dschuff commented Oct 4, 2016

Yeah the problem now is that e.g.
(module (func $type-return-num-vs-enpty (return (i32.const 0))))
is, in stack machine language:

(func
i32.const 0
return
)

Previously the return was encoded by binaryen with arity 1, and parsed with arity 1 which consumed the const. Now the return is inferred as arity 0 (because the function is void), and the const is left on the stack after parsing.

@dschuff dschuff merged commit c5b8f37 into master Oct 4, 2016
@ghost
Copy link

ghost commented Oct 4, 2016

@dschuff That function would be invalid AST because the number of arguments to the return does not match the expected number. The stack machine code is valid, just not yet expressible in binaryen (one of the stacky) examples. The AST still needs to know the number of arguments to check that they match.

dschuff added a commit that referenced this pull request Oct 5, 2016
Compatible with #740
@dschuff dschuff mentioned this pull request Oct 5, 2016
dschuff added a commit that referenced this pull request Oct 5, 2016
Compatible with #740
@dschuff dschuff deleted the return_arity branch October 6, 2016 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants