From 732b39d120161713ae0428de2e62080a30236f43 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 4 Oct 2023 15:22:07 +0200 Subject: [PATCH] LibJS: Don't evaluate computed MemberExpression LHS twice in assignments The following snippet would cause "i" to be incremented twice(!): let a = [] let i = 0 a[++i] += 0 This patch solves the issue by remembering the base object and property name for computed MemberExpression LHS in codegen. We the store the result of the assignment to the same object and property (instead of computing the LHS again). 3 new passes on test262. :^) --- .../Libraries/LibJS/Bytecode/ASTCodegen.cpp | 17 ++++++++---- .../Libraries/LibJS/Bytecode/Generator.cpp | 26 ++++++++++++++++--- Userland/Libraries/LibJS/Bytecode/Generator.h | 14 +++++----- 3 files changed, 42 insertions(+), 15 deletions(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index 5ff9ea8188..1b983753de 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -525,7 +525,7 @@ Bytecode::CodeGenerationErrorOr AssignmentExpression::generate_bytecode(By VERIFY(m_lhs.has>()); auto& lhs = m_lhs.get>(); - TRY(generator.emit_load_from_reference(lhs)); + auto reference_registers = TRY(generator.emit_load_from_reference(lhs)); Bytecode::BasicBlock* rhs_block_ptr { nullptr }; Bytecode::BasicBlock* end_block_ptr { nullptr }; @@ -615,7 +615,10 @@ Bytecode::CodeGenerationErrorOr AssignmentExpression::generate_bytecode(By }; } - TRY(generator.emit_store_to_reference(lhs)); + if (reference_registers.has_value()) + TRY(generator.emit_store_to_reference(*reference_registers)); + else + TRY(generator.emit_store_to_reference(lhs)); if (end_block_ptr) { generator.emit(Bytecode::Label { *end_block_ptr }); @@ -1049,7 +1052,8 @@ Bytecode::CodeGenerationErrorOr ArrayExpression::generate_bytecode(Bytecod Bytecode::CodeGenerationErrorOr MemberExpression::generate_bytecode(Bytecode::Generator& generator) const { Bytecode::Generator::SourceLocationScope scope(generator, *this); - return generator.emit_load_from_reference(*this); + (void)TRY(generator.emit_load_from_reference(*this)); + return {}; } Bytecode::CodeGenerationErrorOr FunctionDeclaration::generate_bytecode(Bytecode::Generator& generator) const @@ -2249,7 +2253,7 @@ Bytecode::CodeGenerationErrorOr TaggedTemplateLiteral::generate_bytecode(B Bytecode::CodeGenerationErrorOr UpdateExpression::generate_bytecode(Bytecode::Generator& generator) const { Bytecode::Generator::SourceLocationScope scope(generator, *this); - TRY(generator.emit_load_from_reference(*m_argument)); + auto reference_registers = TRY(generator.emit_load_from_reference(*m_argument)); Optional previous_value_for_postfix_reg; if (!m_prefixed) { @@ -2263,7 +2267,10 @@ Bytecode::CodeGenerationErrorOr UpdateExpression::generate_bytecode(Byteco else generator.emit(); - TRY(generator.emit_store_to_reference(*m_argument)); + if (reference_registers.has_value()) + TRY(generator.emit_store_to_reference(*reference_registers)); + else + TRY(generator.emit_store_to_reference(*m_argument)); if (!m_prefixed) generator.emit(*previous_value_for_postfix_reg); diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.cpp b/Userland/Libraries/LibJS/Bytecode/Generator.cpp index 6448c0f4c0..d37729a43a 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Generator.cpp @@ -189,12 +189,12 @@ CodeGenerationErrorOr Generator::emit_super_refer }; } -CodeGenerationErrorOr Generator::emit_load_from_reference(JS::ASTNode const& node) +CodeGenerationErrorOr> Generator::emit_load_from_reference(JS::ASTNode const& node) { if (is(node)) { auto& identifier = static_cast(node); TRY(identifier.generate_bytecode(*this)); - return {}; + return Optional {}; } if (is(node)) { auto& expression = static_cast(node); @@ -213,15 +213,24 @@ CodeGenerationErrorOr Generator::emit_load_from_reference(JS::ASTNode cons auto identifier_table_ref = intern_identifier(verify_cast(expression.property()).string()); emit_get_by_id_with_this(identifier_table_ref, super_reference.this_value); } + + return super_reference; } else { TRY(expression.object().generate_bytecode(*this)); - if (expression.is_computed()) { auto object_reg = allocate_register(); emit(object_reg); TRY(expression.property().generate_bytecode(*this)); + auto property_reg = allocate_register(); + emit(property_reg); + emit(object_reg); + return ReferenceRegisters { + .base = object_reg, + .referenced_name = property_reg, + .this_value = object_reg, + }; } else if (expression.property().is_identifier()) { auto identifier_table_ref = intern_identifier(verify_cast(expression.property()).string()); emit_get_by_id(identifier_table_ref); @@ -235,7 +244,7 @@ CodeGenerationErrorOr Generator::emit_load_from_reference(JS::ASTNode cons }; } } - return {}; + return Optional {}; } VERIFY_NOT_REACHED(); } @@ -306,6 +315,15 @@ CodeGenerationErrorOr Generator::emit_store_to_reference(JS::ASTNode const }; } +CodeGenerationErrorOr Generator::emit_store_to_reference(ReferenceRegisters const& reference_registers) +{ + if (reference_registers.base == reference_registers.this_value) + emit(reference_registers.base, reference_registers.referenced_name.value()); + else + emit(reference_registers.base, reference_registers.referenced_name.value(), reference_registers.this_value); + return {}; +} + CodeGenerationErrorOr Generator::emit_delete_reference(JS::ASTNode const& node) { if (is(node)) { diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.h b/Userland/Libraries/LibJS/Bytecode/Generator.h index 9590063d2a..63ab04de63 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.h +++ b/Userland/Libraries/LibJS/Bytecode/Generator.h @@ -74,15 +74,17 @@ public: op->set_source_record({ m_current_ast_node->start_offset(), m_current_ast_node->end_offset() }); } - CodeGenerationErrorOr emit_load_from_reference(JS::ASTNode const&); + struct ReferenceRegisters { + Register base; // [[Base]] + Optional referenced_name; // [[ReferencedName]] + Register this_value; // [[ThisValue]] + }; + + CodeGenerationErrorOr> emit_load_from_reference(JS::ASTNode const&); CodeGenerationErrorOr emit_store_to_reference(JS::ASTNode const&); + CodeGenerationErrorOr emit_store_to_reference(ReferenceRegisters const&); CodeGenerationErrorOr emit_delete_reference(JS::ASTNode const&); - struct ReferenceRegisters { - Register base; // [[Base]] - Optional referenced_name; // [[ReferencedName]] - Register this_value; // [[ThisValue]] - }; CodeGenerationErrorOr emit_super_reference(MemberExpression const&); void emit_set_variable(JS::Identifier const& identifier, Bytecode::Op::SetVariable::InitializationMode initialization_mode = Bytecode::Op::SetVariable::InitializationMode::Set, Bytecode::Op::EnvironmentMode mode = Bytecode::Op::EnvironmentMode::Lexical);