From 12305f501e7b050a4d2b29b5f3e7b6903e0a3835 Mon Sep 17 00:00:00 2001 From: Ben Grant Date: Thu, 26 Sep 2024 16:07:32 -0700 Subject: [PATCH] TaggedPointer assertions and refactoring --- src/bun.js/bindings/v8/V8Data.h | 6 +-- .../v8/V8EscapableHandleScopeBase.cpp | 2 +- src/bun.js/bindings/v8/V8HandleScope.cpp | 2 +- src/bun.js/bindings/v8/real_v8.h | 5 ++ .../bindings/v8/shim/FunctionTemplate.cpp | 2 +- src/bun.js/bindings/v8/shim/Handle.cpp | 2 +- src/bun.js/bindings/v8/shim/Handle.h | 2 +- src/bun.js/bindings/v8/shim/TaggedPointer.cpp | 8 +++ src/bun.js/bindings/v8/shim/TaggedPointer.h | 49 ++++++++++--------- src/bun.js/bindings/v8/v8_api_internal.cpp | 2 +- 10 files changed, 47 insertions(+), 33 deletions(-) create mode 100644 src/bun.js/bindings/v8/shim/TaggedPointer.cpp diff --git a/src/bun.js/bindings/v8/V8Data.h b/src/bun.js/bindings/v8/V8Data.h index 9295db2093701..f3ebc81aafe9e 100644 --- a/src/bun.js/bindings/v8/V8Data.h +++ b/src/bun.js/bindings/v8/V8Data.h @@ -18,7 +18,7 @@ class Data { JSC::JSCell* localToCell() { TaggedPointer root = localToTagged(); - RELEASE_ASSERT(root.type() != TaggedPointer::Type::Smi); + RELEASE_ASSERT(root.tag() != TaggedPointer::Tag::Smi); return root.getPtr()->asCell(); } @@ -44,7 +44,7 @@ class Data { JSC::JSValue localToJSValue() const { TaggedPointer root = localToTagged(); - if (root.type() == TaggedPointer::Type::Smi) { + if (root.tag() == TaggedPointer::Tag::Smi) { return JSC::jsNumber(root.getSmiUnchecked()); } else { using shim::InstanceType; @@ -65,7 +65,7 @@ class Data { const JSC::JSCell* localToCell() const { TaggedPointer root = localToTagged(); - RELEASE_ASSERT(root.type() != TaggedPointer::Type::Smi); + RELEASE_ASSERT(root.tag() != TaggedPointer::Tag::Smi); return root.getPtr()->asCell(); } diff --git a/src/bun.js/bindings/v8/V8EscapableHandleScopeBase.cpp b/src/bun.js/bindings/v8/V8EscapableHandleScopeBase.cpp index 1aa8e117b6079..e9c3c01968801 100644 --- a/src/bun.js/bindings/v8/V8EscapableHandleScopeBase.cpp +++ b/src/bun.js/bindings/v8/V8EscapableHandleScopeBase.cpp @@ -24,7 +24,7 @@ uintptr_t* EscapableHandleScopeBase::EscapeSlot(uintptr_t* escape_value) m_isolate, m_escapeSlot); m_escapeSlot = nullptr; - return newHandle->asRawPtr(); + return newHandle->asRawPtrLocation(); } } // namespace v8 diff --git a/src/bun.js/bindings/v8/V8HandleScope.cpp b/src/bun.js/bindings/v8/V8HandleScope.cpp index 10053311afd68..9071d9621c5c5 100644 --- a/src/bun.js/bindings/v8/V8HandleScope.cpp +++ b/src/bun.js/bindings/v8/V8HandleScope.cpp @@ -32,7 +32,7 @@ uintptr_t* HandleScope::CreateHandle(internal::Isolate* i_isolate, uintptr_t val auto* handleScope = isolate->globalInternals()->currentHandleScope(); TaggedPointer* newSlot = handleScope->m_buffer->createHandleFromExistingObject(TaggedPointer::fromRaw(value), isolate); // basically a reinterpret - return newSlot->asRawPtr(); + return newSlot->asRawPtrLocation(); } } // namespace v8 diff --git a/src/bun.js/bindings/v8/real_v8.h b/src/bun.js/bindings/v8/real_v8.h index b8d139c00343c..dec9fa2b1a97a 100644 --- a/src/bun.js/bindings/v8/real_v8.h +++ b/src/bun.js/bindings/v8/real_v8.h @@ -5,6 +5,11 @@ // files from Bun's V8 implementation, and it should only be included in source files (not // header files). +// Microsoft's C++ headers cause a compiler error if `private` has been redefined, like we do below. +// These are all the standard library headers included by V8. We include them here so that they +// are included while `private` is not redefined yet, and then when V8 includes them they will get +// skipped by include guards. + #include #include #include diff --git a/src/bun.js/bindings/v8/shim/FunctionTemplate.cpp b/src/bun.js/bindings/v8/shim/FunctionTemplate.cpp index e46a552d59d10..db04da9fc27aa 100644 --- a/src/bun.js/bindings/v8/shim/FunctionTemplate.cpp +++ b/src/bun.js/bindings/v8/shim/FunctionTemplate.cpp @@ -98,7 +98,7 @@ JSC::EncodedJSValue FunctionTemplate::functionCall(JSC::JSGlobalObject* globalOb functionTemplate->m_callback(info); - if (implicit_args.return_value.type() != TaggedPointer::Type::Smi && implicit_args.return_value.getPtr() == nullptr) { + if (implicit_args.return_value.isEmpty()) { // callback forgot to set a return value, so return undefined return JSValue::encode(JSC::jsUndefined()); } else { diff --git a/src/bun.js/bindings/v8/shim/Handle.cpp b/src/bun.js/bindings/v8/shim/Handle.cpp index d7b92cec04ba8..3aa33c7880c7e 100644 --- a/src/bun.js/bindings/v8/shim/Handle.cpp +++ b/src/bun.js/bindings/v8/shim/Handle.cpp @@ -41,7 +41,7 @@ Handle::Handle(const ObjectLayout* that) Handle& Handle::operator=(const Handle& that) { m_object = that.m_object; - if (that.m_toV8Object.type() == TaggedPointer::Type::Smi) { + if (that.m_toV8Object.tag() == TaggedPointer::Tag::Smi) { m_toV8Object = that.m_toV8Object; } else { m_toV8Object = &this->m_object; diff --git a/src/bun.js/bindings/v8/shim/Handle.h b/src/bun.js/bindings/v8/shim/Handle.h index eb15f14917bba..bd87f417f0dff 100644 --- a/src/bun.js/bindings/v8/shim/Handle.h +++ b/src/bun.js/bindings/v8/shim/Handle.h @@ -75,7 +75,7 @@ struct Handle { bool isCell() const { - if (m_toV8Object.type() == TaggedPointer::Type::Smi) { + if (m_toV8Object.tag() == TaggedPointer::Tag::Smi) { return false; } const Map* map_ptr = m_object.map(); diff --git a/src/bun.js/bindings/v8/shim/TaggedPointer.cpp b/src/bun.js/bindings/v8/shim/TaggedPointer.cpp new file mode 100644 index 0000000000000..eb0e786899cda --- /dev/null +++ b/src/bun.js/bindings/v8/shim/TaggedPointer.cpp @@ -0,0 +1,8 @@ +#include "TaggedPointer.h" +#include "../real_v8.h" + +static_assert(sizeof(v8::shim::TaggedPointer) == sizeof(real_v8::internal::Address), + "TaggedPointer has wrong size"); + +static_assert(alignof(v8::shim::TaggedPointer) == alignof(real_v8::internal::Address), + "TaggedPointer has wrong alignment"); diff --git a/src/bun.js/bindings/v8/shim/TaggedPointer.h b/src/bun.js/bindings/v8/shim/TaggedPointer.h index 46ea98701386c..a11cac1bd784d 100644 --- a/src/bun.js/bindings/v8/shim/TaggedPointer.h +++ b/src/bun.js/bindings/v8/shim/TaggedPointer.h @@ -15,12 +15,15 @@ struct TaggedPointer { uintptr_t m_value; public: - enum class Type : uint8_t { - Smi, - StrongPointer, - WeakPointer, + enum class Tag : uint8_t { + Smi = 0, + StrongPointer = 1, + WeakPointer = 3, }; + static constexpr uintptr_t TagMask = 0b11; + + // empty TaggedPointer() : TaggedPointer(nullptr) {}; TaggedPointer(const TaggedPointer&) = default; @@ -28,13 +31,14 @@ struct TaggedPointer { bool operator==(const TaggedPointer& other) const { return m_value == other.m_value; } TaggedPointer(void* ptr, bool weak = false) - : m_value(reinterpret_cast(ptr) | (weak ? 3 : 1)) + : m_value(reinterpret_cast(ptr) | static_cast(weak ? Tag::WeakPointer : Tag::StrongPointer)) { - RELEASE_ASSERT((reinterpret_cast(ptr) & 3) == 0); + // check original pointer was aligned + RELEASE_ASSERT((reinterpret_cast(ptr) & TagMask) == 0); } TaggedPointer(int32_t smi) - : m_value(static_cast(smi) << 32) + : m_value((static_cast(smi) << 32) | static_cast(Tag::Smi)) { } @@ -46,37 +50,34 @@ struct TaggedPointer { return tagged; } - // Get a pointer to where this TaggedPointer is located - uintptr_t* asRawPtr() + bool isEmpty() const + { + return *this == TaggedPointer(); + } + + // Get a pointer to where this TaggedPointer is located (use ->asRawPtrLocation() to reinterpret + // TaggedPointer* as uintptr_t*) + uintptr_t* asRawPtrLocation() { return &m_value; } - Type type() const + Tag tag() const { - switch (m_value & 3) { - case 0: - return Type::Smi; - case 1: - return Type::StrongPointer; - case 3: - return Type::WeakPointer; - default: - RELEASE_ASSERT_NOT_REACHED(); - } + return static_cast(m_value & TagMask); } template T* getPtr() const { - if (type() == Type::Smi) { + if (tag() == Tag::Smi) { return nullptr; } - return reinterpret_cast(m_value & ~3ull); + return reinterpret_cast(m_value & ~TagMask); } bool getSmi(int32_t& smi) const { - if (type() != Type::Smi) { + if (tag() != Tag::Smi) { return false; } smi = static_cast(m_value >> 32); @@ -85,7 +86,7 @@ struct TaggedPointer { int32_t getSmiUnchecked() const { - ASSERT(type() == Type::Smi); + ASSERT(tag() == Tag::Smi); return static_cast(m_value >> 32); } }; diff --git a/src/bun.js/bindings/v8/v8_api_internal.cpp b/src/bun.js/bindings/v8/v8_api_internal.cpp index 2153b11f3c201..860129c777a0b 100644 --- a/src/bun.js/bindings/v8/v8_api_internal.cpp +++ b/src/bun.js/bindings/v8/v8_api_internal.cpp @@ -17,7 +17,7 @@ uintptr_t* GlobalizeReference(internal::Isolate* i_isolate, uintptr_t address) auto* isolate = reinterpret_cast(i_isolate); auto* globalHandles = isolate->globalInternals()->globalHandles(); TaggedPointer* newSlot = globalHandles->createHandleFromExistingObject(TaggedPointer::fromRaw(address), isolate); - return newSlot->asRawPtr(); + return newSlot->asRawPtrLocation(); } void DisposeGlobal(uintptr_t* location)