From 772891f6270c20c34f3dc1d3984cffc6fa824d02 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Sat, 27 Jul 2019 22:41:50 -0700 Subject: Fix extra unreachable generation (#2266) Currently various expressions handle this differently, and now we consistently follow this rules: --- For all non-control-flow value-returning instructions, if a type of an expression is unreachable, we emit an unreachable and don't emit the instruction itself. If we don't emit an unreachable, instructions that follow can have validation failure in wasm binary format. For example: ``` [unreachable] (f32.add [unreachable] (i32.eqz [unreachable] (unreachable) ) ... ) ``` This is a valid prgram in binaryen IR, because the unreachable type propagates out of an expression, making both i32.eqz and f32.add unreachable. But in binary format, this becomes: ``` unreachable i32.eqz f32.add ;; validation failure; it expects f32 but takes an i32! ``` And here f32.add causes validation failure in wasm validation. So in this case we add an unreachable to prevent following instructions to consume the current value (here i32.eqz). In actual tests, I used `global.get` to an f32 global, which does not return a value, instead of `f32.add`, because `f32.add` itself will not be emitted if one of argument is unreachable. --- So the changes are: - For instructions that don't return a value, removes unreachable emitting code if it exists. - Add the unreachable emitting code for value-returning instructions if there isn't one. - Check for unreachability only once after emitting all children for atomic instructions. Currently only atomic instructions check unreachability after visiting each children and bail out right after, which is valid, but not consistent with others. - Don't emit an extra unreachable after a return (and return_call). I guess it is unnecessary. --- src/wasm-stack.h | 100 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 45 deletions(-) (limited to 'src/wasm-stack.h') diff --git a/src/wasm-stack.h b/src/wasm-stack.h index f4d116d77..64718b7b2 100644 --- a/src/wasm-stack.h +++ b/src/wasm-stack.h @@ -382,11 +382,33 @@ void BinaryenIRWriter::visitCall(Call* curr) { for (auto* operand : curr->operands) { visit(operand); } - emit(curr); - // TODO FIXME: this and similar can be removed - if (curr->type == unreachable) { + + // For non-control-flow value-returning instructions, if the type of an + // expression is unreachable, we emit an unreachable and don't emit the + // instruction itself. If we don't emit an unreachable, instructions that + // follow can have a validation failure in wasm binary format. For example: + // [unreachable] (f32.add + // [unreachable] (i32.eqz + // [unreachable] (unreachable) + // ) + // ... + // ) + // This is a valid prgram in binaryen IR, because the unreachable type + // propagates out of an expression, making both i32.eqz and f32.add + // unreachable. But in binary format, this becomes: + // unreachable + // i32.eqz + // f32.add ;; validation failure; it takes an i32! + // And here f32.add causes validation failure in wasm validation. So in this + // case we add an unreachable to prevent following instructions to consume + // the current value (here i32.eqz). + // + // The same applies for other expressions. + if (curr->type == unreachable && !curr->isReturn) { emitUnreachable(); + return; } + emit(curr); } template @@ -395,10 +417,11 @@ void BinaryenIRWriter::visitCallIndirect(CallIndirect* curr) { visit(operand); } visit(curr->target); - emit(curr); - if (curr->type == unreachable) { + if (curr->type == unreachable && !curr->isReturn) { emitUnreachable(); + return; } + emit(curr); } template @@ -409,10 +432,11 @@ void BinaryenIRWriter::visitLocalGet(LocalGet* curr) { template void BinaryenIRWriter::visitLocalSet(LocalSet* curr) { visit(curr->value); - emit(curr); - if (curr->type == unreachable) { + if (curr->isTee() && curr->type == unreachable) { emitUnreachable(); + return; } + emit(curr); } template @@ -430,7 +454,6 @@ template void BinaryenIRWriter::visitLoad(Load* curr) { visit(curr->ptr); if (curr->type == unreachable) { - // don't even emit it; we don't know the right type emitUnreachable(); return; } @@ -441,27 +464,14 @@ template void BinaryenIRWriter::visitStore(Store* curr) { visit(curr->ptr); visit(curr->value); - if (curr->type == unreachable) { - // don't even emit it; we don't know the right type - emitUnreachable(); - return; - } emit(curr); } template void BinaryenIRWriter::visitAtomicRMW(AtomicRMW* curr) { visit(curr->ptr); - // stop if the rest isn't reachable anyhow - if (curr->ptr->type == unreachable) { - return; - } visit(curr->value); - if (curr->value->type == unreachable) { - return; - } if (curr->type == unreachable) { - // don't even emit it; we don't know the right type emitUnreachable(); return; } @@ -471,20 +481,9 @@ void BinaryenIRWriter::visitAtomicRMW(AtomicRMW* curr) { template void BinaryenIRWriter::visitAtomicCmpxchg(AtomicCmpxchg* curr) { visit(curr->ptr); - // stop if the rest isn't reachable anyhow - if (curr->ptr->type == unreachable) { - return; - } visit(curr->expected); - if (curr->expected->type == unreachable) { - return; - } visit(curr->replacement); - if (curr->replacement->type == unreachable) { - return; - } if (curr->type == unreachable) { - // don't even emit it; we don't know the right type emitUnreachable(); return; } @@ -494,16 +493,10 @@ void BinaryenIRWriter::visitAtomicCmpxchg(AtomicCmpxchg* curr) { template void BinaryenIRWriter::visitAtomicWait(AtomicWait* curr) { visit(curr->ptr); - // stop if the rest isn't reachable anyhow - if (curr->ptr->type == unreachable) { - return; - } visit(curr->expected); - if (curr->expected->type == unreachable) { - return; - } visit(curr->timeout); - if (curr->timeout->type == unreachable) { + if (curr->type == unreachable) { + emitUnreachable(); return; } emit(curr); @@ -512,12 +505,9 @@ void BinaryenIRWriter::visitAtomicWait(AtomicWait* curr) { template void BinaryenIRWriter::visitAtomicNotify(AtomicNotify* curr) { visit(curr->ptr); - // stop if the rest isn't reachable anyhow - if (curr->ptr->type == unreachable) { - return; - } visit(curr->notifyCount); - if (curr->notifyCount->type == unreachable) { + if (curr->type == unreachable) { + emitUnreachable(); return; } emit(curr); @@ -526,6 +516,10 @@ void BinaryenIRWriter::visitAtomicNotify(AtomicNotify* curr) { template void BinaryenIRWriter::visitSIMDExtract(SIMDExtract* curr) { visit(curr->vec); + if (curr->type == unreachable) { + emitUnreachable(); + return; + } emit(curr); } @@ -533,6 +527,10 @@ template void BinaryenIRWriter::visitSIMDReplace(SIMDReplace* curr) { visit(curr->vec); visit(curr->value); + if (curr->type == unreachable) { + emitUnreachable(); + return; + } emit(curr); } @@ -540,6 +538,10 @@ template void BinaryenIRWriter::visitSIMDShuffle(SIMDShuffle* curr) { visit(curr->left); visit(curr->right); + if (curr->type == unreachable) { + emitUnreachable(); + return; + } emit(curr); } @@ -548,6 +550,10 @@ void BinaryenIRWriter::visitSIMDBitselect(SIMDBitselect* curr) { visit(curr->left); visit(curr->right); visit(curr->cond); + if (curr->type == unreachable) { + emitUnreachable(); + return; + } emit(curr); } @@ -555,6 +561,10 @@ template void BinaryenIRWriter::visitSIMDShift(SIMDShift* curr) { visit(curr->vec); visit(curr->shift); + if (curr->type == unreachable) { + emitUnreachable(); + return; + } emit(curr); } -- cgit v1.2.3