From 2517207175c33a8f884045351c3fafbfa1f27326 Mon Sep 17 00:00:00 2001 From: Windel Bouwman Date: Wed, 28 Aug 2019 12:06:46 +0200 Subject: [PATCH] Make subscript its own bytecode. Remove pass bytecode. Move complex bytecodes into seperate functions to reduce complexity of dispatch function. --- bytecode/src/bytecode.rs | 5 +- compiler/src/compile.rs | 10 +- vm/src/frame.rs | 474 ++++++++++++++++++++------------------- 3 files changed, 249 insertions(+), 240 deletions(-) diff --git a/bytecode/src/bytecode.rs b/bytecode/src/bytecode.rs index 23a81a60c..922ed78c0 100644 --- a/bytecode/src/bytecode.rs +++ b/bytecode/src/bytecode.rs @@ -115,6 +115,7 @@ pub enum Instruction { DeleteName { name: String, }, + Subscript, StoreSubscript, DeleteSubscript, StoreAttr { @@ -145,7 +146,6 @@ pub enum Instruction { }, Duplicate, GetIter, - Pass, Continue, Break, Jump { @@ -316,7 +316,6 @@ pub enum BinaryOperator { Modulo, Add, Subtract, - Subscript, Lshift, Rshift, And, @@ -467,6 +466,7 @@ impl Instruction { LoadName { name, scope } => w!(LoadName, name, format!("{:?}", scope)), StoreName { name, scope } => w!(StoreName, name, format!("{:?}", scope)), DeleteName { name } => w!(DeleteName, name), + Subscript => w!(Subscript), StoreSubscript => w!(StoreSubscript), DeleteSubscript => w!(DeleteSubscript), StoreAttr { name } => w!(StoreAttr, name), @@ -487,7 +487,6 @@ impl Instruction { Rotate { amount } => w!(Rotate, amount), Duplicate => w!(Duplicate), GetIter => w!(GetIter), - Pass => w!(Pass), Continue => w!(Continue), Break => w!(Break), Jump { target } => w!(Jump, label_map[target]), diff --git a/compiler/src/compile.rs b/compiler/src/compile.rs index 40173dbaa..7cdfa1a36 100644 --- a/compiler/src/compile.rs +++ b/compiler/src/compile.rs @@ -584,7 +584,7 @@ impl Compiler { } } Pass => { - self.emit(Instruction::Pass); + // No need to emit any code here :) } } Ok(()) @@ -1443,10 +1443,7 @@ impl Compiler { Subscript { a, b } => { self.compile_expression(a)?; self.compile_expression(b)?; - self.emit(Instruction::BinaryOperation { - op: bytecode::BinaryOperator::Subscript, - inplace: false, - }); + self.emit(Instruction::Subscript); } Unop { op, a } => { self.compile_expression(a)?; @@ -2129,7 +2126,6 @@ mod tests { JumpIfFalse { target: Label::new(0) }, - Pass, LoadConst { value: None }, ReturnValue ], @@ -2160,7 +2156,6 @@ mod tests { JumpIfFalse { target: Label::new(0) }, - Pass, LoadConst { value: None }, ReturnValue ], @@ -2197,7 +2192,6 @@ mod tests { JumpIfFalse { target: Label::new(0) }, - Pass, LoadConst { value: None }, ReturnValue ], diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 9028375e6..fc9c5bcc0 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -195,7 +195,6 @@ impl Frame { } /// Execute a single instruction. - #[allow(clippy::cognitive_complexity)] fn execute_instruction(&self, vm: &VirtualMachine) -> FrameResult { #[cfg(not(target_arch = "wasm32"))] check_signals(vm)?; @@ -239,6 +238,7 @@ impl Frame { ref scope, } => self.store_name(vm, name, scope), bytecode::Instruction::DeleteName { ref name } => self.delete_name(vm, name), + bytecode::Instruction::Subscript => self.execute_subscript(vm), bytecode::Instruction::StoreSubscript => self.execute_store_subscript(vm), bytecode::Instruction::DeleteSubscript => self.execute_delete_subscript(vm), bytecode::Instruction::Pop => { @@ -253,29 +253,7 @@ impl Frame { self.push_value(value); Ok(None) } - bytecode::Instruction::Rotate { amount } => { - // Shuffles top of stack amount down - if *amount < 2 { - panic!("Can only rotate two or more values"); - } - - let mut values = Vec::new(); - - // Pop all values from stack: - for _ in 0..*amount { - values.push(self.pop_value()); - } - - // Push top of stack back first: - self.push_value(values.remove(0)); - - // Push other value back in order: - values.reverse(); - for value in values { - self.push_value(value); - } - Ok(None) - } + bytecode::Instruction::Rotate { amount } => self.execute_rotate(*amount), bytecode::Instruction::BuildString { size } => { let s = self .pop_multiple(*size) @@ -308,45 +286,9 @@ impl Frame { Ok(None) } bytecode::Instruction::BuildMap { size, unpack } => { - let map_obj = vm.ctx.new_dict(); - if *unpack { - for obj in self.pop_multiple(*size) { - // Take all key-value pairs from the dict: - let dict: PyDictRef = - obj.downcast().expect("Need a dictionary to build a map."); - for (key, value) in dict { - map_obj.set_item(&key, value, vm).unwrap(); - } - } - } else { - for (key, value) in self.pop_multiple(2 * size).into_iter().tuples() { - map_obj.set_item(&key, value, vm).unwrap(); - } - } - - self.push_value(map_obj.into_object()); - Ok(None) - } - bytecode::Instruction::BuildSlice { size } => { - assert!(*size == 2 || *size == 3); - - let step = if *size == 3 { - Some(self.pop_value()) - } else { - None - }; - let stop = self.pop_value(); - let start = self.pop_value(); - - let obj = PySlice { - start: Some(start), - stop, - step, - } - .into_ref(vm); - self.push_value(obj.into_object()); - Ok(None) + self.execute_build_map(vm, *size, *unpack) } + bytecode::Instruction::BuildSlice { size } => self.execute_build_slice(vm, *size), bytecode::Instruction::ListAppend { i } => { let list_obj = self.nth_value(*i); let item = self.pop_value(); @@ -382,22 +324,7 @@ impl Frame { let value = self.pop_value(); Ok(Some(ExecutionResult::Yield(value))) } - bytecode::Instruction::YieldFrom => { - // Value send into iterator: - self.pop_value(); - - let top_of_stack = self.last_value(); - let next_obj = objiter::get_next_object(vm, &top_of_stack)?; - - match next_obj { - Some(value) => { - // Set back program counter: - *self.lasti.borrow_mut() -= 1; - Ok(Some(ExecutionResult::Yield(value))) - } - None => Ok(None), - } - } + bytecode::Instruction::YieldFrom => self.execute_yield_from(vm), bytecode::Instruction::SetupLoop { start, end } => { self.push_block(BlockType::Loop { start: *start, @@ -470,76 +397,9 @@ impl Frame { self.push_value(iter_obj); Ok(None) } - bytecode::Instruction::ForIter { target } => { - // The top of stack contains the iterator, lets push it forward: - let top_of_stack = self.last_value(); - let next_obj = objiter::get_next_object(vm, &top_of_stack); - - // Check the next object: - match next_obj { - Ok(Some(value)) => { - self.push_value(value); - Ok(None) - } - Ok(None) => { - // Pop iterator from stack: - self.pop_value(); - - // End of for loop - self.jump(*target); - Ok(None) - } - Err(next_error) => { - // Pop iterator from stack: - self.pop_value(); - Err(next_error) - } - } - } + bytecode::Instruction::ForIter { target } => self.execute_for_iter(vm, *target), bytecode::Instruction::MakeFunction { flags } => self.execute_make_function(vm, *flags), - bytecode::Instruction::CallFunction { typ } => { - let args = match typ { - bytecode::CallType::Positional(count) => { - let args: Vec = self.pop_multiple(*count); - PyFuncArgs { - args, - kwargs: IndexMap::new(), - } - } - bytecode::CallType::Keyword(count) => { - let kwarg_names = self.pop_value(); - let args: Vec = self.pop_multiple(*count); - - let kwarg_names = vm - .extract_elements(&kwarg_names)? - .iter() - .map(|pyobj| objstr::get_value(pyobj)) - .collect(); - PyFuncArgs::new(args, kwarg_names) - } - bytecode::CallType::Ex(has_kwargs) => { - let kwargs = if *has_kwargs { - let kw_dict: PyDictRef = - self.pop_value().downcast().expect("Kwargs must be a dict."); - kw_dict - .into_iter() - .map(|elem| (objstr::get_value(&elem.0), elem.1)) - .collect() - } else { - IndexMap::new() - }; - let args = self.pop_value(); - let args = vm.extract_elements(&args)?; - PyFuncArgs { args, kwargs } - } - }; - - // Call function: - let func_ref = self.pop_value(); - let value = vm.invoke(&func_ref, args)?; - self.push_value(value); - Ok(None) - } + bytecode::Instruction::CallFunction { typ } => self.execute_call_function(vm, typ), bytecode::Instruction::Jump { target } => { self.jump(*target); Ok(None) @@ -584,46 +444,9 @@ impl Frame { Ok(None) } - bytecode::Instruction::Raise { argc } => { - let cause = match argc { - 2 => self.get_exception(vm, true)?, - _ => vm.get_none(), - }; - let exception = match argc { - 0 => match vm.current_exception() { - Some(exc) => exc, - None => { - return Err(vm.new_exception( - vm.ctx.exceptions.runtime_error.clone(), - "No active exception to reraise".to_string(), - )); - } - }, - 1 | 2 => self.get_exception(vm, false)?, - 3 => panic!("Not implemented!"), - _ => panic!("Invalid parameter for RAISE_VARARGS, must be between 0 to 3"), - }; - let context = match argc { - 0 => vm.get_none(), // We have already got the exception, - _ => match vm.current_exception() { - Some(exc) => exc, - None => vm.get_none(), - }, - }; - info!( - "Exception raised: {:?} with cause: {:?} and context: {:?}", - exception, cause, context - ); - vm.set_attr(&exception, vm.new_str("__cause__".to_string()), cause)?; - vm.set_attr(&exception, vm.new_str("__context__".to_string()), context)?; - Err(exception) - } + bytecode::Instruction::Raise { argc } => self.execute_raise(vm, *argc), bytecode::Instruction::Break => self.unwind_blocks(vm, UnwindReason::Break), - bytecode::Instruction::Pass => { - // Ah, this is nice, just relax! - Ok(None) - } bytecode::Instruction::Continue => self.unwind_blocks(vm, UnwindReason::Continue), bytecode::Instruction::PrintExpr => { let expr = self.pop_value(); @@ -653,48 +476,9 @@ impl Frame { } } bytecode::Instruction::UnpackEx { before, after } => { - let value = self.pop_value(); - let elements = vm.extract_elements(&value)?; - let min_expected = *before + *after; - if elements.len() < min_expected { - Err(vm.new_value_error(format!( - "Not enough values to unpack (expected at least {}, got {}", - min_expected, - elements.len() - ))) - } else { - let middle = elements.len() - *before - *after; - - // Elements on stack from right-to-left: - for element in elements[*before + middle..].iter().rev() { - self.push_value(element.clone()); - } - - let middle_elements = elements - .iter() - .skip(*before) - .take(middle) - .cloned() - .collect(); - let t = vm.ctx.new_list(middle_elements); - self.push_value(t); - - // Lastly the first reversed values: - for element in elements[..*before].iter().rev() { - self.push_value(element.clone()); - } - - Ok(None) - } - } - bytecode::Instruction::Unpack => { - let value = self.pop_value(); - let elements = vm.extract_elements(&value)?; - for element in elements.into_iter().rev() { - self.push_value(element); - } - Ok(None) + self.execute_unpack_ex(vm, *before, *after) } + bytecode::Instruction::Unpack => self.execute_unpack(vm), bytecode::Instruction::FormatValue { conversion, spec } => { use bytecode::ConversionFlag::*; let value = match conversion { @@ -984,6 +768,38 @@ impl Frame { Ok(None) } + fn execute_rotate(&self, amount: usize) -> FrameResult { + // Shuffles top of stack amount down + if amount < 2 { + panic!("Can only rotate two or more values"); + } + + let mut values = Vec::new(); + + // Pop all values from stack: + for _ in 0..amount { + values.push(self.pop_value()); + } + + // Push top of stack back first: + self.push_value(values.remove(0)); + + // Push other value back in order: + values.reverse(); + for value in values { + self.push_value(value); + } + Ok(None) + } + + fn execute_subscript(&self, vm: &VirtualMachine) -> FrameResult { + let b_ref = self.pop_value(); + let a_ref = self.pop_value(); + let value = a_ref.get_item(&b_ref, vm)?; + self.push_value(value); + Ok(None) + } + fn execute_store_subscript(&self, vm: &VirtualMachine) -> FrameResult { let idx = self.pop_value(); let obj = self.pop_value(); @@ -999,6 +815,183 @@ impl Frame { Ok(None) } + fn execute_build_map(&self, vm: &VirtualMachine, size: usize, unpack: bool) -> FrameResult { + let map_obj = vm.ctx.new_dict(); + if unpack { + for obj in self.pop_multiple(size) { + // Take all key-value pairs from the dict: + let dict: PyDictRef = obj.downcast().expect("Need a dictionary to build a map."); + for (key, value) in dict { + map_obj.set_item(&key, value, vm).unwrap(); + } + } + } else { + for (key, value) in self.pop_multiple(2 * size).into_iter().tuples() { + map_obj.set_item(&key, value, vm).unwrap(); + } + } + + self.push_value(map_obj.into_object()); + Ok(None) + } + + fn execute_build_slice(&self, vm: &VirtualMachine, size: usize) -> FrameResult { + assert!(size == 2 || size == 3); + + let step = if size == 3 { + Some(self.pop_value()) + } else { + None + }; + let stop = self.pop_value(); + let start = self.pop_value(); + + let obj = PySlice { + start: Some(start), + stop, + step, + } + .into_ref(vm); + self.push_value(obj.into_object()); + Ok(None) + } + + fn execute_call_function(&self, vm: &VirtualMachine, typ: &bytecode::CallType) -> FrameResult { + let args = match typ { + bytecode::CallType::Positional(count) => { + let args: Vec = self.pop_multiple(*count); + PyFuncArgs { + args, + kwargs: IndexMap::new(), + } + } + bytecode::CallType::Keyword(count) => { + let kwarg_names = self.pop_value(); + let args: Vec = self.pop_multiple(*count); + + let kwarg_names = vm + .extract_elements(&kwarg_names)? + .iter() + .map(|pyobj| objstr::get_value(pyobj)) + .collect(); + PyFuncArgs::new(args, kwarg_names) + } + bytecode::CallType::Ex(has_kwargs) => { + let kwargs = if *has_kwargs { + let kw_dict: PyDictRef = + self.pop_value().downcast().expect("Kwargs must be a dict."); + kw_dict + .into_iter() + .map(|elem| (objstr::get_value(&elem.0), elem.1)) + .collect() + } else { + IndexMap::new() + }; + let args = self.pop_value(); + let args = vm.extract_elements(&args)?; + PyFuncArgs { args, kwargs } + } + }; + + // Call function: + let func_ref = self.pop_value(); + let value = vm.invoke(&func_ref, args)?; + self.push_value(value); + Ok(None) + } + + fn execute_raise(&self, vm: &VirtualMachine, argc: usize) -> FrameResult { + let cause = match argc { + 2 => self.get_exception(vm, true)?, + _ => vm.get_none(), + }; + let exception = match argc { + 0 => match vm.current_exception() { + Some(exc) => exc, + None => { + return Err(vm.new_exception( + vm.ctx.exceptions.runtime_error.clone(), + "No active exception to reraise".to_string(), + )); + } + }, + 1 | 2 => self.get_exception(vm, false)?, + 3 => panic!("Not implemented!"), + _ => panic!("Invalid parameter for RAISE_VARARGS, must be between 0 to 3"), + }; + let context = match argc { + 0 => vm.get_none(), // We have already got the exception, + _ => match vm.current_exception() { + Some(exc) => exc, + None => vm.get_none(), + }, + }; + info!( + "Exception raised: {:?} with cause: {:?} and context: {:?}", + exception, cause, context + ); + vm.set_attr(&exception, vm.new_str("__cause__".to_string()), cause)?; + vm.set_attr(&exception, vm.new_str("__context__".to_string()), context)?; + Err(exception) + } + + fn execute_yield_from(&self, vm: &VirtualMachine) -> FrameResult { + // Value send into iterator: + self.pop_value(); + + let top_of_stack = self.last_value(); + let next_obj = objiter::get_next_object(vm, &top_of_stack)?; + + match next_obj { + Some(value) => { + // Set back program counter: + *self.lasti.borrow_mut() -= 1; + Ok(Some(ExecutionResult::Yield(value))) + } + None => Ok(None), + } + } + + fn execute_unpack_ex(&self, vm: &VirtualMachine, before: usize, after: usize) -> FrameResult { + let value = self.pop_value(); + let elements = vm.extract_elements(&value)?; + let min_expected = before + after; + if elements.len() < min_expected { + Err(vm.new_value_error(format!( + "Not enough values to unpack (expected at least {}, got {}", + min_expected, + elements.len() + ))) + } else { + let middle = elements.len() - before - after; + + // Elements on stack from right-to-left: + for element in elements[before + middle..].iter().rev() { + self.push_value(element.clone()); + } + + let middle_elements = elements.iter().skip(before).take(middle).cloned().collect(); + let t = vm.ctx.new_list(middle_elements); + self.push_value(t); + + // Lastly the first reversed values: + for element in elements[..before].iter().rev() { + self.push_value(element.clone()); + } + + Ok(None) + } + } + + fn execute_unpack(&self, vm: &VirtualMachine) -> FrameResult { + let value = self.pop_value(); + let elements = vm.extract_elements(&value)?; + for element in elements.into_iter().rev() { + self.push_value(element); + } + Ok(None) + } + fn jump(&self, label: bytecode::Label) { let target_pc = self.code.label_map[&label]; #[cfg(feature = "vm-tracing-logging")] @@ -1006,6 +999,32 @@ impl Frame { self.lasti.replace(target_pc); } + /// The top of stack contains the iterator, lets push it forward + fn execute_for_iter(&self, vm: &VirtualMachine, target: bytecode::Label) -> FrameResult { + let top_of_stack = self.last_value(); + let next_obj = objiter::get_next_object(vm, &top_of_stack); + + // Check the next object: + match next_obj { + Ok(Some(value)) => { + self.push_value(value); + Ok(None) + } + Ok(None) => { + // Pop iterator from stack: + self.pop_value(); + + // End of for loop + self.jump(target); + Ok(None) + } + Err(next_error) => { + // Pop iterator from stack: + self.pop_value(); + Err(next_error) + } + } + } fn execute_make_function( &self, vm: &VirtualMachine, @@ -1086,7 +1105,6 @@ impl Frame { bytecode::BinaryOperator::Power => vm._ipow(a_ref, b_ref), bytecode::BinaryOperator::Divide => vm._itruediv(a_ref, b_ref), bytecode::BinaryOperator::FloorDivide => vm._ifloordiv(a_ref, b_ref), - bytecode::BinaryOperator::Subscript => unreachable!(), bytecode::BinaryOperator::Modulo => vm._imod(a_ref, b_ref), bytecode::BinaryOperator::Lshift => vm._ilshift(a_ref, b_ref), bytecode::BinaryOperator::Rshift => vm._irshift(a_ref, b_ref), @@ -1103,8 +1121,6 @@ impl Frame { bytecode::BinaryOperator::Power => vm._pow(a_ref, b_ref), bytecode::BinaryOperator::Divide => vm._truediv(a_ref, b_ref), bytecode::BinaryOperator::FloorDivide => vm._floordiv(a_ref, b_ref), - // TODO: Subscript should probably have its own op - bytecode::BinaryOperator::Subscript => a_ref.get_item(&b_ref, vm), bytecode::BinaryOperator::Modulo => vm._mod(a_ref, b_ref), bytecode::BinaryOperator::Lshift => vm._lshift(a_ref, b_ref), bytecode::BinaryOperator::Rshift => vm._rshift(a_ref, b_ref),