From 26f240c72dd62ed8a39f7466df99e51ec34487aa Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 28 Jul 2020 06:58:34 -0700 Subject: wasm2js: Don't remove an | 0 or >>> 0 in a boolean context (#2993) It is usually fine to do if (x | 0) => if (x) since it just cares if the value is 0 or not. However, if the cast turns it into 0, then that is incorrect, which the fuzzer found as -2147483648 + -2147483648 | 0 (the sum is 2^32, which | 0 is 0). We can maybe look into doing this in a safe way, but for now just remove it. It doesn't have a big impact on code size as this is pretty rare (e.g. the minimal runtime code size test is not broken by this). --- src/tools/wasm2js.cpp | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/tools/wasm2js.cpp b/src/tools/wasm2js.cpp index fb63627a3..d64f0b0ab 100644 --- a/src/tools/wasm2js.cpp +++ b/src/tools/wasm2js.cpp @@ -260,23 +260,33 @@ static void optimizeJS(Ref ast, Wasm2JSBuilder::Flags flags) { node[0]->setString(UNARY_PREFIX); node[1]->setString(L_NOT); node[3]->setNull(); - } else if (isOrZero(node) || isTrshiftZero(node)) { - // Just being different from 0 is enough, casts don't matter. However, - // in deterministic mode we care about corner cases that would trap in - // wasm, like an integer divide by zero: - // - // if ((1 / 0) | 0) => condition is Infinity | 0 = 0 which is falsey - // - // while - // - // if (1 / 0) => condition is Infinity which is truthy - // - // Thankfully this is not common, and does not occur on % (1 % 0 is a NaN - // which has the right truthiness). - if (!(flags.deterministic && isBinary(node[2], DIV))) { - return node[2]; - } } + // TODO: in some cases it may be possible to turn + // + // if (x | 0) + // + // into + // + // if (x) + // + // In general this is unsafe if e.g. x is -2147483648 + -2147483648 (which + // the | 0 turns into 0, but without it is a truthy value). + // + // Another issue is that in deterministic mode we care about corner cases + // that would trap in wasm, like an integer divide by zero: + // + // if ((1 / 0) | 0) => condition is Infinity | 0 = 0 which is falsey + // + // while + // + // if (1 / 0) => condition is Infinity which is truthy + // + // Thankfully this is not common, and does not occur on % (1 % 0 is a NaN + // which has the right truthiness), so we could perhaps do + // + // if (!(flags.deterministic && isBinary(node[2], DIV))) return node[2]; + // + // (but there is still the first issue). return node; }; -- cgit v1.2.3