Implement env.exit and env.abort by rossberg · Pull Request #244 · WebAssembly/spec

rossberg · 2016-02-17T16:41:59Z

Should make the waterfall significantly happier.

jfbastien · 2016-02-17T16:44:36Z

Awesome, thanks! Once this is in the tests won't actually execute because I'm not generating the .wast files with (start ...) yet. I'll wait for your change to make it in, and then I'll add start :-)

sunfishcode · 2016-02-17T16:52:36Z

"Env" sounds like a fairly non-temporary name. What are the future plans for this module?

Env.abort: Is the existing unreachable instruction insufficient?

Env.exit: I haven't thought about this deeply, but should we make this an instruction too? In a JS context, one can just call out to JS, have it throw an exception, and unwind all the wasm frames. However in a non-JS context, there isn't a way to say "unwind all the stack frames and exit" short of trapping, but that has the additional effect of indicating a failure of some kind.

jfbastien · 2016-02-17T16:57:38Z

FWIW when we get a nice wasm libc that we all agree on (and our tests link to it) these will change names and be __syscall_abort and __syscall_exit or something like that. It'll be a simple rename from what @rossberg-chromium adds here.

The libc won't be Standard since it'll all be user-space, but it'l be what we end up agreeing to use.

sunfishcode · 2016-02-17T17:10:46Z

If we know the names we want, let's just use those from the start. If we don't and want to defer making that decision, let's use sufficiently temporary-looking names so that we don't get confused.

Env.abort: Is the existing unreachable instruction insufficient?

Env.exit: I haven't thought about this deeply, but should we make this an instruction too? In a JS context, one can just call out to JS, have it throw an exception, and unwind all the wasm frames. However in a non-JS context, there isn't a way to say "unwind all the stack frames and exit" short of trapping, but that has the additional effect of indicating a failure of some kind.

rossberg · 2016-02-17T17:12:31Z

@sunfishcode, the point of this change isn't expressiveness, but to be able to run various tests from the waterfall, many of which try to import one of these functions. I don't care about the module name, although I actually agree that env is overly generic. @jfbastien, can't we call it libc or libcbase or something?

sunfishcode · 2016-02-17T17:25:08Z

@rossberg-chromium So this is a hack to underpin another hack. There may be a motivation for this, but I will at least request that hacks be clearly labeled for what they are, because it's very confusing to have so many hacks mixed in with the real design when we're trying to work on the real design at the same time.

libc or most any name including libc would be similarly confusing for me. waterfall_scaffolding would be less problematic.

lukewagner · 2016-02-17T17:46:45Z

We already have spectest for our grab bag of stuff that's good for testing/automation; can we just put them there?

rossberg · 2016-02-17T18:24:58Z

@sunfishcode, @lukewagner, "builtins" mock a useful execution environment, they are not part of the spec. Naming needs to match the other relevant testing/execution environments, otherwise it's useless. The spectest module does not exist anywhere else (and rightfully so, I would argue).

Eventually we should find policies for allocating names in the global namespace of modules, but is that a relevant concern right now? Should development block on it? We can easily adjust tests later.

lukewagner · 2016-02-17T18:27:38Z

Agreed on it not being part of the spec... but how is that different than spectest? That is, why do we need two grabbags?

rossberg · 2016-02-17T18:34:56Z

It's not a grab bag, it's mocking (a subset of) a concrete module that the libc tests on the waterfall import. It does not have any relation to spectest.

lukewagner · 2016-02-17T18:38:40Z

Ah, I see the distinction now. Seems fine then.

kripken · 2016-02-17T18:54:54Z

Putting it on "env" would collide with existing code, that sends exit or abort there, as e.g. asm2wasm does.

I think waterfall_scaffolding makes sense, or waterfalltest (to parallel spectest). If this is for the waterfall specifically, then that seems like it should be in the name.

jfbastien · 2016-02-17T18:55:44Z

"env" comes from binaryen:
https://github.com/WebAssembly/binaryen/blob/master/src/s2wasm.h#L757

I don't really care what it is, but I think we're putting too much thought into something pretty simple.

This isn't just for the waterfall, I want to run code in spec as well as d8 and other interpreters, and I've put off getting agreement on libc at other's prompting.

kripken · 2016-02-17T19:00:04Z

