From 24a94f031ddc004f702cab6e0577843fd883a6f5 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Thu, 21 Jul 2022 04:37:02 +0000 Subject: [PATCH 1/2] src: remove unowned usages of GetBackingStore This removes all usages of GetBackingStore without any entries in the `CODEOWNERS` file. For the most part this is a pretty straightforward review; except `SPREAD_BUFFER_ARG` and the changes to `CopyArrayBuffer`. See the linked issue for an explanation. Refs: https://github.com/nodejs/node/issues/32226 Refs: https://github.com/nodejs/node/pull/43921 --- src/aliased_buffer.h | 7 ++--- src/node_buffer.cc | 59 ++++++++++++++++++++++--------------- src/node_os.cc | 2 +- src/node_process_methods.cc | 6 ++-- src/node_worker.cc | 4 +-- src/node_zlib.cc | 3 +- src/util-inl.h | 3 +- src/util.h | 19 +++++------- 8 files changed, 53 insertions(+), 50 deletions(-) diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index 6dda51c14615cc..98ea2d31febce2 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -50,7 +50,7 @@ class AliasedBufferBase { // allocate v8 ArrayBuffer v8::Local ab = v8::ArrayBuffer::New( isolate_, size_in_bytes); - buffer_ = static_cast(ab->GetBackingStore()->Data()); + buffer_ = static_cast(ab->Data()); // allocate v8 TypedArray v8::Local js_array = V8T::New(ab, byte_offset_, count); @@ -119,8 +119,7 @@ class AliasedBufferBase { // be removed when we expand the snapshot support. DCHECK_EQ(count_, arr->Length()); DCHECK_EQ(byte_offset_, arr->ByteOffset()); - uint8_t* raw = - static_cast(arr->Buffer()->GetBackingStore()->Data()); + uint8_t* raw = static_cast(arr->Buffer()->Data()); buffer_ = reinterpret_cast(raw + byte_offset_); js_array_.Reset(isolate_, arr); index_ = nullptr; @@ -278,7 +277,7 @@ class AliasedBufferBase { isolate_, new_size_in_bytes); // allocate new native buffer - NativeT* new_buffer = static_cast(ab->GetBackingStore()->Data()); + NativeT* new_buffer = static_cast(ab->Data()); // copy old content memcpy(new_buffer, buffer_, old_size_in_bytes); diff --git a/src/node_buffer.cc b/src/node_buffer.cc index aec97f15e2c809..4f94b604a8d607 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -244,8 +244,7 @@ bool HasInstance(Local obj) { char* Data(Local val) { CHECK(val->IsArrayBufferView()); Local ui = val.As(); - return static_cast(ui->Buffer()->GetBackingStore()->Data()) + - ui->ByteOffset(); + return static_cast(ui->Buffer()->Data()) + ui->ByteOffset(); } @@ -1157,14 +1156,13 @@ static void EncodeInto(const FunctionCallbackInfo& args) { Local dest = args[1].As(); Local buf = dest->Buffer(); - char* write_result = - static_cast(buf->GetBackingStore()->Data()) + dest->ByteOffset(); + char* write_result = static_cast(buf->Data()) + dest->ByteOffset(); size_t dest_length = dest->ByteLength(); // results = [ read, written ] Local result_arr = args[2].As(); uint32_t* results = reinterpret_cast( - static_cast(result_arr->Buffer()->GetBackingStore()->Data()) + + static_cast(result_arr->Buffer()->Data()) + result_arr->ByteOffset()); int nchars; @@ -1228,6 +1226,27 @@ void DetachArrayBuffer(const FunctionCallbackInfo& args) { } } +namespace { + +std::pair DecomposeBufferToParts(Local buffer) { + void* pointer; + size_t byte_length; + if (buffer->IsArrayBuffer()) { + Local ab = buffer.As(); + pointer = ab->Data(); + byte_length = ab->ByteLength(); + } else if (buffer->IsSharedArrayBuffer()) { + Local ab = buffer.As(); + pointer = ab->Data(); + byte_length = ab->ByteLength(); + } else { + CHECK(false); // Caller must validate. + } + return {pointer, byte_length}; +} + +} // namespace + void CopyArrayBuffer(const FunctionCallbackInfo& args) { // args[0] == Destination ArrayBuffer // args[1] == Destination ArrayBuffer Offset @@ -1241,32 +1260,24 @@ void CopyArrayBuffer(const FunctionCallbackInfo& args) { CHECK(args[3]->IsUint32()); CHECK(args[4]->IsUint32()); - std::shared_ptr destination; - std::shared_ptr source; + void* destination; + size_t destination_byte_length; + std::tie(destination, destination_byte_length) = + DecomposeBufferToParts(args[0]); - if (args[0]->IsArrayBuffer()) { - destination = args[0].As()->GetBackingStore(); - } else if (args[0]->IsSharedArrayBuffer()) { - destination = args[0].As()->GetBackingStore(); - } - - if (args[2]->IsArrayBuffer()) { - source = args[2].As()->GetBackingStore(); - } else if (args[0]->IsSharedArrayBuffer()) { - source = args[2].As()->GetBackingStore(); - } + void* source; + size_t source_byte_length; + std::tie(source, source_byte_length) = DecomposeBufferToParts(args[2]); uint32_t destination_offset = args[1].As()->Value(); uint32_t source_offset = args[3].As()->Value(); size_t bytes_to_copy = args[4].As()->Value(); - CHECK_GE(destination->ByteLength() - destination_offset, bytes_to_copy); - CHECK_GE(source->ByteLength() - source_offset, bytes_to_copy); + CHECK_GE(destination_byte_length - destination_offset, bytes_to_copy); + CHECK_GE(source_byte_length - source_offset, bytes_to_copy); - uint8_t* dest = - static_cast(destination->Data()) + destination_offset; - uint8_t* src = - static_cast(source->Data()) + source_offset; + uint8_t* dest = static_cast(destination) + destination_offset; + uint8_t* src = static_cast(source) + source_offset; memcpy(dest, src, bytes_to_copy); } diff --git a/src/node_os.cc b/src/node_os.cc index 046a6106ccd0e5..eea6304158d29a 100644 --- a/src/node_os.cc +++ b/src/node_os.cc @@ -161,7 +161,7 @@ static void GetLoadAvg(const FunctionCallbackInfo& args) { Local array = args[0].As(); CHECK_EQ(array->Length(), 3); Local ab = array->Buffer(); - double* loadavg = static_cast(ab->GetBackingStore()->Data()); + double* loadavg = static_cast(ab->Data()); uv_loadavg(loadavg); } diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 350a7094baad59..26a0633b2f0493 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -116,7 +116,7 @@ static void CPUUsage(const FunctionCallbackInfo& args) { // Get the double array pointer from the Float64Array argument. Local ab = get_fields_array_buffer(args, 0, 2); - double* fields = static_cast(ab->GetBackingStore()->Data()); + double* fields = static_cast(ab->Data()); // Set the Float64Array elements to be user / system values in microseconds. fields[0] = MICROS_PER_SEC * rusage.ru_utime.tv_sec + rusage.ru_utime.tv_usec; @@ -189,7 +189,7 @@ static void MemoryUsage(const FunctionCallbackInfo& args) { // Get the double array pointer from the Float64Array argument. Local ab = get_fields_array_buffer(args, 0, 5); - double* fields = static_cast(ab->GetBackingStore()->Data()); + double* fields = static_cast(ab->Data()); size_t rss; int err = uv_resident_set_memory(&rss); @@ -311,7 +311,7 @@ static void ResourceUsage(const FunctionCallbackInfo& args) { return env->ThrowUVException(err, "uv_getrusage"); Local ab = get_fields_array_buffer(args, 0, 16); - double* fields = static_cast(ab->GetBackingStore()->Data()); + double* fields = static_cast(ab->Data()); fields[0] = MICROS_PER_SEC * rusage.ru_utime.tv_sec + rusage.ru_utime.tv_usec; fields[1] = MICROS_PER_SEC * rusage.ru_stime.tv_sec + rusage.ru_stime.tv_usec; diff --git a/src/node_worker.cc b/src/node_worker.cc index c70ccd6bd43edd..b4a10e00c0c9cf 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -710,9 +710,7 @@ void Worker::GetResourceLimits(const FunctionCallbackInfo& args) { Local Worker::GetResourceLimits(Isolate* isolate) const { Local ab = ArrayBuffer::New(isolate, sizeof(resource_limits_)); - memcpy(ab->GetBackingStore()->Data(), - resource_limits_, - sizeof(resource_limits_)); + memcpy(ab->Data(), resource_limits_, sizeof(resource_limits_)); return Float64Array::New(ab, 0, kTotalResourceLimitCount); } diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 5930ffd7a8ae8e..04c176095f8af2 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -605,8 +605,7 @@ class ZlibStream final : public CompressionStream { CHECK(args[4]->IsUint32Array()); Local array = args[4].As(); Local ab = array->Buffer(); - uint32_t* write_result = static_cast( - ab->GetBackingStore()->Data()); + uint32_t* write_result = static_cast(ab->Data()); CHECK(args[5]->IsFunction()); Local write_js_callback = args[5].As(); diff --git a/src/util-inl.h b/src/util-inl.h index 327893618525f8..caadba9dae2caa 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -537,8 +537,7 @@ void ArrayBufferViewContents::Read(v8::Local abv) { static_assert(sizeof(T) == 1, "Only supports one-byte data at the moment"); length_ = abv->ByteLength(); if (length_ > sizeof(stack_storage_) || abv->HasBuffer()) { - data_ = static_cast(abv->Buffer()->GetBackingStore()->Data()) + - abv->ByteOffset(); + data_ = static_cast(abv->Buffer()->Data()) + abv->ByteOffset(); } else { abv->CopyContents(stack_storage_, sizeof(stack_storage_)); data_ = stack_storage_; diff --git a/src/util.h b/src/util.h index a48071b093db97..cfb7d1e65fad08 100644 --- a/src/util.h +++ b/src/util.h @@ -535,17 +535,14 @@ class BufferValue : public MaybeStackBuffer { inline std::string ToString() const { return std::string(out(), length()); } }; -#define SPREAD_BUFFER_ARG(val, name) \ - CHECK((val)->IsArrayBufferView()); \ - v8::Local name = (val).As(); \ - std::shared_ptr name##_bs = \ - name->Buffer()->GetBackingStore(); \ - const size_t name##_offset = name->ByteOffset(); \ - const size_t name##_length = name->ByteLength(); \ - char* const name##_data = \ - static_cast(name##_bs->Data()) + name##_offset; \ - if (name##_length > 0) \ - CHECK_NE(name##_data, nullptr); +#define SPREAD_BUFFER_ARG(val, name) \ + CHECK((val)->IsArrayBufferView()); \ + v8::Local name = (val).As(); \ + const size_t name##_offset = name->ByteOffset(); \ + const size_t name##_length = name->ByteLength(); \ + char* const name##_data = \ + static_cast(name->Buffer()->Data()) + name##_offset; \ + if (name##_length > 0) CHECK_NE(name##_data, nullptr); // Use this when a variable or parameter is unused in order to explicitly // silence a compiler warning about that. From 63af09352d40739c556dcc21832bbf2a2586b8c5 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Mon, 1 Aug 2022 05:48:32 +0000 Subject: [PATCH 2/2] fixup! src: remove unowned usages of GetBackingStore --- src/node_buffer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 4f94b604a8d607..819e24a35298c8 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -1240,7 +1240,7 @@ std::pair DecomposeBufferToParts(Local buffer) { pointer = ab->Data(); byte_length = ab->ByteLength(); } else { - CHECK(false); // Caller must validate. + UNREACHABLE(); // Caller must validate. } return {pointer, byte_length}; }