From 8d169409431d7b55b83e405a009728f5ea9d0db3 Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Sat, 25 Mar 2023 02:43:03 +0000 Subject: [PATCH] Fix update expressions getting values multiple times (#2733) Currently update expressions get values multiple times. This can lead to multiple executions of object property getters. This is very similar to #2551. Unfortunateley it seems like we have to sacrifice some code duplication for correctness in these cases. But that is probably for the best, as we can generate more optimized bytecode for each of these `get/set` cases. --- .../src/bytecompiler/expression/update.rs | 157 ++++++++++++++---- 1 file changed, 126 insertions(+), 31 deletions(-) diff --git a/boa_engine/src/bytecompiler/expression/update.rs b/boa_engine/src/bytecompiler/expression/update.rs index cb3a2d10b6..1249c6a372 100644 --- a/boa_engine/src/bytecompiler/expression/update.rs +++ b/boa_engine/src/bytecompiler/expression/update.rs @@ -2,41 +2,136 @@ use crate::{ bytecompiler::{Access, ByteCompiler}, vm::Opcode, }; -use boa_ast::expression::operator::{update::UpdateOp, Update}; +use boa_ast::expression::{ + access::{PropertyAccess, PropertyAccessField}, + operator::{update::UpdateOp, Update}, +}; impl ByteCompiler<'_, '_> { pub(crate) fn compile_update(&mut self, update: &Update, use_expr: bool) { - let access = Access::from_update_target(update.target()); - - match update.op() { - UpdateOp::IncrementPre => { - self.access_set(access, true, |compiler, _| { - compiler.access_get(access, true); - compiler.emit_opcode(Opcode::Inc); - }); - } - UpdateOp::DecrementPre => { - self.access_set(access, true, |compiler, _| { - compiler.access_get(access, true); - compiler.emit_opcode(Opcode::Dec); - }); - } - UpdateOp::IncrementPost => { - self.access_set(access, false, |compiler, level| { - compiler.access_get(access, true); - compiler.emit_opcode(Opcode::IncPost); - compiler.emit_opcode(Opcode::RotateRight); - compiler.emit_u8(level + 2); - }); - } - UpdateOp::DecrementPost => { - self.access_set(access, false, |compiler, level| { - compiler.access_get(access, true); - compiler.emit_opcode(Opcode::DecPost); - compiler.emit_opcode(Opcode::RotateRight); - compiler.emit_u8(level + 2); - }); + let opcode = match update.op() { + UpdateOp::IncrementPre => Opcode::Inc, + UpdateOp::DecrementPre => Opcode::Dec, + UpdateOp::IncrementPost => Opcode::IncPost, + UpdateOp::DecrementPost => Opcode::DecPost, + }; + let post = matches!( + update.op(), + UpdateOp::IncrementPost | UpdateOp::DecrementPost + ); + + match Access::from_update_target(update.target()) { + Access::Variable { name } => { + let binding = self.context.get_binding_value(name); + let index = self.get_or_insert_binding(binding); + self.emit(Opcode::GetName, &[index]); + + self.emit_opcode(opcode); + if post { + self.emit_opcode(Opcode::Swap); + } else { + self.emit_opcode(Opcode::Dup); + } + + let binding = self.context.set_mutable_binding(name); + let index = self.get_or_insert_binding(binding); + self.emit(Opcode::SetName, &[index]); } + Access::Property { access } => match access { + PropertyAccess::Simple(access) => match access.field() { + PropertyAccessField::Const(name) => { + let index = self.get_or_insert_name((*name).into()); + self.compile_expr(access.target(), true); + self.emit_opcode(Opcode::Dup); + self.emit_opcode(Opcode::Dup); + + self.emit(Opcode::GetPropertyByName, &[index]); + self.emit_opcode(opcode); + if post { + self.emit_opcode(Opcode::RotateRight); + self.emit_u8(4); + } + + self.emit(Opcode::SetPropertyByName, &[index]); + if post { + self.emit_opcode(Opcode::Pop); + } + } + PropertyAccessField::Expr(expr) => { + self.compile_expr(access.target(), true); + self.emit_opcode(Opcode::Dup); + self.compile_expr(expr, true); + + self.emit_opcode(Opcode::GetPropertyByValuePush); + self.emit_opcode(opcode); + if post { + self.emit_opcode(Opcode::RotateRight); + self.emit_u8(4); + } + + self.emit_opcode(Opcode::SetPropertyByValue); + if post { + self.emit_opcode(Opcode::Pop); + } + } + }, + PropertyAccess::Private(access) => { + let index = self.get_or_insert_private_name(access.field()); + self.compile_expr(access.target(), true); + self.emit_opcode(Opcode::Dup); + + self.emit(Opcode::GetPrivateField, &[index]); + self.emit_opcode(opcode); + if post { + self.emit_opcode(Opcode::RotateRight); + self.emit_u8(3); + } + + self.emit(Opcode::SetPrivateField, &[index]); + if post { + self.emit_opcode(Opcode::Pop); + } + } + PropertyAccess::Super(access) => match access.field() { + PropertyAccessField::Const(name) => { + let index = self.get_or_insert_name((*name).into()); + self.emit_opcode(Opcode::Super); + self.emit_opcode(Opcode::Dup); + self.emit_opcode(Opcode::This); + self.emit_opcode(Opcode::Swap); + + self.emit(Opcode::GetPropertyByName, &[index]); + self.emit_opcode(opcode); + if post { + self.emit_opcode(Opcode::RotateRight); + self.emit_u8(3); + } + + self.emit(Opcode::SetPropertyByName, &[index]); + if post { + self.emit_opcode(Opcode::Pop); + } + } + PropertyAccessField::Expr(expr) => { + self.emit_opcode(Opcode::Super); + self.emit_opcode(Opcode::Dup); + self.compile_expr(expr, true); + + self.emit_opcode(Opcode::GetPropertyByValuePush); + self.emit_opcode(opcode); + if post { + self.emit_opcode(Opcode::RotateRight); + self.emit_u8(2); + } + + self.emit_opcode(Opcode::SetPropertyByValue); + if post { + self.emit_opcode(Opcode::Pop); + } + } + }, + }, + Access::This => unreachable!(), } if !use_expr {