New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] better Stack validation and new package #229

Merged
merged 75 commits into from May 14, 2018

Conversation

Projects
None yet
3 participants
@maurobringolf
Copy link
Collaborator

maurobringolf commented Apr 3, 2018

I've had some time to look into validation of ast's, more specifically into type checking. This is a prototype that type checks a subset of all instructions and opts out of anything that is not yet supported, so it should never throw errors because something is not yet supported.

The idea is to run every function instruction per instruction and check if the types work out. In addition to that there is a moduleContext that maps ids to functions, globals, memories etc. It also contains information about the current stack frame like labels and locals. I couldn't find a reason to split the two, although they are conceptually very different (function scoped and module scoped).

There is a lot of work left to do, but the following testcases illustrate what type errors are already being caught correctly:

https://github.com/xtuc/webassemblyjs/tree/stack-validation-prototype/packages/webassemblyjs/test/fixtures/type-errors

@ColinEberhardt I already discussed a bit over Slack with Sven (should probably have used the public channel, sorry!) but if you have any inputs/suggestions/concerns I am happy to take them 馃槈

(The code contains some weird checks and corner cases due to inconsistencies in the AST format, I'll make issues about them).

TODO

  • end pseudo-instruction

  • Tables (nope, after merge --> Issue)

  • Decide what other validation passes to combine/integrate

  • Remove function parameters from initial stack

  • Assert that function results in only the expected result and not just the expected result on top of other things

  • Add test cases that run it over wasm

  • unreachable

  • Traps? Not sure how they relate to typing

  • Memory loads and stores plus its context

  • Complete imports ( currently just globals are implemented)

  • Find a better test format (that doesnt use 150 files)

  • memory.grow and memory.size

  • trap

  • Split into separate package

  • Add tests for invalid label values

  • Support branches to all allowed constructs, currently only block and loop are supported. Func, if etc also need to be.

  • Labels need to be removed from the module context when their blocks are left.

@@ -0,0 +1,6 @@
it("should throw an error during instantiation when type checking fails", () => {
assert.throws(

This comment has been minimized.

@xtuc

xtuc Apr 3, 2018

Owner

Maybe we can take advange of the throws.txt we have for tokensizer. It has less noise that a entire test.

This comment has been minimized.

@maurobringolf

maurobringolf Apr 3, 2018

Author Collaborator

Ah yep I didnt consider this. Would have to modifiy the index.js accordingly

This comment has been minimized.

@xtuc

xtuc Apr 4, 2018

Owner

Well it's not easy actually, this packages uses the exec.js to instantiate the module. Let's leave it as is for now.

// Filter based on last command line argument
if (argv.only) {
testSuites = testSuites.filter(f => f.includes(argv.only));
}

This comment has been minimized.

@xtuc

xtuc Apr 3, 2018

Owner

I added a way to select tests, see You can select test based on their titles. Would that fix this?

This comment has been minimized.

@maurobringolf

maurobringolf Apr 3, 2018

Author Collaborator

Ahh maybe I should have listened to GitHub when it told me "CONTRIBUTING.md has changed..." 馃榿 Thanks!

return stack;
}

// Workaround for node.args which sometimes does not contain instructions (i32.const, call)

This comment has been minimized.

@ColinEberhardt

ColinEberhardt Apr 4, 2018

Collaborator

I've spotted this too. The shape of the AST is something we can change - should these instructions be changed for consistency?

This comment has been minimized.

@maurobringolf

maurobringolf Apr 4, 2018

Author Collaborator

I made a list of possible ast modifications, Ill post them as separate issues today so we can decide individually

This comment has been minimized.

@xtuc

xtuc Apr 4, 2018

Owner

That would be great, thanks! I'm for breaking our AST, no users are relying on it.

/**
* binop
*
* @see https://webassembly.github.io/spec/core/valid/instructions.html#valid-binop

This comment has been minimized.

@ColinEberhardt

ColinEberhardt Apr 4, 2018

Collaborator

It would be really cool if the validation rules could be generated from the specification - I do wonder whether that approach would be less effort in the long run?

This comment has been minimized.

@maurobringolf

maurobringolf Apr 4, 2018

Author Collaborator

Interesting idea. Possibly for some that dont interact with the context (most them are already here though). The type rules that depend or modify the context (add label, change local etc) would be quite difficult I imagine

This comment has been minimized.

@xtuc

xtuc Apr 4, 2018

Owner

So we already have a signature file but it's slightly different because it's meant for parsing. I believe that this one can be generated from the instruction index on the doc.

This comment has been minimized.

@maurobringolf

maurobringolf Apr 4, 2018

Author Collaborator

Some rules have type variables though, so a generator would have to understand the premises and generate code to load types from the context accordingly (load type of a local for example).

We need a translation from the type rules into the moduleContext API which seems difficult to automate for me.

Also note that all modifications of the context dont happen here but in "applyInstruction", so this list is not even the full type rules in those cases.

@ColinEberhardt

This comment has been minimized.

Copy link
Collaborator

ColinEberhardt commented Apr 4, 2018

@ColinEberhardt I already discussed a bit over Slack with Sven (should probably have used the public channel, sorry!) but if you have any inputs/suggestions/concerns I am happy to take them

No worries - looks good to me, just a couple of minor comments.

(call $one)
)

(func (export "callByIndex") (result i32)
(export "callByName" (func $ffa))

This comment has been minimized.

@xtuc

xtuc Apr 4, 2018

Owner

Just noticed that the refmt also desugars, this is related to the discussion in #231 where we should have two code path.

case "extend_u/i32":
case "extend_s/i32":
case "convert_s/i32":
case "convert_u/i32":

This comment has been minimized.

@maurobringolf

maurobringolf Apr 5, 2018

Author Collaborator

@xtuc What do you think about storing the argument type (/i32 etc) as an attribute instead via the instruction name? It would make this code a lot simpler (could use regexes, but feels like something that should happen at the parse level).

@xtuc

This comment has been minimized.

Copy link
Owner

xtuc commented Apr 7, 2018

I think we should figure out something for the tests, It seems that you added 150 files just for the tests. I won't probably have the time to fix, could you please take a look at it? Thanks!

@maurobringolf

This comment has been minimized.

Copy link
Collaborator Author

maurobringolf commented Apr 7, 2018

I've added it to the todo list 馃憤 I'm pretty much done with the type rules themselves so I can focus on organizing the code and tests a bit better next.

@ColinEberhardt

This comment has been minimized.

Copy link
Collaborator

ColinEberhardt commented Apr 7, 2018

Regarding tests, I do like the one where you can modify the test to regenerated the expected JSON. For others, I do feel like it would be worth merging.

@maurobringolf maurobringolf force-pushed the stack-validation-prototype branch from c873d5f to 9dbaa38 Apr 8, 2018

});
});
});

This comment has been minimized.

@maurobringolf

maurobringolf Apr 8, 2018

Author Collaborator

@xtuc What do you think of this way of running tests? See last two files in the diff for an exampl

This comment has been minimized.

@ColinEberhardt

ColinEberhardt Apr 9, 2018

Collaborator

I think this is much better - although, could it be simplified even further ...

expect.throws( () =>
  parse(`(module
    (func (result f32) (f32.convert_s/i32 (i64.const 0)))
  )`),
  "Expected type i32 but got i64.");

This comment has been minimized.

@ColinEberhardt

ColinEberhardt Apr 9, 2018

Collaborator

I'm not sure there is any value in having the test fixture, test data and expected output in three different places.

This comment has been minimized.

@maurobringolf

maurobringolf Apr 9, 2018

Author Collaborator

Good suggestion, that makes sense for these small "explanatory" test cases 馃憤 I keep wondering if writing a parser for the spec tests assert_invalid construct is something we should do. I already started once but somehow regressed back to reformatting them by hand.

This comment has been minimized.

@maurobringolf

maurobringolf Apr 9, 2018

Author Collaborator

We couldn't use the reformatter with this strategy though 馃

This comment has been minimized.

@xtuc

xtuc Apr 9, 2018

Owner

That's a good point! The Try/catch + assert.equal logic already exists for this in the REPL.

Like:

(assert_invalid
  (module
    (func (result f32) (f32.convert_s/i32 (i64.const 0)))
  )
  "Expected type i32 but got i64"
)

Note that currently make test doesn't run the spec tests you will need to add this (maybe just your tests).

I recently added an API for the REPL: https://github.com/xtuc/webassemblyjs/blob/master/packages/repl/src/index.js#L15

We could integrate it in your mocha test suite.

I'm 馃挴 for it :)

This comment has been minimized.

@maurobringolf

maurobringolf Apr 9, 2018

Author Collaborator

I haven't looked into the repl in detail, but combined some more files into single test cases. The build still breaks due to some issue with the spec testsuite though. Locally my testsuite directory is empty is that correct? Even master doesnt pass make-ci locally for me, so I must be doing something wrong.

This comment has been minimized.

@xtuc

xtuc Apr 9, 2018

Owner

You need to update your gitsubmodules

This comment has been minimized.

@maurobringolf

maurobringolf Apr 9, 2018

Author Collaborator

Thanks, was able to fix the corresponding test now! There is so much work left to do, can we merge some MVP of this? I can move all tests to the new setup with throws.txt tomorrow, anything else you see as essential?

This comment has been minimized.

@xtuc

xtuc Apr 10, 2018

Owner

I'll review it shortly, I think we have more than a MVP because it's already better than my first implementation.

@xtuc xtuc changed the title [WIP] Stack validation prototype [WIP] better Stack validation Apr 11, 2018

@maurobringolf maurobringolf force-pushed the stack-validation-prototype branch from 6bd509b to 06d13de Apr 13, 2018

@xtuc xtuc added pkg: ast and removed AST labels Apr 17, 2018

@xtuc xtuc changed the title [WIP] better Stack validation [WIP] better Stack validation and new package Apr 17, 2018

@maurobringolf maurobringolf force-pushed the stack-validation-prototype branch 2 times, most recently from 5b0b5c3 to 373334d Apr 18, 2018

@xtuc xtuc referenced this pull request Apr 20, 2018

Merged

validation package #270

Mauro Bringolf and others added some commits Apr 21, 2018

Mauro Bringolf
@@ -0,0 +1,25 @@
(module

This comment has been minimized.

@xtuc

xtuc Apr 21, 2018

Owner

this test is disabled right?

This comment has been minimized.

@maurobringolf

maurobringolf Apr 21, 2018

Author Collaborator

Damn I forgot to delete this one, sorry! Good catch

xtuc and others added some commits Apr 21, 2018

@xtuc

This comment has been minimized.

xtuc and others added some commits May 10, 2018

Mauro Bringolf

@maurobringolf maurobringolf force-pushed the stack-validation-prototype branch from 30fd42e to 948d608 May 10, 2018

Mauro Bringolf and others added some commits May 10, 2018

Sven SAULEAU

getFunction(index) {
if (typeof index !== "number") {
throw new Error("getFunction only supported for number index");

This comment has been minimized.

@xtuc

xtuc May 13, 2018

Owner

When name are restored from the binary, getFunction is called with an Identifer and the lookup doesn't work. Maybe we should disable name resolution or keep both.

@xtuc

This comment has been minimized.

Copy link
Owner

xtuc commented May 13, 2018

@maurobringolf only errors left are unsupported else instruction, let's merge this after it

@xtuc

xtuc approved these changes May 14, 2018

Copy link
Owner

xtuc left a comment

馃帀 thanks for the work @maurobringolf

@xtuc xtuc merged commit bce4bcf into master May 14, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@xtuc xtuc deleted the stack-validation-prototype branch May 14, 2018

@maurobringolf

This comment has been minimized.

Copy link
Collaborator Author

maurobringolf commented May 14, 2018

Awesome thank you for helping with the annoying fixes!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment