From 1d10e037ef9e69424f4fce998b4857648a96e740 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Tue, 23 Oct 2018 23:26:13 +0000 Subject: [PATCH] [vm/bytecode] Remove unreachable bytecode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I29393604547038cf359b8e19ea48078dd0eea3b1 Reviewed-on: https://dart-review.googlesource.com/c/81201 Reviewed-by: Zach Anderson Reviewed-by: Régis Crelier Commit-Queue: Alexander Markov Auto-Submit: Alexander Markov --- pkg/vm/lib/bytecode/assembler.dart | 44 +++- pkg/vm/lib/bytecode/gen_bytecode.dart | 190 ++++++++++++----- pkg/vm/testcases/bytecode/async.dart.expect | 162 +++++--------- .../bytecode/bootstrapping.dart.expect | 16 -- .../testcases/bytecode/closures.dart.expect | 13 -- .../bytecode/deferred_lib.dart.expect | 4 - .../bytecode/instance_creation.dart.expect | 16 -- .../testcases/bytecode/literals.dart.expect | 8 - pkg/vm/testcases/bytecode/loops.dart.expect | 14 -- .../bytecode/super_calls.dart.expect | 18 -- pkg/vm/testcases/bytecode/switch.dart.expect | 6 - .../testcases/bytecode/try_blocks.dart.expect | 198 +++++------------- .../testcases/bytecode/type_ops.dart.expect | 8 - .../frontend/bytecode_flow_graph_builder.cc | 19 +- 14 files changed, 278 insertions(+), 438 deletions(-) diff --git a/pkg/vm/lib/bytecode/assembler.dart b/pkg/vm/lib/bytecode/assembler.dart index dce3410420dad..b45b4c1501155 100644 --- a/pkg/vm/lib/bytecode/assembler.dart +++ b/pkg/vm/lib/bytecode/assembler.dart @@ -10,15 +10,19 @@ import 'dbc.dart'; import 'exceptions.dart' show ExceptionsTable; class Label { + final bool allowsBackwardJumps; List _jumps = []; int offset = -1; - Label(); + Label({this.allowsBackwardJumps: false}); bool get isBound => offset >= 0; int jumpOperand(int jumpOffset) { if (isBound) { + if (offset <= jumpOffset && !allowsBackwardJumps) { + throw 'Backward jump to this label is not allowed'; + } // Jump instruction takes an offset in DBC words. return (offset - jumpOffset) >> BytecodeAssembler.kLog2BytesPerBytecode; } @@ -44,6 +48,7 @@ class BytecodeAssembler { final Uint32List _encodeBufferIn; final Uint8List _encodeBufferOut; final ExceptionsTable exceptionsTable = new ExceptionsTable(); + bool isUnreachable = false; BytecodeAssembler._(this._encodeBufferIn, this._encodeBufferOut); @@ -60,9 +65,15 @@ class BytecodeAssembler { for (int jumpOffset in jumps) { patchJump(jumpOffset, label.jumpOperand(jumpOffset)); } + if (jumps.isNotEmpty || label.allowsBackwardJumps) { + isUnreachable = false; + } } void emitWord(int word) { + if (isUnreachable) { + return; + } _encodeBufferIn[0] = word; // TODO(alexmarkov): Which endianness to use? bytecode.addAll(_encodeBufferOut); } @@ -145,8 +156,17 @@ class BytecodeAssembler { emitWord(_encode0(opcode)); } + void _emitJumpBytecode(Opcode opcode, Label label) { + assert(isJump(opcode)); + if (!isUnreachable) { + // Do not use label if not generating instruction. + emitWord(_encodeT(opcode, label.jumpOperand(offset))); + } + } + void emitTrap() { emitWord(_encode0(Opcode.kTrap)); + isUnreachable = true; } void emitDrop1() { @@ -154,40 +174,40 @@ class BytecodeAssembler { } void emitJump(Label label) { - emitWord(_encodeT(Opcode.kJump, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJump, label); + isUnreachable = true; } void emitJumpIfNoAsserts(Label label) { - emitWord(_encodeT(Opcode.kJumpIfNoAsserts, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfNoAsserts, label); } void emitJumpIfNotZeroTypeArgs(Label label) { - emitWord( - _encodeT(Opcode.kJumpIfNotZeroTypeArgs, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfNotZeroTypeArgs, label); } void emitJumpIfEqStrict(Label label) { - emitWord(_encodeT(Opcode.kJumpIfEqStrict, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfEqStrict, label); } void emitJumpIfNeStrict(Label label) { - emitWord(_encodeT(Opcode.kJumpIfNeStrict, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfNeStrict, label); } void emitJumpIfTrue(Label label) { - emitWord(_encodeT(Opcode.kJumpIfTrue, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfTrue, label); } void emitJumpIfFalse(Label label) { - emitWord(_encodeT(Opcode.kJumpIfFalse, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfFalse, label); } void emitJumpIfNull(Label label) { - emitWord(_encodeT(Opcode.kJumpIfNull, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfNull, label); } void emitJumpIfNotNull(Label label) { - emitWord(_encodeT(Opcode.kJumpIfNotNull, label.jumpOperand(offset))); + _emitJumpBytecode(Opcode.kJumpIfNotNull, label); } void patchJump(int pos, int rt) { @@ -198,6 +218,7 @@ class BytecodeAssembler { void emitReturnTOS() { emitWord(_encode0(Opcode.kReturnTOS)); + isUnreachable = true; } void emitPush(int rx) { @@ -306,6 +327,7 @@ class BytecodeAssembler { void emitThrow(int ra) { emitWord(_encodeA(Opcode.kThrow, ra)); + isUnreachable = true; } void emitEntry(int rd) { diff --git a/pkg/vm/lib/bytecode/gen_bytecode.dart b/pkg/vm/lib/bytecode/gen_bytecode.dart index ad721072abe98..1a95682b7fbc9 100644 --- a/pkg/vm/lib/bytecode/gen_bytecode.dart +++ b/pkg/vm/lib/bytecode/gen_bytecode.dart @@ -166,7 +166,7 @@ class BytecodeGenerator extends RecursiveVisitor { _genNativeCall(nativeName); } else { node.function?.body?.accept(this); - // TODO(alexmarkov): figure out when 'return null' should be generated. + // BytecodeAssembler eliminates this bytecode if it is unreachable. asm.emitPushNull(); } _genReturnTOS(); @@ -624,6 +624,48 @@ class BytecodeGenerator extends RecursiveVisitor { _genJumpIfFalse(!negated, dest); } + /// Returns value of the given expression if it is a bool constant. + /// Otherwise, returns `null`. + bool _constantConditionValue(Expression condition) { + // TODO(dartbug.com/34585): use constant evaluator to evaluate + // expressions in a non-constant context. + if (condition is Not) { + final operand = _constantConditionValue(condition.operand); + return (operand != null) ? !operand : null; + } + if (condition is BoolLiteral) { + return condition.value; + } + Constant constant; + if (condition is ConstantExpression) { + constant = condition.constant; + } else if ((condition is StaticGet && condition.target.isConst) || + (condition is StaticInvocation && condition.isConst) || + (condition is VariableGet && condition.variable.isConst)) { + constant = _evaluateConstantExpression(condition); + } + if (constant is BoolConstant) { + return constant.value; + } + return null; + } + + void _genConditionAndJumpIf(Expression condition, bool value, Label dest) { + final bool constantValue = _constantConditionValue(condition); + if (constantValue != null) { + if (constantValue == value) { + asm.emitJump(dest); + } + return; + } + bool negated = _genCondition(condition); + if (value) { + _genJumpIfTrue(negated, dest); + } else { + _genJumpIfFalse(negated, dest); + } + } + int _getDefaultParamConstIndex(VariableDeclaration param) { if (param.initializer == null) { return cp.add(const ConstantNull()); @@ -1195,7 +1237,7 @@ class BytecodeGenerator extends RecursiveVisitor { function.body.accept(this); - // TODO(alexmarkov): figure out when 'return null' should be generated. + // BytecodeAssembler eliminates this bytecode if it is unreachable. asm.emitPushNull(); _genReturnTOS(); @@ -1513,8 +1555,7 @@ class BytecodeGenerator extends RecursiveVisitor { final Label done = new Label(); final int temp = locals.tempIndexInFrame(node); - final bool negated = _genCondition(node.condition); - _genJumpIfFalse(negated, otherwisePart); + _genConditionAndJumpIf(node.condition, false, otherwisePart); node.then.accept(this); asm.emitPopLocal(temp); @@ -1708,18 +1749,9 @@ class BytecodeGenerator extends RecursiveVisitor { final int temp = locals.tempIndexInFrame(node); final isOR = (node.operator == '||'); - bool negated = _genCondition(node.left); - if (negated != isOR) { - // OR: if (condition == true) - // AND: if ((!condition) == true) - asm.emitJumpIfTrue(shortCircuit); - } else { - // OR: if ((!condition) != true) - // AND: if (condition != true) - asm.emitJumpIfFalse(shortCircuit); - } + _genConditionAndJumpIf(node.left, isOR, shortCircuit); - negated = _genCondition(node.right); + bool negated = _genCondition(node.right); if (negated) { asm.emitBooleanNegateTOS(); } @@ -2171,8 +2203,7 @@ class BytecodeGenerator extends RecursiveVisitor { final Label done = new Label(); asm.emitJumpIfNoAsserts(done); - final bool negated = _genCondition(node.condition); - _genJumpIfTrue(negated, done); + _genConditionAndJumpIf(node.condition, true, done); _genPushInt(omitSourcePositions ? 0 : node.conditionStartOffset); _genPushInt(omitSourcePositions ? 0 : node.conditionEndOffset); @@ -2234,16 +2265,20 @@ class BytecodeGenerator extends RecursiveVisitor { @override visitDoStatement(DoStatement node) { - final Label join = new Label(); + if (asm.isUnreachable) { + // Bail out before binding a label which allows backward jumps, + // as it is not handled by local unreachable code elimination. + return; + } + + final Label join = new Label(allowsBackwardJumps: true); asm.bind(join); asm.emitCheckStack(); node.body.accept(this); - // TODO(alexmarkov): do we need to break this critical edge in CFG? - bool negated = _genCondition(node.condition); - _genJumpIfTrue(negated, join); + _genConditionAndJumpIf(node.condition, true, join); } @override @@ -2283,8 +2318,14 @@ class BytecodeGenerator extends RecursiveVisitor { _genStoreVar(capturedIteratorVar); } + if (asm.isUnreachable) { + // Bail out before binding a label which allows backward jumps, + // as it is not handled by local unreachable code elimination. + return; + } + final Label done = new Label(); - final Label join = new Label(); + final Label join = new Label(allowsBackwardJumps: true); asm.bind(join); asm.emitCheckStack(); @@ -2325,37 +2366,44 @@ class BytecodeGenerator extends RecursiveVisitor { @override visitForStatement(ForStatement node) { _enterScope(node); + try { + visitList(node.variables, this); - visitList(node.variables, this); + if (asm.isUnreachable) { + // Bail out before binding a label which allows backward jumps, + // as it is not handled by local unreachable code elimination. + return; + } - final Label done = new Label(); - final Label join = new Label(); - asm.bind(join); + final Label done = new Label(); + final Label join = new Label(allowsBackwardJumps: true); + asm.bind(join); - asm.emitCheckStack(); + asm.emitCheckStack(); - if (node.condition != null) { - bool negated = _genCondition(node.condition); - _genJumpIfFalse(negated, done); - } + if (node.condition != null) { + _genConditionAndJumpIf(node.condition, false, done); + } - node.body.accept(this); + node.body.accept(this); - if (locals.currentContextSize > 0) { - asm.emitPush(locals.contextVarIndexInFrame); - asm.emitCloneContext(); - asm.emitPopLocal(locals.contextVarIndexInFrame); - } + if (locals.currentContextSize > 0) { + asm.emitPush(locals.contextVarIndexInFrame); + asm.emitCloneContext(); + asm.emitPopLocal(locals.contextVarIndexInFrame); + } - for (var update in node.updates) { - update.accept(this); - asm.emitDrop1(); - } + for (var update in node.updates) { + update.accept(this); + asm.emitDrop1(); + } - asm.emitJump(join); + asm.emitJump(join); - asm.bind(done); - _leaveScope(); + asm.bind(done); + } finally { + _leaveScope(); + } } @override @@ -2369,8 +2417,7 @@ class BytecodeGenerator extends RecursiveVisitor { visitIfStatement(IfStatement node) { final Label otherwisePart = new Label(); - final bool negated = _genCondition(node.condition); - _genJumpIfFalse(negated, otherwisePart); + _genConditionAndJumpIf(node.condition, false, otherwisePart); node.then.accept(this); @@ -2431,12 +2478,18 @@ class BytecodeGenerator extends RecursiveVisitor { node.expression.accept(this); + if (asm.isUnreachable) { + // Bail out before binding labels which allow backward jumps, + // as they are not handled by local unreachable code elimination. + return; + } + final int temp = locals.tempIndexInFrame(node); asm.emitPopLocal(temp); final Label done = new Label(); - final List