From fe46ecc75ebfd084e5f216002c71a700ba065e90 Mon Sep 17 00:00:00 2001 From: codehag Date: Wed, 26 Feb 2020 17:39:19 +0100 Subject: [PATCH] Implement a safer api for forward jumps (fixes #231) --- crates/emitter/src/ast_emitter.rs | 98 +++++++++------------- crates/emitter/src/emitter.rs | 3 +- crates/emitter/src/forward_jump_emitter.rs | 80 ++++++++++++++++++ crates/emitter/src/lib.rs | 1 + 4 files changed, 122 insertions(+), 60 deletions(-) create mode 100644 crates/emitter/src/forward_jump_emitter.rs diff --git a/crates/emitter/src/ast_emitter.rs b/crates/emitter/src/ast_emitter.rs index 3aab6b238..bff2943e4 100644 --- a/crates/emitter/src/ast_emitter.rs +++ b/crates/emitter/src/ast_emitter.rs @@ -2,10 +2,12 @@ //! //! Converts AST nodes to bytecode. -use super::emitter::{BytecodeOffset, EmitError, EmitOptions, EmitResult, InstructionWriter}; +use super::emitter::{EmitError, EmitOptions, EmitResult, InstructionWriter}; use super::opcode::Opcode; use ast::types::*; +use crate::forward_jump_emitter::{ForwardJumpEmitter, JumpKind}; + /// Emit a program, converting the AST directly to bytecode. pub fn emit_program(ast: &Program, options: &EmitOptions) -> Result { let mut emitter = AstEmitter { @@ -23,8 +25,8 @@ pub fn emit_program(ast: &Program, options: &EmitOptions) -> Result { - emit: InstructionWriter, +pub struct AstEmitter<'alloc> { + pub emit: InstructionWriter, options: &'alloc EmitOptions, } @@ -129,27 +131,31 @@ impl<'alloc> AstEmitter<'alloc> { fn emit_if(&mut self, if_statement: &IfStatement) -> Result<(), EmitError> { self.emit_expression(&if_statement.test)?; - let offset_alternate = self.emit.bytecode_offset(); - self.emit.if_eq(0); + let alternate_jump = ForwardJumpEmitter { + jump: JumpKind::IfEq, + } + .emit(self); // Then branch self.emit.jump_target(); self.emit_statement(&if_statement.consequent)?; if let Some(alternate) = &if_statement.alternate { - let offset_final = self.emit.bytecode_offset(); - self.emit.goto(0); + let then_jump = ForwardJumpEmitter { + jump: JumpKind::IfEq, + } + .emit(self); // ^^ part of then branch // Else branch - self.emit_jump_target(vec![offset_alternate]); + alternate_jump.patch(self); self.emit_statement(alternate)?; // Merge point after else - self.emit_jump_target(vec![offset_final]); + then_jump.patch(self); } else { // Merge point without else - self.emit_jump_target(vec![offset_alternate]) + alternate_jump.patch(self); } Ok(()) @@ -402,10 +408,16 @@ impl<'alloc> AstEmitter<'alloc> { BinaryOperator::BitwiseXor { .. } => Opcode::BitXor, BinaryOperator::BitwiseAnd { .. } => Opcode::BitAnd, - BinaryOperator::Coalesce { .. } - | BinaryOperator::LogicalOr { .. } - | BinaryOperator::LogicalAnd { .. } => { - self.emit_short_circuit(operator, left, right)?; + BinaryOperator::Coalesce { .. } => { + self.emit_short_circuit(JumpKind::Coalesce, left, right)?; + return Ok(()); + } + BinaryOperator::LogicalOr { .. } => { + self.emit_short_circuit(JumpKind::LogicalOr, left, right)?; + return Ok(()); + } + BinaryOperator::LogicalAnd { .. } => { + self.emit_short_circuit(JumpKind::LogicalAnd, left, right)?; return Ok(()); } @@ -425,54 +437,18 @@ impl<'alloc> AstEmitter<'alloc> { fn emit_short_circuit( &mut self, - operator: &BinaryOperator, + jump: JumpKind, left: &Expression, right: &Expression, ) -> Result<(), EmitError> { self.emit_expression(left)?; - let mut jumplist: Vec = Vec::with_capacity(1); - self.emit_jump(operator, &mut jumplist)?; - self.emit.jump_target(); + let jump = ForwardJumpEmitter { jump: jump }.emit(self); self.emit.pop(); self.emit_expression(right)?; - self.emit_jump_target(jumplist); + jump.patch(self); return Ok(()); } - fn emit_jump( - &mut self, - operator: &BinaryOperator, - jumplist: &mut Vec, - ) -> Result<(), EmitError> { - let offset: BytecodeOffset = self.emit.bytecode_offset(); - jumplist.push(offset); - - // in the c++ bytecode emitter, the jumplist is emitted - // and four bytes are used in order to save memory. We are not using that - // here, so instead we are using a placeholder offset set to 0, which will - // be updated later in patch_jump_target. - let placeholder_offset: i32 = 0; - match operator { - BinaryOperator::Coalesce { .. } => { - self.emit.coalesce(placeholder_offset); - } - BinaryOperator::LogicalOr { .. } => { - self.emit.or(placeholder_offset); - } - BinaryOperator::LogicalAnd { .. } => { - self.emit.and(placeholder_offset); - } - _ => panic!("unrecognized operator used in jump"), - } - - return Ok(()); - } - - fn emit_jump_target(&mut self, jumplist: Vec) { - self.emit.patch_jump_target(jumplist); - self.emit.jump_target(); - } - fn emit_numeric_expression(&mut self, value: f64) { if value.is_finite() && value.fract() == 0.0 { if i8::min_value() as f64 <= value && value <= i8::max_value() as f64 { @@ -565,22 +541,26 @@ impl<'alloc> AstEmitter<'alloc> { ) -> Result<(), EmitError> { self.emit_expression(test)?; - let offset_else = self.emit.bytecode_offset(); - self.emit.if_eq(0); + let else_jump = ForwardJumpEmitter { + jump: JumpKind::IfEq, + } + .emit(self); // Then branch self.emit.jump_target(); self.emit_expression(consequent)?; - let offset_final = self.emit.bytecode_offset(); - self.emit.goto(0); + let finally_jump = ForwardJumpEmitter { + jump: JumpKind::Goto, + } + .emit(self); // Else branch - self.emit_jump_target(vec![offset_else]); + else_jump.patch(self); self.emit_expression(alternate)?; // Merge point - self.emit_jump_target(vec![offset_final]); + finally_jump.patch(self); Ok(()) } diff --git a/crates/emitter/src/emitter.rs b/crates/emitter/src/emitter.rs index ff91e0bdc..6f2d8d2fd 100644 --- a/crates/emitter/src/emitter.rs +++ b/crates/emitter/src/emitter.rs @@ -26,7 +26,8 @@ pub enum AsyncFunctionResolveKind { pub type u24 = u32; /// For tracking bytecode offsets in jumps -#[derive(PartialEq, Debug)] +#[derive(Clone, Copy, PartialEq, Debug)] +#[must_use] pub struct BytecodeOffset { pub offset: usize, } diff --git a/crates/emitter/src/forward_jump_emitter.rs b/crates/emitter/src/forward_jump_emitter.rs new file mode 100644 index 000000000..7475dd15d --- /dev/null +++ b/crates/emitter/src/forward_jump_emitter.rs @@ -0,0 +1,80 @@ +use super::emitter::{BytecodeOffset}; +use crate::ast_emitter::AstEmitter; + +#[derive(Debug)] +pub enum JumpKind { + Coalesce, + LogicalAnd, + LogicalOr, + IfEq, + Goto, +} + +#[derive(Debug)] +#[must_use] +pub struct JumpPatchEmitter { + offset: BytecodeOffset, +} +impl JumpPatchEmitter { + pub fn patch(self, emitter: &mut AstEmitter) { + // TODO: if multiple jumps arrive at same point, only single JumpTarget + // should be emitted. See: + // https://searchfox.org/mozilla-central/rev/49ed791eec93335abfe6c2880f84c324e73e47e6/js/src/frontend/BytecodeEmitter.cpp#371-377 + emitter.emit.patch_jump_target(vec![self.offset]); + emitter.emit.jump_target(); + } +} + +// Struct for emitting bytecode for forward jump. +#[derive(Debug)] +pub struct ForwardJumpEmitter { + pub jump: JumpKind, +} +impl ForwardJumpEmitter { + pub fn emit(&mut self, emitter: &mut AstEmitter) -> JumpPatchEmitter { + let offset: BytecodeOffset = emitter.emit.bytecode_offset(); + self.emit_jump(emitter); + + // The JITs rely on a jump target being emitted after the + // conditional jump + if self.should_fallthrough() { + emitter.emit.jump_target(); + } + JumpPatchEmitter { offset: offset } + } + + fn should_fallthrough(&mut self) -> bool { + // a fallthrough occurs if the jump is a conditional jump and if the + // condition doesn't met, the execution goes to the next opcode + // instead of the target of the jump. + match self.jump { + JumpKind::Goto { .. } => false, + _ => true, + } + } + + fn emit_jump(&mut self, emitter: &mut AstEmitter) { + // in the c++ bytecode emitter, the jumplist is emitted + // and four bytes are used in order to save memory. We are not using that + // here, so instead we are using a placeholder offset set to 0, which will + // be updated later in patch_jump_target. + let placeholder_offset: i32 = 0; + match self.jump { + JumpKind::Coalesce { .. } => { + emitter.emit.coalesce(placeholder_offset); + } + JumpKind::LogicalOr { .. } => { + emitter.emit.or(placeholder_offset); + } + JumpKind::LogicalAnd { .. } => { + emitter.emit.and(placeholder_offset); + } + JumpKind::IfEq { .. } => { + emitter.emit.if_eq(placeholder_offset); + } + JumpKind::Goto { .. } => { + emitter.emit.goto(placeholder_offset); + } + } + } +} diff --git a/crates/emitter/src/lib.rs b/crates/emitter/src/lib.rs index ad452517d..39b380b56 100644 --- a/crates/emitter/src/lib.rs +++ b/crates/emitter/src/lib.rs @@ -1,6 +1,7 @@ mod ast_emitter; mod dis; mod emitter; +mod forward_jump_emitter; mod lower; pub mod opcode;