[interpreter] Fix JS generator #34
Conversation
interpreter/script/js.ml
Outdated
| @@ -347,6 +346,8 @@ let wrap item_name wrap_action wrap_assertion at = | |||
| {module_name = Utf8.decode "spectest"; item_name = Utf8.decode "eq_ref"; | |||
| idesc = FuncImport (4l @@ at) @@ at} @@ at ] | |||
| in | |||
| let idesc_is_func = match idesc.it with FuncImport _ -> 1l | _ -> 0l in | |||
There was a problem hiding this comment.
I don't completely understand the logic here, but for me it causes an off-by-one error. Just using
let item = Lib.List32.length imports @@ at in in the next line fixes the problem for me.
There was a problem hiding this comment.
Oops, that went the wrong way round. Replaced with a correct and less brittle version.
| @@ -83,7 +83,7 @@ let harness = | |||
| "}\n" ^ | |||
| "\n" ^ | |||
| "function exports(instance) {\n" ^ | |||
| " return {module: instance.exports, host: {ref: hostref}};\n" ^ | |||
| " return {module: instance.exports, spectest: spectest};\n" ^ | |||
There was a problem hiding this comment.
The generated module expects an import spectest::eq_ref. Could you extend spectest above with an eq_ref field?
There was a problem hiding this comment.
Also, some tests expect that hostref(1) provide an exported wasm function, but it doesn't.
There was a problem hiding this comment.
Added missing eq_ref. I'm not sure I understand your other comment. Which test is this?
There was a problem hiding this comment.
For the test in ref_is_null.wast:33, hostref(1) is passed in as a valid value of funcref. However, afaict, it is not a valid value. When I change hostref(1) with $1.exports["funcref-elem"], which is a valid funcref value, the test passes.
There was a problem hiding this comment.
Ah, thanks! That test actually was bogus (but the interpreter does not type-check script commands like invoke -- it probably should). I removed it.
| @@ -83,7 +83,7 @@ let harness = | |||
| "}\n" ^ | |||
| "\n" ^ | |||
| "function exports(instance) {\n" ^ | |||
| " return {module: instance.exports, host: {ref: hostref}};\n" ^ | |||
| " return {module: instance.exports, spectest: spectest};\n" ^ | |||
There was a problem hiding this comment.
Added missing eq_ref. I'm not sure I understand your other comment. Which test is this?
interpreter/script/js.ml
Outdated
| @@ -347,6 +346,8 @@ let wrap item_name wrap_action wrap_assertion at = | |||
| {module_name = Utf8.decode "spectest"; item_name = Utf8.decode "eq_ref"; | |||
| idesc = FuncImport (4l @@ at) @@ at} @@ at ] | |||
| in | |||
| let idesc_is_func = match idesc.it with FuncImport _ -> 1l | _ -> 0l in | |||
There was a problem hiding this comment.
Oops, that went the wrong way round. Replaced with a correct and less brittle version.
This commit fixes #34 by specifying that the flags field (which indicates if a segment is passive) is a `varuint32` instead of a `uint8`. It was discovered in #34 that the memory index located at that position today is a `varuint32`, which can be validly encoded as `0x80 0x00` in addition to `0x00` (in addition to a number of other encodings). This means that if the first field were repurposed as a single byte of flags, it would break these existing modules that work today. It's not currently known how many modules in the wild actually take advantage of such an encoding, but it's probably better to be safe than sorry! Closes #34
Fix bugs reported by @gahaas.