From a01bd35c67e35e9f0e33f46bb3a4431e49af1b60 Mon Sep 17 00:00:00 2001 From: Matthew Olsson Date: Tue, 8 Jun 2021 14:46:46 -0700 Subject: [PATCH] LibJS: Add bytecode instruction handles This change removes the mmap inside of Block in favor of a growing vector of bytes. This is favorable for two reasons: - We don't take more space than we need - There is no limit to the growth of the vector (previously, if the Block overstepped its 64kb boundary, it would just crash) However, if that vector happens to resize, any pointer pointing into that vector would become invalid. To avoid this, this commit adds an InstructionHandle class which just stores a block and an offset into that block. --- .../Libraries/LibJS/Bytecode/ASTCodegen.cpp | 34 +++++++-------- Userland/Libraries/LibJS/Bytecode/Block.cpp | 28 ------------- Userland/Libraries/LibJS/Bytecode/Block.h | 14 +++---- .../Libraries/LibJS/Bytecode/Generator.cpp | 12 ------ Userland/Libraries/LibJS/Bytecode/Generator.h | 27 ++++++------ .../Libraries/LibJS/Bytecode/Instruction.cpp | 1 + .../Libraries/LibJS/Bytecode/Instruction.h | 42 +++++++++++++++++++ Userland/Libraries/LibJS/Forward.h | 2 + 8 files changed, 81 insertions(+), 79 deletions(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index dd92c4a1963..ad24e32e509 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -119,16 +119,16 @@ void LogicalExpression::generate_bytecode(Bytecode::Generator& generator) const { m_lhs->generate_bytecode(generator); - Bytecode::Op::Jump* test_instr; + Bytecode::InstructionHandle test_instr; switch (m_op) { case LogicalOp::And: - test_instr = &generator.emit(); + test_instr = generator.emit(); break; case LogicalOp::Or: - test_instr = &generator.emit(); + test_instr = generator.emit(); break; case LogicalOp::NullishCoalescing: - test_instr = &generator.emit(); + test_instr = generator.emit(); break; default: VERIFY_NOT_REACHED(); @@ -285,10 +285,10 @@ void WhileStatement::generate_bytecode(Bytecode::Generator& generator) const auto result_reg = generator.allocate_register(); generator.emit(result_reg); m_test->generate_bytecode(generator); - auto& test_jump = generator.emit(); + auto test_jump = generator.emit(); m_body->generate_bytecode(generator); generator.emit(test_label); - test_jump.set_target(generator.make_label()); + test_jump->set_target(generator.make_label()); generator.end_continuable_scope(); generator.emit(result_reg); } @@ -309,7 +309,7 @@ void DoWhileStatement::generate_bytecode(Bytecode::Generator& generator) const void ForStatement::generate_bytecode(Bytecode::Generator& generator) const { - Bytecode::Op::Jump* test_jump { nullptr }; + Bytecode::InstructionHandle test_jump; if (m_init) m_init->generate_bytecode(generator); @@ -321,7 +321,7 @@ void ForStatement::generate_bytecode(Bytecode::Generator& generator) const generator.emit(result_reg); if (m_test) { m_test->generate_bytecode(generator); - test_jump = &generator.emit(); + test_jump = generator.emit(); } m_body->generate_bytecode(generator); @@ -387,16 +387,16 @@ void ReturnStatement::generate_bytecode(Bytecode::Generator& generator) const void IfStatement::generate_bytecode(Bytecode::Generator& generator) const { m_predicate->generate_bytecode(generator); - auto& else_jump = generator.emit(); + auto else_jump = generator.emit(); m_consequent->generate_bytecode(generator); if (m_alternate) { - auto& if_jump = generator.emit(); - else_jump.set_target(generator.make_label()); + auto if_jump = generator.emit(); + else_jump->set_target(generator.make_label()); m_alternate->generate_bytecode(generator); - if_jump.set_target(generator.make_label()); + if_jump->set_target(generator.make_label()); } else { - else_jump.set_target(generator.make_label()); + else_jump->set_target(generator.make_label()); } } @@ -412,15 +412,15 @@ void DebuggerStatement::generate_bytecode(Bytecode::Generator&) const void ConditionalExpression::generate_bytecode(Bytecode::Generator& generator) const { m_test->generate_bytecode(generator); - auto& alternate_jump = generator.emit(); + auto alternate_jump = generator.emit(); m_consequent->generate_bytecode(generator); - auto& end_jump = generator.emit(); + auto end_jump = generator.emit(); - alternate_jump.set_target(generator.make_label()); + alternate_jump->set_target(generator.make_label()); m_alternate->generate_bytecode(generator); - end_jump.set_target(generator.make_label()); + end_jump->set_target(generator.make_label()); } void SequenceExpression::generate_bytecode(Bytecode::Generator& generator) const diff --git a/Userland/Libraries/LibJS/Bytecode/Block.cpp b/Userland/Libraries/LibJS/Bytecode/Block.cpp index ef0add31a11..7ca1a43ec06 100644 --- a/Userland/Libraries/LibJS/Bytecode/Block.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Block.cpp @@ -4,10 +4,8 @@ * SPDX-License-Identifier: BSD-2-Clause */ -#include #include #include -#include namespace JS::Bytecode { @@ -16,16 +14,6 @@ NonnullOwnPtr Block::create() return adopt_own(*new Block); } -Block::Block() -{ - // FIXME: This is not the smartest solution ever. Find something cleverer! - // The main issue we're working around here is that we don't want pointers into the bytecode stream to become invalidated - // during code generation due to dynamic buffer resizing. Otherwise we could just use a Vector. - m_buffer_capacity = 64 * KiB; - m_buffer = (u8*)mmap(nullptr, m_buffer_capacity, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, 0, 0); - VERIFY(m_buffer != MAP_FAILED); -} - Block::~Block() { Bytecode::InstructionStreamIterator it(instruction_stream()); @@ -34,16 +22,6 @@ Block::~Block() ++it; Instruction::destroy(const_cast(to_destroy)); } - - munmap(m_buffer, m_buffer_capacity); -} - -void Block::seal() -{ - // FIXME: mprotect the instruction stream as PROT_READ - // This is currently not possible because instructions can have destructors (that clean up strings) - // Instructions should instead be destructor-less and refer to strings in a string table on the Bytecode::Block. - // It also doesn't work because instructions that have String members use RefPtr internally which must be in writable memory. } void Block::dump() const @@ -55,12 +33,6 @@ void Block::dump() const } } -void Block::grow(size_t additional_size) -{ - m_buffer_size += additional_size; - VERIFY(m_buffer_size <= m_buffer_capacity); -} - void InstructionStreamIterator::operator++() { m_offset += dereference().length(); diff --git a/Userland/Libraries/LibJS/Bytecode/Block.h b/Userland/Libraries/LibJS/Bytecode/Block.h index 4e81d2b1d32..09dca9461af 100644 --- a/Userland/Libraries/LibJS/Bytecode/Block.h +++ b/Userland/Libraries/LibJS/Bytecode/Block.h @@ -8,6 +8,7 @@ #include #include +#include #include namespace JS::Bytecode { @@ -42,25 +43,20 @@ public: static NonnullOwnPtr create(); ~Block(); - void seal(); - void dump() const; - ReadonlyBytes instruction_stream() const { return ReadonlyBytes { m_buffer, m_buffer_size }; } + ReadonlyBytes instruction_stream() const { return m_buffer.span(); } size_t register_count() const { return m_register_count; } void set_register_count(Badge, size_t count) { m_register_count = count; } - void* next_slot() { return m_buffer + m_buffer_size; } - void grow(size_t additional_size); + Vector& buffer() { return m_buffer; } private: - Block(); + Block() = default; size_t m_register_count { 0 }; - u8* m_buffer { nullptr }; - size_t m_buffer_capacity { 0 }; - size_t m_buffer_size { 0 }; + Vector m_buffer; }; } diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.cpp b/Userland/Libraries/LibJS/Bytecode/Generator.cpp index 2d2ad735209..822da0ebfab 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Generator.cpp @@ -10,7 +10,6 @@ #include #include #include -#include namespace JS::Bytecode { @@ -28,20 +27,9 @@ OwnPtr Generator::generate(ASTNode const& node) Generator generator; node.generate_bytecode(generator); generator.m_block->set_register_count({}, generator.m_next_register); - generator.m_block->seal(); return move(generator.m_block); } -void Generator::grow(size_t additional_size) -{ - m_block->grow(additional_size); -} - -void* Generator::next_slot() -{ - return m_block->next_slot(); -} - Register Generator::allocate_register() { VERIFY(m_next_register != NumericLimits::max()); diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.h b/Userland/Libraries/LibJS/Bytecode/Generator.h index 88bac76ebe1..110ad25ac5b 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.h +++ b/Userland/Libraries/LibJS/Bytecode/Generator.h @@ -7,8 +7,8 @@ #pragma once #include +#include #include -#include #include namespace JS::Bytecode { @@ -20,21 +20,15 @@ public: Register allocate_register(); template - OpType& emit(Args&&... args) + InstructionHandle emit(Args&&... args) { - void* slot = next_slot(); - grow(sizeof(OpType)); - new (slot) OpType(forward(args)...); - return *static_cast(slot); + return make_instruction(0, forward(args)...); } template - OpType& emit_with_extra_register_slots(size_t extra_register_slots, Args&&... args) + InstructionHandle emit_with_extra_register_slots(size_t extra_register_slots, Args&&... args) { - void* slot = next_slot(); - grow(sizeof(OpType) + extra_register_slots * sizeof(Register)); - new (slot) OpType(forward(args)...); - return *static_cast(slot); + return make_instruction(extra_register_slots, forward(args)...); } Label make_label() const; @@ -48,8 +42,15 @@ private: Generator(); ~Generator(); - void grow(size_t); - void* next_slot(); + template + InstructionHandle make_instruction(size_t extra_register_slots, Args&&... args) + { + auto& buffer = m_block->buffer(); + auto offset = buffer.size(); + buffer.resize(buffer.size() + sizeof(OpType) + extra_register_slots * sizeof(Register)); + new (buffer.data() + offset) OpType(forward(args)...); + return InstructionHandle(offset, m_block); + } OwnPtr m_block; u32 m_next_register { 1 }; diff --git a/Userland/Libraries/LibJS/Bytecode/Instruction.cpp b/Userland/Libraries/LibJS/Bytecode/Instruction.cpp index 899229607d9..7c85efec48d 100644 --- a/Userland/Libraries/LibJS/Bytecode/Instruction.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Instruction.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include diff --git a/Userland/Libraries/LibJS/Bytecode/Instruction.h b/Userland/Libraries/LibJS/Bytecode/Instruction.h index 1b7286eea8d..b38ed0b804f 100644 --- a/Userland/Libraries/LibJS/Bytecode/Instruction.h +++ b/Userland/Libraries/LibJS/Bytecode/Instruction.h @@ -7,6 +7,7 @@ #pragma once #include +#include #include #define ENUMERATE_BYTECODE_OPS(O) \ @@ -83,4 +84,45 @@ private: Type m_type {}; }; +template +class InstructionHandle { +public: + InstructionHandle() = default; + + InstructionHandle(size_t offset, Block* block) + : m_offset(offset) + , m_block(block) + { + } + + OpType* operator->() const + { + VERIFY(m_block); + return reinterpret_cast(m_block->buffer().data() + m_offset); + } + + OpType& operator*() const + { + VERIFY(m_block); + return *reinterpret_cast(m_block->buffer().data() + m_offset); + } + + template + InstructionHandle& operator=(InstructionHandle const& other) requires(IsBaseOf) + { + m_offset = other.offset(); + m_block = other.block(); + return *this; + } + + size_t offset() const { return m_offset; } + Block* block() const { return m_block; } + +private: + friend class Block; + + size_t m_offset { 0 }; + Block* m_block { nullptr }; +}; + } diff --git a/Userland/Libraries/LibJS/Forward.h b/Userland/Libraries/LibJS/Forward.h index 0a2665cc429..a4fd02fb086 100644 --- a/Userland/Libraries/LibJS/Forward.h +++ b/Userland/Libraries/LibJS/Forward.h @@ -166,6 +166,8 @@ namespace Bytecode { class Block; class Generator; class Instruction; +template +class InstructionHandle; class Interpreter; class Register; }