Skip to content

Fix v8 validation error in -O0 builds #897

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix v8 validation error in -O0 builds #897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f16972
the top block in a function is special: if the function returns, it m…
kripken Feb 4, 2017
0aab29e
fix tests
kripken Feb 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions auto_update_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
cmd += ['-O0'] # test that -O0 does nothing
else:
cmd += ['-O']
if 'PRINT_FULL' in asm:
os.environ['BINARYEN_PRINT_FULL'] = '1'
else:
os.environ['BINARYEN_PRINT_FULL'] = '0'
if precise and opts:
# test mem init importing
open('a.mem', 'wb').write(asm)
Expand All @@ -34,6 +38,8 @@
actual = run_command(cmd)
with open(os.path.join('test', wasm), 'w') as o: o.write(actual)

os.environ['BINARYEN_PRINT_FULL'] = '0'

for dot_s_dir in ['dot_s', 'llvm_autogenerated']:
for s in sorted(os.listdir(os.path.join('test', dot_s_dir))):
if not s.endswith('.s'): continue
Expand Down
9 changes: 9 additions & 0 deletions check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@
cmd += ['-O0'] # test that -O0 does nothing
else:
cmd += ['-O']
if 'PRINT_FULL' in asm:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is adding a class of tests that can be ran with BINARYEN_PRINT_FULL set, then the test that this PR adds should have a name besides just PRINT_FULL.asm, something like PRINT_FULL.unreachable_function_block_type.asm.js I'd think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I considered that, but instead I documented this in more detail in the test itself. i.e., we might have just one PRINT_FULL file with multiple small unit tests inside it eventually, with some text describing each one in the file. I prefer that because a filename isn't much room to go into detail. if i'm in the minority though i can change that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case why not have the test script only set this if asm == 'PRINT_FULL.asm.js'? That way the intent of multiple small tests in this one special file is documented by the code itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I wanted to keep my options open ;) In case we do want multiple files, this would mean we don't need to change anything.

Anyhow, I don't feel strongly about any of it. If you do, let me know what you'd prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't feel strongly enough about it to block

Actually a best-of-both-worlds thing would be to leave the PRINT_FULL substring check, and rename the test to PRINT_FULL_generic.asm.js or something. That way both are reasonably-well supported, and it's more clear that generic tests go there.

os.environ['BINARYEN_PRINT_FULL'] = '1'
else:
os.environ['BINARYEN_PRINT_FULL'] = '0'
if precise and opts:
# test mem init importing
open('a.mem', 'wb').write(asm)
Expand All @@ -126,6 +130,9 @@
if actual != expected:
fail(actual, expected)

if 'PRINT_FULL' in asm:
continue # that's it, we can't do binary checks on this special output format

binary_format_check(wasm, verify_final_result=False)

# verify in wasm
Expand All @@ -151,6 +158,8 @@
fail_with_error('wasm interpreter error: ' + err) # failed to pretty-print
fail_with_error('wasm interpreter error')

os.environ['BINARYEN_PRINT_FULL'] = '0'

print '\n[ checking asm2wasm binary reading/writing... ]\n'

asmjs = os.path.join(options.binaryen_test, 'hello_world.asm.js')
Expand Down
9 changes: 9 additions & 0 deletions src/asm2wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,6 +2276,15 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
// cleanups/checks
assert(breakStack.size() == 0 && continueStack.size() == 0);
assert(parentLabel.isNull());
// a wasm function with a return value must have a body of that type,
// it is not ok to have an unreachable
if (function->result != none && function->body->type == unreachable) {
// if it's already a block, can just set the type; otherwise, add a block
if (!function->body->is<Block>()) {
function->body = builder.makeBlock(function->body);
}
function->body->type = function->result;
}
return function;
}

