brson / miri

An experimental compiler from Rust to WebAssembly (inactive - do not use)
Apache License 2.0
209 stars 15 forks source link

V8 can't run tests due to binary version mismatch #49

Open eholk opened 8 years ago

eholk commented 8 years ago

We recently updated binaryen to a version that generates Wasm 0xC binaries. The version of V8 downloaded from http://wasm-stat.us unfortunately is still 0xB. We need to update V8.

I tried updating to a more recent snapshot, but that doesn't appear to be updated yet either.

It might be better to fetch and build V8 on our own. This would let us make sure we're running on a version we support, and also let us run V8 on other platforms as well.

Alternatively, maybe we should add an environment variable or something that points to a suitable d8, and leave finding an appropriate one up to person running the tests.

eholk commented 8 years ago

Oh, it looks like the build paths changed sometime. Once I got those fixed, I got a d8 that supports 0xC.

Now I'm getting validation errors, which are similar to the errors in #47.

lqd commented 8 years ago

What kind of validation errors, could you post them somewhere maybe ? This is where having CI running the v8 tests would be very valuable on this project!

eholk commented 8 years ago

Here's the output from one of the tests that's failing: https://gist.github.com/eholk/f2748e1b0c98b4e39a56356b25e9aaea

That was from my work-in-progress V8 update branch. I've pushed the code to https://github.com/eholk/mir2wasm/tree/0xc if you want to see what's changed.

The issue seems to be that we don't have the right number of values on the stack at certain times. This in turn seems to be related to our handling of the existence or not of return values from functions.

When I read over the .wast code in question, everything looks okay to me. I wonder if binaryen or wabt are missing some of the control flow? If so, we might be able to work around this with clever placements of unreachable opcodes.

eholk commented 8 years ago

Also, CI is a great idea! I thought it wasn't really possible on this project for some reason, but it occurs to me I had no reason to think that and that this should be straightforward to do with Travis CI.

@brson - Do you think you could set us up with Travis (or something similar)? Alternatively, if you give me admin access on this repo then I could do it too.

lqd commented 8 years ago

hum. $main:

(local $0 i32)
...
(local $12 i32)
(local $13 i32)

errors:

[wasm-validator error in function $main] 1 != 0: set_local type must match function, on 
[none] (set_local $12
  [none] (get_local $0)
)
[wasm-validator error in function $main] 1 != 0: set_local type must match function, on 
[none] (set_local $13
  [none] (get_local $12)
)

they're all i32s, does this type error look weird to you as well ?

eholk commented 8 years ago

I must be misinterpreting the error message, because my interpretation is blatantly contradicted by reality.

To me this means the validator is expecting $main to return a single value, but that the set_local does not have a return value, so it doesn't match the expectation.

There are a couple of problems. First, the type for $main does not have a return value. Second, it seems like for set_local to be the return value it should be in tail position, which it is not.

@kripken - Am I misunderstanding the error message?

lqd commented 8 years ago

@eholk I've tried running the wast in the gist with the wasm-shell from master and there are no validation errors. Are those errors in the gist from v8 or binaryen ?

kripken commented 8 years ago

About

 [wasm-validator error in function $main] 1 != 0: set_local type must match function, on 
[none] (set_local $12
  [none] (get_local $0)
)

The first question is why the get_local has type none. BinaryenGetLocal receives the type as the last parameter - perhaps we are giving it none or something else invalid?

lqd commented 8 years ago

@kripken ooooh, so that's why in-memory validation can be different from "text" validation ? (I was wondering what those new [type] were as well)

I guess most/all node creation related to locals, with a none type, are invalid in-memory but valid textually.

Another thing to add on our list to fix :)

kripken commented 8 years ago

It could be invalid in memory but valid textually, if we don't validate those, which is indeed a bug in binaryen, I now see. I'll add validation.