The env there is for real imports arriving from the outside, though. And exit and abort will in fact be provided in a lot of existing code. Whereas this adds env.exit and abort to the actual spec, if I understand correctly? In other words, this moves a userspace issue into something to be standardized? That doesn't feel right.

jfbastien · 2016-02-17T19:11:36Z

I'm fine with changing the name to whatever is decided upon, and adding support for it in binaryen. Just pick a name.

rossberg · 2016-02-18T07:10:34Z

@kripken, "builtins" are not part of the spec. Maybe the name is misleading. They are part of the host directory of ml-proto, used to mock an embedding environment, i.e., real imports from the outside as you say. In ml-proto, it's the only way to provide external modules.

rossberg · 2016-02-18T15:38:33Z

Okay, I refactored the handling of imports to make it (a) more modular and configurable, (b) clearer that the actual modules provided by the host are custom. Renamed builtins to imports, and there now is a new directory host/import where host-provided modules live. They can be registered in main.

kripken · 2016-02-18T18:31:40Z

Perhaps it's worth documenting the distinction that the stuff in host/ is not part of the spec? As this is the spec repo, my assumption was that everything here is part of the spec.

rossberg · 2016-02-19T10:55:34Z

@kripken, done.

PTAL

kripken · 2016-02-19T18:51:43Z

lgtm, thanks.

jfbastien · 2016-02-19T19:26:29Z

lgtm as well.

sunfishcode · 2016-02-19T19:30:00Z

ml-proto/host/import/env.ml

@@ -0,0 +1,39 @@
+open Values
+open Types
+


The name env is confusing to me. The name waterfalltest was suggested in this PR; I'd be ok with that.

Also, please add a top-level comment to this file explaining what its purpose is. The text in the README.md is a good introduction, but we should be clear about what these various builtin modules are for.

This isn't just for the waterfall.

What is it for?

Running compiled code as a stopgap until we have agreement on what libc should look like. When we have an agreed libc we'll likely do something like syscalls, and abort/exit will be simple renames. libc still won't be "standard", but it'll be an ad-hoc thing we end up using.

I'll be testing this on the waterfall, but something that only works on the waterfall is a silly idea so I want to avoid silly.

Please put this in a comment in the file then, so that we can all know what it is we're dealing with here.

Also, please rename the module to stopgap or something similarly descriptive.

Added comments to spectest and env modules explaining their purpose. PTAL

@sunfishcode, as explained before, the env module emulates the respective module imported by Binaryen code. Renaming it here would defeat the purpose. Binaryen would have to decide that.

kripken · 2016-02-22T20:00:43Z

Thinking about this some more, I'm not sure of the scope plans here. This adds exit and abort, what if next we see some tests that would run if we also added memcpy, printf, sbrk, and so forth?

If there's a clear line that explains why exit and abort but not the rest of libc, then I think it's worth mentioning in the README.

jfbastien · 2016-02-22T20:28:42Z

@kripken same reasoning as for:
https://github.com/WebAssembly/binaryen/blob/ed567a4f49d3296dcf91bdb966c0deb077dacff7/src/binaryen-shell.cpp#L130
The torture tests are self-checking, and call abort / exit on success / failure. Until we have a from-the-ground-up wasm libc which isn't contended we need a simple way to run tests (more than the torture tests IMO) and this is a basic first step.

We're waayyyyy over thinking things right now.

kripken · 2016-02-22T20:39:46Z

I agree let's not overthink this. Just seems worth documenting "this is stuff specifically for the torture tests, and not intended to include any significant parts of libc", or something along those lines. I added some docs to binaryen now for that one hack we have.

rossberg · 2016-02-23T21:34:37Z

Okay, is anybody still opposed to landing this?

sunfishcode · 2016-02-23T21:38:54Z

"A subset of the env module as used by Binaryen." isn't a clear explanation of what this module is for. Would it be possible to put JF's description, "Running compiled code as a stopgap until we have agreement on what libc should look like." in the comment in the file?

Also, binaryen is changeable. We can rename things. "env" is a very generic name. I think we should rename this module, and teach Binaryen about the new name, to help avoid ending up making something which outlives its intended use, such after all this is, as JF says, a stopgap.

rossberg · 2016-02-23T22:21:53Z

Tweaked comment to express the purpose. Landing, but happy to keep this in sync with any name change Binaryen chooses to implement.

Implement env.exit and env.abort

sunfishcode · 2016-02-23T22:23:39Z

Why are you so unwilling to put what you're actually doing here in a comment in the file?

This just adds a comment, using wording that JF used to explain what this file is for, in [this comment](#244 (comment)).

Clarify that the natural alignment is always the size of the loaded data. Resolves WebAssembly#230 and resolves and WebAssembly#162.

… everywhere. (WebAssembly#241) This addresses additional review comments to PR WebAssembly#222, that were made after it was merged. The last review comment in the discussion suggests to adjust all validation labels to use label types instead of just result types. Should address all occurrences of validation labels. Additionally adds a boolean catch_label to control frames in the validation algorithm, and some related functionality, fixing the cases for opcodes `catch` and `catch_all`. * Apply suggestions from code review Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org> Co-authored-by: Heejin Ahn <aheejin@gmail.com> * Reverting changes to typing of CAUGHTadm. Changes to this rule are now done in PR WebAssembly#244

* Fixes the typing rule of CAUGHTadm as suggested in: WebAssembly/exception-handling#229 (comment) * Applying suggestion from PR WebAssembly#241 https://github.com/WebAssembly/exception-handling/pull/241/files/201ff276be6c458e7a819662eb2862bd475f2fcd..f97c9d108a999c3c3fd30f25554a113822bed1b8#r1030097793 * Applied suggestions from code review, fixed notation, also adding the missing _n to CAUGHTadm. The subscript is introduced by the changes in PR WebAssembly#226

Implement env.exit and env.abort

12f041c

Make import environment more customisable

c829d93

Clarify dirs in readme

a8115a8

sunfishcode reviewed Feb 19, 2016
View reviewed changes

Comments describing import modules

ce729a7

Tweak comment

41fc968

rossberg added a commit that referenced this pull request Feb 23, 2016

Merge pull request #244 from WebAssembly/lib

182e3c9

Implement env.exit and env.abort

rossberg merged commit 182e3c9 into master Feb 23, 2016

rossberg deleted the lib branch February 23, 2016 22:22

sunfishcode added a commit that referenced this pull request Feb 24, 2016

Capture JF's description of env.ml in a comment.

69364e1

This just adds a comment, using wording that JF used to explain what this file is for, in [this comment](#244 (comment)).

sunfishcode mentioned this pull request Feb 24, 2016

Capture JF's description of env.ml in a comment. #250

Merged

Conversation

rossberg commented Feb 17, 2016

Uh oh!

jfbastien commented Feb 17, 2016

Uh oh!

sunfishcode commented Feb 17, 2016

Uh oh!

jfbastien commented Feb 17, 2016

Uh oh!

sunfishcode commented Feb 17, 2016

Uh oh!

rossberg commented Feb 17, 2016

Uh oh!

sunfishcode commented Feb 17, 2016

Uh oh!

lukewagner commented Feb 17, 2016

Uh oh!

rossberg commented Feb 17, 2016

Uh oh!

lukewagner commented Feb 17, 2016

Uh oh!

rossberg commented Feb 17, 2016

Uh oh!

lukewagner commented Feb 17, 2016

Uh oh!

kripken commented Feb 17, 2016

Uh oh!

jfbastien commented Feb 17, 2016

Uh oh!

kripken commented Feb 17, 2016

Uh oh!

jfbastien commented Feb 17, 2016

Uh oh!

rossberg commented Feb 18, 2016

Uh oh!

rossberg commented Feb 18, 2016

Uh oh!

kripken commented Feb 18, 2016

Uh oh!

rossberg commented Feb 19, 2016

Uh oh!

kripken commented Feb 19, 2016

Uh oh!

jfbastien commented Feb 19, 2016

Uh oh!

sunfishcode Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

sunfishcode Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

jfbastien Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

sunfishcode Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

jfbastien Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

sunfishcode Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

rossberg Feb 22, 2016

Choose a reason for hiding this comment

Uh oh!

kripken commented Feb 22, 2016

Uh oh!

jfbastien commented Feb 22, 2016

Uh oh!

kripken commented Feb 22, 2016

Uh oh!

rossberg commented Feb 23, 2016

Uh oh!

sunfishcode commented Feb 23, 2016

Uh oh!

rossberg commented Feb 23, 2016

Uh oh!

sunfishcode commented Feb 23, 2016

Uh oh!