Expand Down
14 changes: 12 additions & 2 deletions src/ast_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,15 @@ struct ExpressionAnalyzer {
struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, Visitor<ReFinalize>>> {
ReFinalize() { name = "refinalize"; }

void visitBlock(Block *curr) { curr->finalize(); }
void visitBlock(Block *curr) {
// a wasm function with a return value must have a body of that type
if (getFunction()->result != none && curr == getFunction()->body) {
// do nothing, leave the proper type here
assert(curr->type == getFunction()->result);
return;
}
curr->finalize();
}
void visitIf(If *curr) { curr->finalize(); }
void visitLoop(Loop *curr) { curr->finalize(); }
void visitBreak(Break *curr) { curr->finalize(); }
Expand Down Expand Up @@ -351,7 +359,9 @@ struct AutoDrop : public WalkerPass<ExpressionStackWalker<AutoDrop, Visitor<Auto
void reFinalize() {
for (int i = int(expressionStack.size()) - 1; i >= 0; i--) {
auto* curr = expressionStack[i];
ReFinalize().visit(curr);
ReFinalize refinalizer;
refinalizer.setFunction(getFunction());
refinalizer.visit(curr);
}
}

Expand Down
32 changes: 32 additions & 0 deletions test/PRINT_FULL.asm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
Module["asm"] = (function(global, env, buffer) {
'almost asm';

var HEAP8 = new global.Int8Array(buffer);
var HEAP16 = new global.Int16Array(buffer);
var HEAP32 = new global.Int32Array(buffer);
var HEAPU8 = new global.Uint8Array(buffer);
var HEAPU16 = new global.Uint16Array(buffer);
var HEAPU32 = new global.Uint32Array(buffer);
var HEAPF32 = new global.Float32Array(buffer);
var HEAPF64 = new global.Float64Array(buffer);

var STACKTOP=env.STACKTOP|0;
var STACK_MAX=env.STACK_MAX|0;

var abortStackOverflow=env.abortStackOverflow;

// when printing the full ast, we should see a |block i32| that
// normal printing hides, but is crucial for validation
function stackAlloc(size) {
size = size|0;
var ret = 0;
ret = STACKTOP;
STACKTOP = (STACKTOP + size)|0;
STACKTOP = (STACKTOP + 15)&-16;
if ((STACKTOP|0) >= (STACK_MAX|0)) abortStackOverflow(size|0);
return ret|0;
}

return { stackAlloc: stackAlloc };
})
;
47 changes: 47 additions & 0 deletions test/PRINT_FULL.fromasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
(module
(type $FUNCSIG$vi (func (param i32)))
(import "env" "STACKTOP" (global $STACKTOP$asm2wasm$import i32))
(import "env" "STACK_MAX" (global $STACK_MAX$asm2wasm$import i32))
(import "env" "abortStackOverflow" (func $abortStackOverflow (param i32)))
(import "env" "memory" (memory $0 256 256))
(import "env" "table" (table 0 0 anyfunc))
(import "env" "memoryBase" (global $memoryBase i32))
(import "env" "tableBase" (global $tableBase i32))
(global $STACKTOP (mut i32) (get_global $STACKTOP$asm2wasm$import))
(global $STACK_MAX (mut i32) (get_global $STACK_MAX$asm2wasm$import))
(data (get_global $memoryBase) "PRINT_FULL.asm.js")
(export "stackAlloc" (func $stackAlloc))
(func $stackAlloc (param $0 i32) (result i32)
(local $1 i32)
[i32] [i32] (block i32
[none] (set_local $1
[i32] (get_global $STACKTOP)
)
[none] (set_global $STACKTOP
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (get_local $0)
)
)
[none] (set_global $STACKTOP
[i32] (i32.and
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (i32.const 15)
)
[i32] (i32.const -16)
)
)
[none] (if
[i32] (i32.ge_s
[i32] (get_global $STACKTOP)
[i32] (get_global $STACK_MAX)
)
[none] (call $abortStackOverflow
[i32] (get_local $0)
)
)
[i32] (get_local $1)
)
)
)
46 changes: 46 additions & 0 deletions test/PRINT_FULL.fromasm.imprecise
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
(module
(type $FUNCSIG$vi (func (param i32)))
(import "env" "STACKTOP" (global $STACKTOP$asm2wasm$import i32))
(import "env" "STACK_MAX" (global $STACK_MAX$asm2wasm$import i32))
(import "env" "abortStackOverflow" (func $abortStackOverflow (param i32)))
(import "env" "memory" (memory $0 256 256))
(import "env" "table" (table 0 0 anyfunc))
(import "env" "memoryBase" (global $memoryBase i32))
(import "env" "tableBase" (global $tableBase i32))
(global $STACKTOP (mut i32) (get_global $STACKTOP$asm2wasm$import))
(global $STACK_MAX (mut i32) (get_global $STACK_MAX$asm2wasm$import))
(export "stackAlloc" (func $stackAlloc))
(func $stackAlloc (param $0 i32) (result i32)
(local $1 i32)
[i32] [i32] (block i32
[none] (set_local $1
[i32] (get_global $STACKTOP)
)
[none] (set_global $STACKTOP
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (get_local $0)
)
)
[none] (set_global $STACKTOP
[i32] (i32.and
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (i32.const 15)
)
[i32] (i32.const -16)
)
)
[none] (if
[i32] (i32.ge_s
[i32] (get_global $STACKTOP)
[i32] (get_global $STACK_MAX)
)
[none] (call $abortStackOverflow
[i32] (get_local $0)
)
)
[i32] (get_local $1)
)
)
)
48 changes: 48 additions & 0 deletions test/PRINT_FULL.fromasm.imprecise.no-opts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
(module
(type $FUNCSIG$vi (func (param i32)))
(import "env" "STACKTOP" (global $STACKTOP$asm2wasm$import i32))
(import "env" "STACK_MAX" (global $STACK_MAX$asm2wasm$import i32))
(import "env" "abortStackOverflow" (func $abortStackOverflow (param i32)))
(import "env" "memory" (memory $0 256 256))
(import "env" "table" (table 0 0 anyfunc))
(import "env" "memoryBase" (global $memoryBase i32))
(import "env" "tableBase" (global $tableBase i32))
(global $STACKTOP (mut i32) (get_global $STACKTOP$asm2wasm$import))
(global $STACK_MAX (mut i32) (get_global $STACK_MAX$asm2wasm$import))
(export "stackAlloc" (func $stackAlloc))
(func $stackAlloc (param $size i32) (result i32)
(local $ret i32)
[i32] [i32] (block i32
[none] (set_local $ret
[i32] (get_global $STACKTOP)
)
[none] (set_global $STACKTOP
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (get_local $size)
)
)
[none] (set_global $STACKTOP
[i32] (i32.and
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (i32.const 15)
)
[i32] (i32.const -16)
)
)
[none] (if
[i32] (i32.ge_s
[i32] (get_global $STACKTOP)
[i32] (get_global $STACK_MAX)
)
[none] (call $abortStackOverflow
[i32] (get_local $size)
)
)
[unreachable] (return
[i32] (get_local $ret)
)
)
)
)
48 changes: 48 additions & 0 deletions test/PRINT_FULL.fromasm.no-opts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
(module
(type $FUNCSIG$vi (func (param i32)))
(import "env" "STACKTOP" (global $STACKTOP$asm2wasm$import i32))
(import "env" "STACK_MAX" (global $STACK_MAX$asm2wasm$import i32))
(import "env" "abortStackOverflow" (func $abortStackOverflow (param i32)))
(import "env" "memory" (memory $0 256 256))
(import "env" "table" (table 0 0 anyfunc))
(import "env" "memoryBase" (global $memoryBase i32))
(import "env" "tableBase" (global $tableBase i32))
(global $STACKTOP (mut i32) (get_global $STACKTOP$asm2wasm$import))
(global $STACK_MAX (mut i32) (get_global $STACK_MAX$asm2wasm$import))
(export "stackAlloc" (func $stackAlloc))
(func $stackAlloc (param $size i32) (result i32)
(local $ret i32)
[i32] [i32] (block i32
[none] (set_local $ret
[i32] (get_global $STACKTOP)
)
[none] (set_global $STACKTOP
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (get_local $size)
)
)
[none] (set_global $STACKTOP
[i32] (i32.and
[i32] (i32.add
[i32] (get_global $STACKTOP)
[i32] (i32.const 15)
)
[i32] (i32.const -16)
)
)
[none] (if
[i32] (i32.ge_s
[i32] (get_global $STACKTOP)
[i32] (get_global $STACK_MAX)
)
[none] (call $abortStackOverflow
[i32] (get_local $size)
)
)
[unreachable] (return
[i32] (get_local $ret)
)
)
)
)