From 5bb985d3312191d67490f9d826fd13cdbda610e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Wed, 25 Jul 2018 20:03:35 +0200 Subject: [PATCH 01/11] deps: cherry-pick 0dd3390 from upstream V8 Original commit message: Reland "[builtins] Add %IsTraceCategoryEnabled and %Trace builtins" This is a reland of 8d4572a22b5d2fa0547195bcc40baa18b7565386 Original change's description: > [builtins] Add %IsTraceCategoryEnabled and %Trace builtins > > Adds the builtin Trace and IsTraceCategoryEnabled functions > exposed via extra bindings. These are intended to use by > embedders to allow basic trace event support from JavaScript. > > ```js > isTraceCategoryEnabled('v8.some-category') > > trace('e'.charCodeAt(0), 'v8.some-category', > 'Foo', 0, { abc: 'xyz'}) > ``` > > Bug: v8:7851 > Change-Id: I7bfb9bb059efdf87d92a56a0aae326650730c250 > Reviewed-on: https://chromium-review.googlesource.com/1103294 > Commit-Queue: Yang Guo > Reviewed-by: Yang Guo > Reviewed-by: Fadi Meawad > Reviewed-by: Camillo Bruni > Reviewed-by: Benedikt Meurer > Cr-Commit-Position: refs/heads/master@{#54121} TBR=cbruni@chromium.org Bug: v8:7851 Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8 Reviewed-on: https://chromium-review.googlesource.com/1137071 Commit-Queue: Yang Guo Reviewed-by: Yang Guo Reviewed-by: Camillo Bruni Reviewed-by: Benedikt Meurer Cr-Commit-Position: refs/heads/master@{#54532} Refs: https://github.com/v8/v8/commit/0dd33901a16c7c64290b7e7ddf13945b773c5d79 PR-URL: https://github.com/nodejs/node/pull/21983 Reviewed-By: Refael Ackermann Reviewed-By: Gus Caplan Reviewed-By: Ujjwal Sharma Reviewed-By: Matteo Collina --- AUTHORS | 1 + BUILD.gn | 1 + src/bootstrapper.cc | 9 ++ src/builtins/builtins-definitions.h | 6 +- src/builtins/builtins-trace.cc | 191 ++++++++++++++++++++++++++++ src/debug/debug-evaluate.cc | 3 + src/messages.h | 9 +- test/cctest/test-trace-event.cc | 134 ++++++++++++++++++- 8 files changed, 351 insertions(+), 3 deletions(-) create mode 100644 deps/v8/src/builtins/builtins-trace.cc diff --git a/AUTHORS b/AUTHORS index b9391a0d28..3873f0ca68 100644 --- a/AUTHORS +++ b/AUTHORS @@ -87,6 +87,7 @@ Jan de Mooij Jan Krems Jay Freeman James Pike +James M Snell Jianghua Yang Joel Stanley Johan Bergström diff --git a/BUILD.gn b/BUILD.gn index 1e31acb277..4c8c8acd59 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1588,6 +1588,7 @@ v8_source_set("v8_base") { "src/builtins/builtins-sharedarraybuffer.cc", "src/builtins/builtins-string.cc", "src/builtins/builtins-symbol.cc", + "src/builtins/builtins-trace.cc", "src/builtins/builtins-typed-array.cc", "src/builtins/builtins-utils.h", "src/builtins/builtins.cc", diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 6723f3d5d4..656650cd64 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -5077,6 +5077,15 @@ bool Genesis::InstallExtraNatives() { Handle extras_binding = factory()->NewJSObject(isolate()->object_function()); + + // binding.isTraceCategoryEnabled(category) + SimpleInstallFunction(isolate(), extras_binding, "isTraceCategoryEnabled", + Builtins::kIsTraceCategoryEnabled, 1, true); + + // binding.trace(phase, category, name, id, data) + SimpleInstallFunction(isolate(), extras_binding, "trace", Builtins::kTrace, 5, + true); + native_context()->set_extras_binding_object(*extras_binding); for (int i = ExtraNatives::GetDebuggerCount(); diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 46b02d88d8..2d7c780c70 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -1308,7 +1308,11 @@ namespace internal { ASM(CallApiGetter) \ ASM(DoubleToI) \ TFC(GetProperty, GetProperty, 1) \ - ASM(MathPowInternal) + ASM(MathPowInternal) \ + \ + /* Trace */ \ + CPP(IsTraceCategoryEnabled) \ + CPP(Trace) #ifdef V8_INTL_SUPPORT #define BUILTIN_LIST(CPP, API, TFJ, TFC, TFS, TFH, ASM) \ diff --git a/src/builtins/builtins-trace.cc b/src/builtins/builtins-trace.cc new file mode 100644 index 0000000000..cd0f5a77d0 --- /dev/null +++ b/src/builtins/builtins-trace.cc @@ -0,0 +1,191 @@ +// Copyright 2018 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "src/api.h" +#include "src/builtins/builtins-utils.h" +#include "src/builtins/builtins.h" +#include "src/counters.h" +#include "src/json-stringifier.h" +#include "src/objects-inl.h" + +namespace v8 { +namespace internal { + +namespace { + +using v8::tracing::TracedValue; + +#define MAX_STACK_LENGTH 100 + +class MaybeUtf8 { + public: + explicit MaybeUtf8(Isolate* isolate, Handle string) : buf_(data_) { + string = String::Flatten(isolate, string); + int len; + if (string->IsOneByteRepresentation()) { + // Technically this allows unescaped latin1 characters but the trace + // events mechanism currently does the same and the current consuming + // tools are tolerant of it. A more correct approach here would be to + // escape non-ascii characters but this is easier and faster. + len = string->length(); + AllocateSufficientSpace(len); + if (len > 0) { + // Why copy? Well, the trace event mechanism requires null-terminated + // strings, the bytes we get from SeqOneByteString are not. buf_ is + // guaranteed to be null terminated. + memcpy(buf_, Handle::cast(string)->GetChars(), len); + } + } else { + Local local = Utils::ToLocal(string); + len = local->Utf8Length(); + AllocateSufficientSpace(len); + if (len > 0) { + local->WriteUtf8(reinterpret_cast(buf_)); + } + } + buf_[len] = 0; + } + const char* operator*() const { return reinterpret_cast(buf_); } + + private: + void AllocateSufficientSpace(int len) { + if (len + 1 > MAX_STACK_LENGTH) { + allocated_.reset(new uint8_t[len + 1]); + buf_ = allocated_.get(); + } + } + + // In the most common cases, the buffer here will be stack allocated. + // A heap allocation will only occur if the data is more than MAX_STACK_LENGTH + // Given that this is used primarily for trace event categories and names, + // the MAX_STACK_LENGTH should be more than enough. + uint8_t* buf_; + uint8_t data_[MAX_STACK_LENGTH]; + std::unique_ptr allocated_; +}; + +class JsonTraceValue : public ConvertableToTraceFormat { + public: + explicit JsonTraceValue(Isolate* isolate, Handle object) { + // object is a JSON string serialized using JSON.stringify() from within + // the BUILTIN(Trace) method. This may (likely) contain UTF8 values so + // to grab the appropriate buffer data we have to serialize it out. We + // hold on to the bits until the AppendAsTraceFormat method is called. + MaybeUtf8 data(isolate, object); + data_ = *data; + } + + void AppendAsTraceFormat(std::string* out) const override { *out += data_; } + + private: + std::string data_; +}; + +const uint8_t* GetCategoryGroupEnabled(Isolate* isolate, + Handle string) { + MaybeUtf8 category(isolate, string); + return TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(*category); +} + +#undef MAX_STACK_LENGTH + +} // namespace + +// Builins::kIsTraceCategoryEnabled(category) : bool +BUILTIN(IsTraceCategoryEnabled) { + HandleScope scope(isolate); + Handle category = args.atOrUndefined(isolate, 1); + if (!category->IsString()) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kTraceEventCategoryError)); + } + return isolate->heap()->ToBoolean( + *GetCategoryGroupEnabled(isolate, Handle::cast(category))); +} + +// Builtins::kTrace(phase, category, name, id, data) : bool +BUILTIN(Trace) { + HandleScope handle_scope(isolate); + + Handle phase_arg = args.atOrUndefined(isolate, 1); + Handle category = args.atOrUndefined(isolate, 2); + Handle name_arg = args.atOrUndefined(isolate, 3); + Handle id_arg = args.atOrUndefined(isolate, 4); + Handle data_arg = args.atOrUndefined(isolate, 5); + + const uint8_t* category_group_enabled = + GetCategoryGroupEnabled(isolate, Handle::cast(category)); + + // Exit early if the category group is not enabled. + if (!*category_group_enabled) { + return ReadOnlyRoots(isolate).false_value(); + } + + if (!phase_arg->IsNumber()) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kTraceEventPhaseError)); + } + if (!category->IsString()) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kTraceEventCategoryError)); + } + if (!name_arg->IsString()) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kTraceEventNameError)); + } + + uint32_t flags = TRACE_EVENT_FLAG_COPY; + int32_t id = 0; + if (!id_arg->IsNullOrUndefined(isolate)) { + if (!id_arg->IsNumber()) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kTraceEventIDError)); + } + flags |= TRACE_EVENT_FLAG_HAS_ID; + id = DoubleToInt32(id_arg->Number()); + } + + Handle name_str = Handle::cast(name_arg); + if (name_str->length() == 0) { + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, NewTypeError(MessageTemplate::kTraceEventNameLengthError)); + } + MaybeUtf8 name(isolate, name_str); + + // We support passing one additional trace event argument with the + // name "data". Any JSON serializable value may be passed. + static const char* arg_name = "data"; + int32_t num_args = 0; + uint8_t arg_type; + uint64_t arg_value; + + if (!data_arg->IsUndefined(isolate)) { + // Serializes the data argument as a JSON string, which is then + // copied into an object. This eliminates duplicated code but + // could have perf costs. It is also subject to all the same + // limitations as JSON.stringify() as it relates to circular + // references and value limitations (e.g. BigInt is not supported). + JsonStringifier stringifier(isolate); + Handle result; + ASSIGN_RETURN_FAILURE_ON_EXCEPTION( + isolate, result, + stringifier.Stringify(data_arg, isolate->factory()->undefined_value(), + isolate->factory()->undefined_value())); + std::unique_ptr traced_value; + traced_value.reset( + new JsonTraceValue(isolate, Handle::cast(result))); + tracing::SetTraceValue(std::move(traced_value), &arg_type, &arg_value); + num_args++; + } + + TRACE_EVENT_API_ADD_TRACE_EVENT( + static_cast(DoubleToInt32(phase_arg->Number())), + category_group_enabled, *name, tracing::kGlobalScope, id, tracing::kNoId, + num_args, &arg_name, &arg_type, &arg_value, flags); + + return ReadOnlyRoots(isolate).true_value(); +} + +} // namespace internal +} // namespace v8 diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc index 0dd2303772..d263fa45a9 100644 --- a/src/debug/debug-evaluate.cc +++ b/src/debug/debug-evaluate.cc @@ -567,6 +567,9 @@ DebugInfo::SideEffectState BuiltinGetSideEffectState(Builtins::Name id) { case Builtins::kArrayMap: case Builtins::kArrayReduce: case Builtins::kArrayReduceRight: + // Trace builtins. + case Builtins::kIsTraceCategoryEnabled: + case Builtins::kTrace: // TypedArray builtins. case Builtins::kTypedArrayConstructor: case Builtins::kTypedArrayPrototypeBuffer: diff --git a/src/messages.h b/src/messages.h index 1d1a07d7b6..68078bb373 100644 --- a/src/messages.h +++ b/src/messages.h @@ -757,7 +757,14 @@ class ErrorUtils : public AllStatic { T(DataCloneDeserializationError, "Unable to deserialize cloned data.") \ T(DataCloneDeserializationVersionError, \ "Unable to deserialize cloned data due to invalid or unsupported " \ - "version.") + "version.") \ + /* Builtins-Trace Errors */ \ + T(TraceEventCategoryError, "Trace event category must be a string.") \ + T(TraceEventNameError, "Trace event name must be a string.") \ + T(TraceEventNameLengthError, \ + "Trace event name must not be an empty string.") \ + T(TraceEventPhaseError, "Trace event phase must be a number.") \ + T(TraceEventIDError, "Trace event id must be a number.") class MessageTemplate { public: diff --git a/test/cctest/test-trace-event.cc b/test/cctest/test-trace-event.cc index 7b736b907d..47545af37f 100644 --- a/test/cctest/test-trace-event.cc +++ b/test/cctest/test-trace-event.cc @@ -75,7 +75,7 @@ class MockTracingController : public v8::TracingController { const char* name, uint64_t handle) override {} const uint8_t* GetCategoryGroupEnabled(const char* name) override { - if (strcmp(name, "v8-cat")) { + if (strncmp(name, "v8-cat", 6)) { static uint8_t no = 0; return &no; } else { @@ -282,3 +282,135 @@ TEST(TestEventWithTimestamp) { CHECK_EQ(20683, GET_TRACE_OBJECT(3)->timestamp); CHECK_EQ(32832, GET_TRACE_OBJECT(4)->timestamp); } + +TEST(BuiltinsIsTraceCategoryEnabled) { + CcTest::InitializeVM(); + MockTracingPlatform platform; + + v8::Isolate* isolate = CcTest::isolate(); + v8::HandleScope handle_scope(isolate); + LocalContext env; + + v8::Local binding = env->GetExtrasBindingObject(); + CHECK(!binding.IsEmpty()); + + auto undefined = v8::Undefined(isolate); + auto isTraceCategoryEnabled = + binding->Get(env.local(), v8_str("isTraceCategoryEnabled")) + .ToLocalChecked() + .As(); + + { + // Test with an enabled category + v8::Local argv[] = {v8_str("v8-cat")}; + auto result = isTraceCategoryEnabled->Call(env.local(), undefined, 1, argv) + .ToLocalChecked() + .As(); + + CHECK(result->BooleanValue()); + } + + { + // Test with a disabled category + v8::Local argv[] = {v8_str("cat")}; + auto result = isTraceCategoryEnabled->Call(env.local(), undefined, 1, argv) + .ToLocalChecked() + .As(); + + CHECK(!result->BooleanValue()); + } + + { + // Test with an enabled utf8 category + v8::Local argv[] = {v8_str("v8-cat\u20ac")}; + auto result = isTraceCategoryEnabled->Call(env.local(), undefined, 1, argv) + .ToLocalChecked() + .As(); + + CHECK(result->BooleanValue()); + } +} + +TEST(BuiltinsTrace) { + CcTest::InitializeVM(); + MockTracingPlatform platform; + + v8::Isolate* isolate = CcTest::isolate(); + v8::HandleScope handle_scope(isolate); + LocalContext env; + + v8::Local binding = env->GetExtrasBindingObject(); + CHECK(!binding.IsEmpty()); + + auto undefined = v8::Undefined(isolate); + auto trace = binding->Get(env.local(), v8_str("trace")) + .ToLocalChecked() + .As(); + + // Test with disabled category + { + v8::Local category = v8_str("cat"); + v8::Local name = v8_str("name"); + v8::Local argv[] = { + v8::Integer::New(isolate, 'b'), // phase + category, name, v8::Integer::New(isolate, 0), // id + undefined // data + }; + auto result = trace->Call(env.local(), undefined, 5, argv) + .ToLocalChecked() + .As(); + + CHECK(!result->BooleanValue()); + CHECK_EQ(0, GET_TRACE_OBJECTS_LIST->size()); + } + + // Test with enabled category + { + v8::Local category = v8_str("v8-cat"); + v8::Local name = v8_str("name"); + v8::Local context = isolate->GetCurrentContext(); + v8::Local data = v8::Object::New(isolate); + data->Set(context, v8_str("foo"), v8_str("bar")).FromJust(); + v8::Local argv[] = { + v8::Integer::New(isolate, 'b'), // phase + category, name, v8::Integer::New(isolate, 123), // id + data // data arg + }; + auto result = trace->Call(env.local(), undefined, 5, argv) + .ToLocalChecked() + .As(); + + CHECK(result->BooleanValue()); + CHECK_EQ(1, GET_TRACE_OBJECTS_LIST->size()); + + CHECK_EQ(123, GET_TRACE_OBJECT(0)->id); + CHECK_EQ('b', GET_TRACE_OBJECT(0)->phase); + CHECK_EQ("name", GET_TRACE_OBJECT(0)->name); + CHECK_EQ(1, GET_TRACE_OBJECT(0)->num_args); + } + + // Test with enabled utf8 category + { + v8::Local category = v8_str("v8-cat\u20ac"); + v8::Local name = v8_str("name\u20ac"); + v8::Local context = isolate->GetCurrentContext(); + v8::Local data = v8::Object::New(isolate); + data->Set(context, v8_str("foo"), v8_str("bar")).FromJust(); + v8::Local argv[] = { + v8::Integer::New(isolate, 'b'), // phase + category, name, v8::Integer::New(isolate, 123), // id + data // data arg + }; + auto result = trace->Call(env.local(), undefined, 5, argv) + .ToLocalChecked() + .As(); + + CHECK(result->BooleanValue()); + CHECK_EQ(2, GET_TRACE_OBJECTS_LIST->size()); + + CHECK_EQ(123, GET_TRACE_OBJECT(1)->id); + CHECK_EQ('b', GET_TRACE_OBJECT(1)->phase); + CHECK_EQ("name\u20ac", GET_TRACE_OBJECT(1)->name); + CHECK_EQ(1, GET_TRACE_OBJECT(1)->num_args); + } +} -- 2.17.0 From 127e7035ce70e70bdb62969f965d6941156c042c Mon Sep 17 00:00:00 2001 From: Matheus Marchini Date: Wed, 1 Aug 2018 15:09:51 -0300 Subject: [PATCH 02/11] deps: cherry-pick 09bca09 from upstream V8 Original commit message: [postmortem] add ScopeInfo and Context types The metadata introduced in this patch will be useful for postmortem tools to inspect Contexts and ScopeInfos (see https://github.com/nodejs/llnode/issues/211). R=bmeurer@google.com, yangguo@google.com Change-Id: I927fcab4014d128bd782046c1ecb9ee045723e95 Reviewed-on: https://chromium-review.googlesource.com/1153858 Reviewed-by: Yang Guo Commit-Queue: Yang Guo Cr-Commit-Position: refs/heads/master@{#54768} Refs: https://github.com/v8/v8/commit/09bca095e38d6e4770ae48e174f59d33c PR-URL: https://github.com/nodejs/node/pull/22068 Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- BUILD.gn | 1 + tools/gen-postmortem-metadata.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/BUILD.gn b/BUILD.gn index 4c8c8acd59..443694d880 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -852,6 +852,7 @@ action("postmortem-metadata") { "src/objects/js-regexp-string-iterator.h", "src/objects/map.h", "src/objects/map-inl.h", + "src/objects/scope-info.h", "src/objects/script.h", "src/objects/script-inl.h", "src/objects/shared-function-info.h", diff --git a/tools/gen-postmortem-metadata.py b/tools/gen-postmortem-metadata.py index d8ef7b4a15..8191c8152f 100644 --- a/tools/gen-postmortem-metadata.py +++ b/tools/gen-postmortem-metadata.py @@ -58,6 +58,9 @@ consts_misc = [ { 'name': 'APIObjectType', 'value': 'JS_API_OBJECT_TYPE' }, { 'name': 'SpecialAPIObjectType', 'value': 'JS_SPECIAL_API_OBJECT_TYPE' }, + { 'name': 'FirstContextType', 'value': 'FIRST_CONTEXT_TYPE' }, + { 'name': 'LastContextType', 'value': 'LAST_CONTEXT_TYPE' }, + { 'name': 'IsNotStringMask', 'value': 'kIsNotStringMask' }, { 'name': 'StringTag', 'value': 'kStringTag' }, @@ -282,7 +285,7 @@ extras_accessors = [ expected_classes = [ 'ConsString', 'FixedArray', 'HeapNumber', 'JSArray', 'JSFunction', 'JSObject', 'JSRegExp', 'JSValue', 'Map', 'Oddball', 'Script', - 'SeqOneByteString', 'SharedFunctionInfo' + 'SeqOneByteString', 'SharedFunctionInfo', 'ScopeInfo' ]; -- 2.17.0 From 8dc159658c97ff04dfc08ff5bfcd1a1a17b98430 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Sun, 19 Aug 2018 21:53:40 +0200 Subject: [PATCH 03/11] deps: cherry-pick c608122 from upstream V8 Original commit message: [api][keys] Allow skipping indices for Proxies with GetPropertyNames Bug: v8:7942 Change-Id: I7b3740b04cbcaa56dc809150900ab8d821b054ce Reviewed-on: https://chromium-review.googlesource.com/1156544 Reviewed-by: Toon Verwaest Commit-Queue: Camillo Bruni Cr-Commit-Position: refs/heads/master@{#54821} Refs: https://github.com/v8/v8/commit/c608122b85238397a43910246f5ff218eb43fb24 PR-URL: https://github.com/nodejs/node/pull/21983 Reviewed-By: Refael Ackermann Reviewed-By: Gus Caplan Reviewed-By: Ujjwal Sharma Reviewed-By: Matteo Collina --- src/keys.cc | 48 +++++++++++------- src/keys.h | 14 ++++-- src/runtime/runtime-forin.cc | 3 +- test/cctest/test-api.cc | 97 +++++++++++++++++++++++++++++++++++- 4 files changed, 136 insertions(+), 26 deletions(-) diff --git a/src/keys.cc b/src/keys.cc index 8ecbe0a1d7..689f4ac3df 100644 --- a/src/keys.cc +++ b/src/keys.cc @@ -38,10 +38,10 @@ static bool ContainsOnlyValidKeys(Handle array) { // static MaybeHandle KeyAccumulator::GetKeys( Handle object, KeyCollectionMode mode, PropertyFilter filter, - GetKeysConversion keys_conversion, bool is_for_in) { + GetKeysConversion keys_conversion, bool is_for_in, bool skip_indices) { Isolate* isolate = object->GetIsolate(); - FastKeyAccumulator accumulator(isolate, object, mode, filter); - accumulator.set_is_for_in(is_for_in); + FastKeyAccumulator accumulator(isolate, object, mode, filter, is_for_in, + skip_indices); return accumulator.GetKeys(keys_conversion); } @@ -355,7 +355,8 @@ Handle GetFastEnumPropertyKeys(Isolate* isolate, template MaybeHandle GetOwnKeysWithElements(Isolate* isolate, Handle object, - GetKeysConversion convert) { + GetKeysConversion convert, + bool skip_indices) { Handle keys; ElementsAccessor* accessor = object->GetElementsAccessor(); if (fast_properties) { @@ -364,8 +365,14 @@ MaybeHandle GetOwnKeysWithElements(Isolate* isolate, // TODO(cbruni): preallocate big enough array to also hold elements. keys = KeyAccumulator::GetOwnEnumPropertyKeys(isolate, object); } - MaybeHandle result = - accessor->PrependElementIndices(object, keys, convert, ONLY_ENUMERABLE); + + MaybeHandle result; + if (skip_indices) { + result = keys; + } else { + result = + accessor->PrependElementIndices(object, keys, convert, ONLY_ENUMERABLE); + } if (FLAG_trace_for_in_enumerate) { PrintF("| strings=%d symbols=0 elements=%u || prototypes>=1 ||\n", @@ -403,7 +410,8 @@ MaybeHandle FastKeyAccumulator::GetKeysFast( // Do not try to use the enum-cache for dict-mode objects. if (map->is_dictionary_map()) { - return GetOwnKeysWithElements(isolate_, object, keys_conversion); + return GetOwnKeysWithElements(isolate_, object, keys_conversion, + skip_indices_); } int enum_length = receiver_->map()->EnumLength(); if (enum_length == kInvalidEnumCacheSentinel) { @@ -421,7 +429,8 @@ MaybeHandle FastKeyAccumulator::GetKeysFast( } // The properties-only case failed because there were probably elements on the // receiver. - return GetOwnKeysWithElements(isolate_, object, keys_conversion); + return GetOwnKeysWithElements(isolate_, object, keys_conversion, + skip_indices_); } MaybeHandle @@ -450,6 +459,7 @@ MaybeHandle FastKeyAccumulator::GetKeysSlow( GetKeysConversion keys_conversion) { KeyAccumulator accumulator(isolate_, mode_, filter_); accumulator.set_is_for_in(is_for_in_); + accumulator.set_skip_indices(skip_indices_); accumulator.set_last_non_empty_prototype(last_non_empty_prototype_); MAYBE_RETURN(accumulator.CollectKeys(receiver_, receiver_), @@ -699,13 +709,15 @@ Maybe KeyAccumulator::CollectOwnPropertyNames(Handle receiver, Maybe KeyAccumulator::CollectAccessCheckInterceptorKeys( Handle access_check_info, Handle receiver, Handle object) { - MAYBE_RETURN((CollectInterceptorKeysInternal( - receiver, object, - handle(InterceptorInfo::cast( - access_check_info->indexed_interceptor()), - isolate_), - this, kIndexed)), - Nothing()); + if (!skip_indices_) { + MAYBE_RETURN((CollectInterceptorKeysInternal( + receiver, object, + handle(InterceptorInfo::cast( + access_check_info->indexed_interceptor()), + isolate_), + this, kIndexed)), + Nothing()); + } MAYBE_RETURN( (CollectInterceptorKeysInternal( receiver, object, @@ -942,9 +954,9 @@ Maybe KeyAccumulator::CollectOwnJSProxyTargetKeys( Handle keys; ASSIGN_RETURN_ON_EXCEPTION_VALUE( isolate_, keys, - KeyAccumulator::GetKeys(target, KeyCollectionMode::kOwnOnly, - ALL_PROPERTIES, - GetKeysConversion::kConvertToString, is_for_in_), + KeyAccumulator::GetKeys( + target, KeyCollectionMode::kOwnOnly, ALL_PROPERTIES, + GetKeysConversion::kConvertToString, is_for_in_, skip_indices_), Nothing()); Maybe result = AddKeysFromJSProxy(proxy, keys); return result; diff --git a/src/keys.h b/src/keys.h index 649d6a9599..5abbaac5cd 100644 --- a/src/keys.h +++ b/src/keys.h @@ -40,7 +40,7 @@ class KeyAccumulator final BASE_EMBEDDED { static MaybeHandle GetKeys( Handle object, KeyCollectionMode mode, PropertyFilter filter, GetKeysConversion keys_conversion = GetKeysConversion::kKeepNumbers, - bool is_for_in = false); + bool is_for_in = false, bool skip_indices = false); Handle GetKeys( GetKeysConversion convert = GetKeysConversion::kKeepNumbers); @@ -128,14 +128,19 @@ class KeyAccumulator final BASE_EMBEDDED { class FastKeyAccumulator { public: FastKeyAccumulator(Isolate* isolate, Handle receiver, - KeyCollectionMode mode, PropertyFilter filter) - : isolate_(isolate), receiver_(receiver), mode_(mode), filter_(filter) { + KeyCollectionMode mode, PropertyFilter filter, + bool is_for_in = false, bool skip_indices = false) + : isolate_(isolate), + receiver_(receiver), + mode_(mode), + filter_(filter), + is_for_in_(is_for_in), + skip_indices_(skip_indices) { Prepare(); } bool is_receiver_simple_enum() { return is_receiver_simple_enum_; } bool has_empty_prototype() { return has_empty_prototype_; } - void set_is_for_in(bool value) { is_for_in_ = value; } MaybeHandle GetKeys( GetKeysConversion convert = GetKeysConversion::kKeepNumbers); @@ -153,6 +158,7 @@ class FastKeyAccumulator { KeyCollectionMode mode_; PropertyFilter filter_; bool is_for_in_ = false; + bool skip_indices_ = false; bool is_receiver_simple_enum_ = false; bool has_empty_prototype_ = false; diff --git a/src/runtime/runtime-forin.cc b/src/runtime/runtime-forin.cc index 5df16faf46..ed1e226060 100644 --- a/src/runtime/runtime-forin.cc +++ b/src/runtime/runtime-forin.cc @@ -26,8 +26,7 @@ MaybeHandle Enumerate(Isolate* isolate, JSObject::MakePrototypesFast(receiver, kStartAtReceiver, isolate); FastKeyAccumulator accumulator(isolate, receiver, KeyCollectionMode::kIncludePrototypes, - ENUMERABLE_STRINGS); - accumulator.set_is_for_in(true); + ENUMERABLE_STRINGS, true); // Test if we have an enum cache for {receiver}. if (!accumulator.is_receiver_simple_enum()) { Handle keys; diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index d5d1ae101b..a018e12853 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -15360,14 +15360,107 @@ THREADED_TEST(PropertyEnumeration2) { } } -THREADED_TEST(PropertyNames) { +THREADED_TEST(GetPropertyNames) { LocalContext context; v8::Isolate* isolate = context->GetIsolate(); v8::HandleScope scope(isolate); v8::Local result = CompileRun( "var result = {0: 0, 1: 1, a: 2, b: 3};" "result[Symbol('symbol')] = true;" - "result.__proto__ = {2: 4, 3: 5, c: 6, d: 7};" + "result.__proto__ = {__proto__:null, 2: 4, 3: 5, c: 6, d: 7};" + "result;"); + v8::Local object = result.As(); + v8::PropertyFilter default_filter = + static_cast(v8::ONLY_ENUMERABLE | v8::SKIP_SYMBOLS); + v8::PropertyFilter include_symbols_filter = v8::ONLY_ENUMERABLE; + + v8::Local properties = + object->GetPropertyNames(context.local()).ToLocalChecked(); + const char* expected_properties1[] = {"0", "1", "a", "b", "2", "3", "c", "d"}; + CheckStringArray(isolate, properties, 8, expected_properties1); + + properties = + object + ->GetPropertyNames(context.local(), + v8::KeyCollectionMode::kIncludePrototypes, + default_filter, v8::IndexFilter::kIncludeIndices) + .ToLocalChecked(); + CheckStringArray(isolate, properties, 8, expected_properties1); + + properties = object + ->GetPropertyNames(context.local(), + v8::KeyCollectionMode::kIncludePrototypes, + include_symbols_filter, + v8::IndexFilter::kIncludeIndices) + .ToLocalChecked(); + const char* expected_properties1_1[] = {"0", "1", "a", "b", nullptr, + "2", "3", "c", "d"}; + CheckStringArray(isolate, properties, 9, expected_properties1_1); + CheckIsSymbolAt(isolate, properties, 4, "symbol"); + + properties = + object + ->GetPropertyNames(context.local(), + v8::KeyCollectionMode::kIncludePrototypes, + default_filter, v8::IndexFilter::kSkipIndices) + .ToLocalChecked(); + const char* expected_properties2[] = {"a", "b", "c", "d"}; + CheckStringArray(isolate, properties, 4, expected_properties2); + + properties = object + ->GetPropertyNames(context.local(), + v8::KeyCollectionMode::kIncludePrototypes, + include_symbols_filter, + v8::IndexFilter::kSkipIndices) + .ToLocalChecked(); + const char* expected_properties2_1[] = {"a", "b", nullptr, "c", "d"}; + CheckStringArray(isolate, properties, 5, expected_properties2_1); + CheckIsSymbolAt(isolate, properties, 2, "symbol"); + + properties = + object + ->GetPropertyNames(context.local(), v8::KeyCollectionMode::kOwnOnly, + default_filter, v8::IndexFilter::kIncludeIndices) + .ToLocalChecked(); + const char* expected_properties3[] = {"0", "1", "a", "b"}; + CheckStringArray(isolate, properties, 4, expected_properties3); + + properties = object + ->GetPropertyNames( + context.local(), v8::KeyCollectionMode::kOwnOnly, + include_symbols_filter, v8::IndexFilter::kIncludeIndices) + .ToLocalChecked(); + const char* expected_properties3_1[] = {"0", "1", "a", "b", nullptr}; + CheckStringArray(isolate, properties, 5, expected_properties3_1); + CheckIsSymbolAt(isolate, properties, 4, "symbol"); + + properties = + object + ->GetPropertyNames(context.local(), v8::KeyCollectionMode::kOwnOnly, + default_filter, v8::IndexFilter::kSkipIndices) + .ToLocalChecked(); + const char* expected_properties4[] = {"a", "b"}; + CheckStringArray(isolate, properties, 2, expected_properties4); + + properties = object + ->GetPropertyNames( + context.local(), v8::KeyCollectionMode::kOwnOnly, + include_symbols_filter, v8::IndexFilter::kSkipIndices) + .ToLocalChecked(); + const char* expected_properties4_1[] = {"a", "b", nullptr}; + CheckStringArray(isolate, properties, 3, expected_properties4_1); + CheckIsSymbolAt(isolate, properties, 2, "symbol"); +} + +THREADED_TEST(ProxyGetPropertyNames) { + LocalContext context; + v8::Isolate* isolate = context->GetIsolate(); + v8::HandleScope scope(isolate); + v8::Local result = CompileRun( + "var target = {0: 0, 1: 1, a: 2, b: 3};" + "target[Symbol('symbol')] = true;" + "target.__proto__ = {__proto__:null, 2: 4, 3: 5, c: 6, d: 7};" + "var result = new Proxy(target, {});" "result;"); v8::Local object = result.As(); v8::PropertyFilter default_filter = -- 2.17.0 From 56d7411be3c70a3e99cbc60aadee06bbc99a233e Mon Sep 17 00:00:00 2001 From: Camillo Bruni Date: Sat, 18 Aug 2018 13:41:51 -0400 Subject: [PATCH 04/11] deps: cherry-pick e1a7699 from upstream V8 Original commit message: [api][runtime] Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration - Explicitly allows construction of {Named,Indexed}PropertyHandlerConfiguration with all the members filled. Bug: v8:7612 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded Reviewed-on: https://chromium-review.googlesource.com/1163882 Reviewed-by: Toon Verwaest Reviewed-by: Adam Klein Commit-Queue: Camillo Bruni Cr-Commit-Position: refs/heads/master@{#55142} PR-URL: https://github.com/nodejs/node/pull/22390 Fixes: https://github.com/nodejs/node/issues/17480 Fixes: https://github.com/nodejs/node/issues/17481 Refs: https://github.com/v8/v8/commit/e1a76995ef311eb3ca66e12ef1941ed596034d59 Reviewed-By: Anna Henningsen Reviewed-By: Gus Caplan Reviewed-By: James M Snell --- include/v8.h | 39 +++++ src/api.cc | 4 - src/objects.cc | 63 ++++---- test/unittests/api/interceptor-unittest.cc | 176 +++++++++++++++++++++ 4 files changed, 247 insertions(+), 35 deletions(-) diff --git a/include/v8.h b/include/v8.h index 23d5ba36db..20a65afcbc 100644 --- a/include/v8.h +++ b/include/v8.h @@ -5994,6 +5994,26 @@ enum class PropertyHandlerFlags { }; struct NamedPropertyHandlerConfiguration { + NamedPropertyHandlerConfiguration( + GenericNamedPropertyGetterCallback getter, + GenericNamedPropertySetterCallback setter, + GenericNamedPropertyQueryCallback query, + GenericNamedPropertyDeleterCallback deleter, + GenericNamedPropertyEnumeratorCallback enumerator, + GenericNamedPropertyDefinerCallback definer, + GenericNamedPropertyDescriptorCallback descriptor, + Local data = Local(), + PropertyHandlerFlags flags = PropertyHandlerFlags::kNone) + : getter(getter), + setter(setter), + query(query), + deleter(deleter), + enumerator(enumerator), + definer(definer), + descriptor(descriptor), + data(data), + flags(flags) {} + NamedPropertyHandlerConfiguration( /** Note: getter is required */ GenericNamedPropertyGetterCallback getter = 0, @@ -6045,6 +6065,25 @@ struct NamedPropertyHandlerConfiguration { struct IndexedPropertyHandlerConfiguration { + IndexedPropertyHandlerConfiguration( + IndexedPropertyGetterCallback getter, + IndexedPropertySetterCallback setter, IndexedPropertyQueryCallback query, + IndexedPropertyDeleterCallback deleter, + IndexedPropertyEnumeratorCallback enumerator, + IndexedPropertyDefinerCallback definer, + IndexedPropertyDescriptorCallback descriptor, + Local data = Local(), + PropertyHandlerFlags flags = PropertyHandlerFlags::kNone) + : getter(getter), + setter(setter), + query(query), + deleter(deleter), + enumerator(enumerator), + definer(definer), + descriptor(descriptor), + data(data), + flags(flags) {} + IndexedPropertyHandlerConfiguration( /** Note: getter is required */ IndexedPropertyGetterCallback getter = 0, diff --git a/src/api.cc b/src/api.cc index 7c569e3a9f..6155cbb325 100644 --- a/src/api.cc +++ b/src/api.cc @@ -1879,10 +1879,6 @@ static i::Handle CreateInterceptorInfo( i::Isolate* isolate, Getter getter, Setter setter, Query query, Descriptor descriptor, Deleter remover, Enumerator enumerator, Definer definer, Local data, PropertyHandlerFlags flags) { - // Either intercept attributes or descriptor. - DCHECK(query == nullptr || descriptor == nullptr); - // Only use descriptor callback with definer callback. - DCHECK(query == nullptr || definer == nullptr); auto obj = i::Handle::cast( isolate->factory()->NewStruct(i::INTERCEPTOR_INFO_TYPE, i::TENURED)); obj->set_flags(0); diff --git a/src/objects.cc b/src/objects.cc index 01421bacc4..0aac39d304 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -7817,41 +7817,42 @@ Maybe GetPropertyDescriptorWithInterceptor(LookupIterator* it, } } - if (it->state() == LookupIterator::INTERCEPTOR) { - Isolate* isolate = it->isolate(); - Handle interceptor = it->GetInterceptor(); - if (!interceptor->descriptor()->IsUndefined(isolate)) { - Handle result; - Handle holder = it->GetHolder(); + if (it->state() != LookupIterator::INTERCEPTOR) return Just(false); - Handle receiver = it->GetReceiver(); - if (!receiver->IsJSReceiver()) { - ASSIGN_RETURN_ON_EXCEPTION_VALUE( - isolate, receiver, Object::ConvertReceiver(isolate, receiver), - Nothing()); - } + Isolate* isolate = it->isolate(); + Handle interceptor = it->GetInterceptor(); + if (interceptor->descriptor()->IsUndefined(isolate)) return Just(false); - PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, kDontThrow); - if (it->IsElement()) { - result = args.CallIndexedDescriptor(interceptor, it->index()); - } else { - result = args.CallNamedDescriptor(interceptor, it->name()); - } - if (!result.is_null()) { - // Request successfully intercepted, try to set the property - // descriptor. - Utils::ApiCheck( - PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc), - it->IsElement() ? "v8::IndexedPropertyDescriptorCallback" - : "v8::NamedPropertyDescriptorCallback", - "Invalid property descriptor."); + Handle result; + Handle holder = it->GetHolder(); - return Just(true); - } - it->Next(); - } + Handle receiver = it->GetReceiver(); + if (!receiver->IsJSReceiver()) { + ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, receiver, + Object::ConvertReceiver(isolate, receiver), + Nothing()); + } + + PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, + *holder, kDontThrow); + if (it->IsElement()) { + result = args.CallIndexedDescriptor(interceptor, it->index()); + } else { + result = args.CallNamedDescriptor(interceptor, it->name()); } + if (!result.is_null()) { + // Request successfully intercepted, try to set the property + // descriptor. + Utils::ApiCheck( + PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc), + it->IsElement() ? "v8::IndexedPropertyDescriptorCallback" + : "v8::NamedPropertyDescriptorCallback", + "Invalid property descriptor."); + + return Just(true); + } + + it->Next(); return Just(false); } } // namespace diff --git a/test/unittests/api/interceptor-unittest.cc b/test/unittests/api/interceptor-unittest.cc index 2f9f0e459e..b13384f18a 100644 --- a/test/unittests/api/interceptor-unittest.cc +++ b/test/unittests/api/interceptor-unittest.cc @@ -29,4 +29,180 @@ TEST_F(InterceptorTest, FreezeApiObjectWithInterceptor) { } } // namespace + +namespace internal { +namespace { + +class InterceptorLoggingTest : public TestWithNativeContext { + public: + InterceptorLoggingTest() {} + + static const int kTestIndex = 0; + + static void NamedPropertyGetter(Local name, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named getter"); + } + + static void NamedPropertySetter(Local name, Local value, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named setter"); + } + + static void NamedPropertyQuery( + Local name, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named query"); + } + + static void NamedPropertyDeleter( + Local name, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named deleter"); + } + + static void NamedPropertyEnumerator( + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named enumerator"); + } + + static void NamedPropertyDefiner( + Local name, const v8::PropertyDescriptor& desc, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named definer"); + } + + static void NamedPropertyDescriptor( + Local name, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named descriptor"); + } + + static void IndexedPropertyGetter( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed getter"); + } + + static void IndexedPropertySetter( + uint32_t index, Local value, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed setter"); + } + + static void IndexedPropertyQuery( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed query"); + } + + static void IndexedPropertyDeleter( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed deleter"); + } + + static void IndexedPropertyEnumerator( + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed enumerator"); + } + + static void IndexedPropertyDefiner( + uint32_t index, const v8::PropertyDescriptor& desc, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed definer"); + } + + static void IndexedPropertyDescriptor( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed descriptor"); + } + + template + static void LogCallback(const v8::PropertyCallbackInfo& info, + const char* callback_name) { + InterceptorLoggingTest* test = reinterpret_cast( + info.This()->GetAlignedPointerFromInternalField(kTestIndex)); + test->Log(callback_name); + } + + void Log(const char* callback_name) { + if (log_is_empty_) { + log_is_empty_ = false; + } else { + log_ << ", "; + } + log_ << callback_name; + } + + protected: + void SetUp() override { + // Set up the object that supports full interceptors. + v8::Local templ = v8::ObjectTemplate::New(v8_isolate()); + templ->SetInternalFieldCount(1); + templ->SetHandler(v8::NamedPropertyHandlerConfiguration( + NamedPropertyGetter, NamedPropertySetter, NamedPropertyQuery, + NamedPropertyDeleter, NamedPropertyEnumerator, NamedPropertyDefiner, + NamedPropertyDescriptor)); + templ->SetHandler(v8::IndexedPropertyHandlerConfiguration( + IndexedPropertyGetter, IndexedPropertySetter, IndexedPropertyQuery, + IndexedPropertyDeleter, IndexedPropertyEnumerator, + IndexedPropertyDefiner, IndexedPropertyDescriptor)); + v8::Local instance = + templ->NewInstance(context()).ToLocalChecked(); + instance->SetAlignedPointerInInternalField(kTestIndex, this); + SetGlobalProperty("obj", instance); + } + + std::string Run(const char* script) { + log_is_empty_ = true; + log_.str(std::string()); + log_.clear(); + + RunJS(script); + return log_.str(); + } + + private: + bool log_is_empty_ = false; + std::stringstream log_; +}; + +TEST_F(InterceptorLoggingTest, DispatchTest) { + EXPECT_EQ(Run("for (var p in obj) {}"), + "indexed enumerator, named enumerator"); + EXPECT_EQ(Run("Object.keys(obj)"), "indexed enumerator, named enumerator"); + + EXPECT_EQ(Run("obj.foo"), "named getter"); + EXPECT_EQ(Run("obj[42]"), "indexed getter"); + + EXPECT_EQ(Run("obj.foo = null"), "named setter"); + EXPECT_EQ(Run("obj[42] = null"), "indexed setter"); + + EXPECT_EQ(Run("Object.getOwnPropertyDescriptor(obj, 'foo')"), + "named descriptor"); + + EXPECT_EQ(Run("Object.getOwnPropertyDescriptor(obj, 42)"), + "indexed descriptor"); + + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {value: 42})"), + "named descriptor, named definer, named setter"); + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {get(){} })"), + "named descriptor, named definer"); + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {set(value){}})"), + "named descriptor, named definer"); + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {get(){}, set(value){}})"), + "named descriptor, named definer"); + + EXPECT_EQ(Run("Object.defineProperty(obj, 42, {value: 'foo'})"), + "indexed descriptor, " + // then attempt definer first and fallback to setter. + "indexed definer, indexed setter"); + + EXPECT_EQ(Run("Object.prototype.propertyIsEnumerable.call(obj, 'a')"), + "named query"); + EXPECT_EQ(Run("Object.prototype.propertyIsEnumerable.call(obj, 42)"), + "indexed query"); + + EXPECT_EQ(Run("Object.prototype.hasOwnProperty.call(obj, 'a')"), + "named query"); + // TODO(cbruni): Fix once hasOnwProperty is fixed (https://crbug.com/872628) + EXPECT_EQ(Run("Object.prototype.hasOwnProperty.call(obj, '42')"), ""); +} +} // namespace +} // namespace internal } // namespace v8 -- 2.17.0 From 7766baf943a61843eaf706f349dd663218639e8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 4 Sep 2018 21:22:51 +0200 Subject: [PATCH 05/11] deps: cherry-pick ba752ea from upstream V8 Original commit message: [cpu-profiler] Use instruction start as the key for the CodeMap Previously we used the start address of the AbstractCode object. This doesn't make sense for off-heap builtins, where the code isn't contained in the object itself. It also hides other potential problems - sometimes the sample.pc is inside the AbstractCode object header - this is never valid. There were a few changes necessary to make this happen: - Change the interface of CodeMoveEvent. Now 'to' and 'from' are both AbstractCode objects, which is nice because many users were taking 'to' and adding the header offset to it to try and find the instruction start address. This isn't valid for off-heap builtins. - Fix a bug in CodeMap::MoveCode where we didn't update the CodeEntry object to reflect the new instruction_start. - Rename the 'start' field in all of the CodeEventRecord sub-classes to make it clear that this is the address of the first instruction. - Fix the confusion in RecordTickSample between 'tos' and 'pc' which caused pc_offset to be calculated incorrectly. Bug: v8:7983 Change-Id: I3e9dddf74e4b2e96a5f031d216ef7008d6f184d1 Reviewed-on: https://chromium-review.googlesource.com/1148457 Commit-Queue: Peter Marshall Reviewed-by: Jakob Gruber Reviewed-by: Michael Lippautz Cr-Commit-Position: refs/heads/master@{#54749} Refs: https://github.com/v8/v8/commit/ba752ea4c50713dff1e94f45a79db3ba968a8d66 PR-URL: https://github.com/nodejs/node/pull/21983 Reviewed-By: Refael Ackermann Reviewed-By: Gus Caplan Reviewed-By: Ujjwal Sharma Reviewed-By: Matteo Collina --- src/code-events.h | 4 +-- src/heap/mark-compact.cc | 2 +- src/log.cc | 25 ++++++-------- src/log.h | 4 +-- src/perf-jit.cc | 2 +- src/perf-jit.h | 4 +-- src/profiler/cpu-profiler-inl.h | 10 +++--- src/profiler/cpu-profiler.cc | 2 +- src/profiler/cpu-profiler.h | 14 ++++---- src/profiler/profile-generator.cc | 41 +++++++++++++++------- src/profiler/profile-generator.h | 2 ++ src/profiler/profiler-listener.cc | 49 +++++++++++++-------------- src/profiler/profiler-listener.h | 2 +- src/snapshot/serializer.h | 4 +-- test/cctest/test-cpu-profiler.cc | 18 +++++----- test/cctest/test-log.cc | 2 +- test/cctest/test-profile-generator.cc | 3 +- 17 files changed, 101 insertions(+), 87 deletions(-) diff --git a/src/code-events.h b/src/code-events.h index 09cd5a62e0..ec07a2e107 100644 --- a/src/code-events.h +++ b/src/code-events.h @@ -83,7 +83,7 @@ class CodeEventListener { virtual void GetterCallbackEvent(Name* name, Address entry_point) = 0; virtual void SetterCallbackEvent(Name* name, Address entry_point) = 0; virtual void RegExpCodeCreateEvent(AbstractCode* code, String* source) = 0; - virtual void CodeMoveEvent(AbstractCode* from, Address to) = 0; + virtual void CodeMoveEvent(AbstractCode* from, AbstractCode* to) = 0; virtual void SharedFunctionInfoMoveEvent(Address from, Address to) = 0; virtual void CodeMovingGCEvent() = 0; virtual void CodeDisableOptEvent(AbstractCode* code, @@ -154,7 +154,7 @@ class CodeEventDispatcher { void RegExpCodeCreateEvent(AbstractCode* code, String* source) { CODE_EVENT_DISPATCH(RegExpCodeCreateEvent(code, source)); } - void CodeMoveEvent(AbstractCode* from, Address to) { + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) { CODE_EVENT_DISPATCH(CodeMoveEvent(from, to)); } void SharedFunctionInfoMoveEvent(Address from, Address to) { diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc index 6bc01238b8..af56c72418 100644 --- a/src/heap/mark-compact.cc +++ b/src/heap/mark-compact.cc @@ -1149,7 +1149,7 @@ class ProfilingMigrationObserver final : public MigrationObserver { int size) final { if (dest == CODE_SPACE || (dest == OLD_SPACE && dst->IsBytecodeArray())) { PROFILE(heap_->isolate(), - CodeMoveEvent(AbstractCode::cast(src), dst->address())); + CodeMoveEvent(AbstractCode::cast(src), AbstractCode::cast(dst))); } heap_->OnMoveEvent(dst, src, size); } diff --git a/src/log.cc b/src/log.cc index 77d68ef94e..f19897b27b 100644 --- a/src/log.cc +++ b/src/log.cc @@ -270,7 +270,7 @@ class PerfBasicLogger : public CodeEventLogger { explicit PerfBasicLogger(Isolate* isolate); ~PerfBasicLogger() override; - void CodeMoveEvent(AbstractCode* from, Address to) override {} + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override {} void CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) override {} @@ -496,7 +496,7 @@ class LowLevelLogger : public CodeEventLogger { LowLevelLogger(Isolate* isolate, const char* file_name); ~LowLevelLogger() override; - void CodeMoveEvent(AbstractCode* from, Address to) override; + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override; void CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) override {} void SnapshotPositionEvent(HeapObject* obj, int pos); @@ -615,11 +615,10 @@ void LowLevelLogger::LogRecordedBuffer(const wasm::WasmCode* code, code->instructions().length()); } -void LowLevelLogger::CodeMoveEvent(AbstractCode* from, Address to) { +void LowLevelLogger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) { CodeMoveStruct event; event.from_address = from->InstructionStart(); - size_t header_size = from->InstructionStart() - from->address(); - event.to_address = to + header_size; + event.to_address = to->InstructionStart(); LogWriteStruct(event); } @@ -641,7 +640,7 @@ class JitLogger : public CodeEventLogger { public: JitLogger(Isolate* isolate, JitCodeEventHandler code_event_handler); - void CodeMoveEvent(AbstractCode* from, Address to) override; + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override; void CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) override {} void AddCodeLinePosInfoEvent(void* jit_handler_data, int pc_offset, @@ -700,7 +699,7 @@ void JitLogger::LogRecordedBuffer(const wasm::WasmCode* code, const char* name, code_event_handler_(&event); } -void JitLogger::CodeMoveEvent(AbstractCode* from, Address to) { +void JitLogger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) { base::LockGuard guard(&logger_mutex_); JitCodeEvent event; @@ -709,12 +708,7 @@ void JitLogger::CodeMoveEvent(AbstractCode* from, Address to) { from->IsCode() ? JitCodeEvent::JIT_CODE : JitCodeEvent::BYTE_CODE; event.code_start = reinterpret_cast(from->InstructionStart()); event.code_len = from->InstructionSize(); - - // Calculate the header size. - const size_t header_size = from->InstructionStart() - from->address(); - - // Calculate the new start address of the instructions. - event.new_code_start = reinterpret_cast(to + header_size); + event.new_code_start = reinterpret_cast(to->InstructionStart()); event.isolate = reinterpret_cast(isolate_); code_event_handler_(&event); @@ -1431,9 +1425,10 @@ void Logger::RegExpCodeCreateEvent(AbstractCode* code, String* source) { msg.WriteToLogFile(); } -void Logger::CodeMoveEvent(AbstractCode* from, Address to) { +void Logger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) { if (!is_listening_to_code_events()) return; - MoveEventInternal(CodeEventListener::CODE_MOVE_EVENT, from->address(), to); + MoveEventInternal(CodeEventListener::CODE_MOVE_EVENT, from->address(), + to->address()); } namespace { diff --git a/src/log.h b/src/log.h index 485de0f4d1..3bc54a5926 100644 --- a/src/log.h +++ b/src/log.h @@ -222,7 +222,7 @@ class Logger : public CodeEventListener { // Emits a code create event for a RegExp. void RegExpCodeCreateEvent(AbstractCode* code, String* source); // Emits a code move event. - void CodeMoveEvent(AbstractCode* from, Address to); + void CodeMoveEvent(AbstractCode* from, AbstractCode* to); // Emits a code line info record event. void CodeLinePosInfoRecordEvent(Address code_start, ByteArray* source_position_table); @@ -486,7 +486,7 @@ class ExternalCodeEventListener : public CodeEventListener { void GetterCallbackEvent(Name* name, Address entry_point) override {} void SetterCallbackEvent(Name* name, Address entry_point) override {} void SharedFunctionInfoMoveEvent(Address from, Address to) override {} - void CodeMoveEvent(AbstractCode* from, Address to) override {} + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override {} void CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) override {} void CodeMovingGCEvent() override {} diff --git a/src/perf-jit.cc b/src/perf-jit.cc index 3aaa36bc12..f6b2cf401a 100644 --- a/src/perf-jit.cc +++ b/src/perf-jit.cc @@ -420,7 +420,7 @@ void PerfJitLogger::LogWriteUnwindingInfo(Code* code) { LogWriteBytes(padding_bytes, static_cast(padding_size)); } -void PerfJitLogger::CodeMoveEvent(AbstractCode* from, Address to) { +void PerfJitLogger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) { // We may receive a CodeMove event if a BytecodeArray object moves. Otherwise // code relocation is not supported. CHECK(from->IsBytecodeArray()); diff --git a/src/perf-jit.h b/src/perf-jit.h index d08f4b91ab..3b11cf30c2 100644 --- a/src/perf-jit.h +++ b/src/perf-jit.h @@ -41,7 +41,7 @@ class PerfJitLogger : public CodeEventLogger { explicit PerfJitLogger(Isolate* isolate); virtual ~PerfJitLogger(); - void CodeMoveEvent(AbstractCode* from, Address to) override; + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override; void CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) override {} @@ -120,7 +120,7 @@ class PerfJitLogger : public CodeEventLogger { public: explicit PerfJitLogger(Isolate* isolate) : CodeEventLogger(isolate) {} - void CodeMoveEvent(AbstractCode* from, Address to) override { + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override { UNIMPLEMENTED(); } diff --git a/src/profiler/cpu-profiler-inl.h b/src/profiler/cpu-profiler-inl.h index f6eaa8f8a3..9274bc03c6 100644 --- a/src/profiler/cpu-profiler-inl.h +++ b/src/profiler/cpu-profiler-inl.h @@ -16,17 +16,17 @@ namespace v8 { namespace internal { void CodeCreateEventRecord::UpdateCodeMap(CodeMap* code_map) { - code_map->AddCode(start, entry, size); + code_map->AddCode(instruction_start, entry, instruction_size); } void CodeMoveEventRecord::UpdateCodeMap(CodeMap* code_map) { - code_map->MoveCode(from, to); + code_map->MoveCode(from_instruction_start, to_instruction_start); } void CodeDisableOptEventRecord::UpdateCodeMap(CodeMap* code_map) { - CodeEntry* entry = code_map->FindEntry(start); + CodeEntry* entry = code_map->FindEntry(instruction_start); if (entry != nullptr) { entry->set_bailout_reason(bailout_reason); } @@ -34,7 +34,7 @@ void CodeDisableOptEventRecord::UpdateCodeMap(CodeMap* code_map) { void CodeDeoptEventRecord::UpdateCodeMap(CodeMap* code_map) { - CodeEntry* entry = code_map->FindEntry(start); + CodeEntry* entry = code_map->FindEntry(instruction_start); if (entry == nullptr) return; std::vector frames_vector( deopt_frames, deopt_frames + deopt_frame_count); @@ -44,7 +44,7 @@ void CodeDeoptEventRecord::UpdateCodeMap(CodeMap* code_map) { void ReportBuiltinEventRecord::UpdateCodeMap(CodeMap* code_map) { - CodeEntry* entry = code_map->FindEntry(start); + CodeEntry* entry = code_map->FindEntry(instruction_start); if (!entry) { // Code objects for builtins should already have been added to the map but // some of them have been filtered out by CpuProfiler. diff --git a/src/profiler/cpu-profiler.cc b/src/profiler/cpu-profiler.cc index 463a30f184..555c47f2f4 100644 --- a/src/profiler/cpu-profiler.cc +++ b/src/profiler/cpu-profiler.cc @@ -426,7 +426,7 @@ void CpuProfiler::LogBuiltins() { CodeEventsContainer evt_rec(CodeEventRecord::REPORT_BUILTIN); ReportBuiltinEventRecord* rec = &evt_rec.ReportBuiltinEventRecord_; Builtins::Name id = static_cast(i); - rec->start = builtins->builtin(id)->address(); + rec->instruction_start = builtins->builtin(id)->InstructionStart(); rec->builtin_id = id; processor_->Enqueue(evt_rec); } diff --git a/src/profiler/cpu-profiler.h b/src/profiler/cpu-profiler.h index febc154802..78bb3b4a25 100644 --- a/src/profiler/cpu-profiler.h +++ b/src/profiler/cpu-profiler.h @@ -53,9 +53,9 @@ class CodeEventRecord { class CodeCreateEventRecord : public CodeEventRecord { public: - Address start; + Address instruction_start; CodeEntry* entry; - unsigned size; + unsigned instruction_size; V8_INLINE void UpdateCodeMap(CodeMap* code_map); }; @@ -63,8 +63,8 @@ class CodeCreateEventRecord : public CodeEventRecord { class CodeMoveEventRecord : public CodeEventRecord { public: - Address from; - Address to; + Address from_instruction_start; + Address to_instruction_start; V8_INLINE void UpdateCodeMap(CodeMap* code_map); }; @@ -72,7 +72,7 @@ class CodeMoveEventRecord : public CodeEventRecord { class CodeDisableOptEventRecord : public CodeEventRecord { public: - Address start; + Address instruction_start; const char* bailout_reason; V8_INLINE void UpdateCodeMap(CodeMap* code_map); @@ -81,7 +81,7 @@ class CodeDisableOptEventRecord : public CodeEventRecord { class CodeDeoptEventRecord : public CodeEventRecord { public: - Address start; + Address instruction_start; const char* deopt_reason; int deopt_id; Address pc; @@ -95,7 +95,7 @@ class CodeDeoptEventRecord : public CodeEventRecord { class ReportBuiltinEventRecord : public CodeEventRecord { public: - Address start; + Address instruction_start; Builtins::Name builtin_id; V8_INLINE void UpdateCodeMap(CodeMap* code_map); diff --git a/src/profiler/profile-generator.cc b/src/profiler/profile-generator.cc index 92619f2fbf..845fe97b64 100644 --- a/src/profiler/profile-generator.cc +++ b/src/profiler/profile-generator.cc @@ -529,6 +529,8 @@ void CodeMap::AddCode(Address addr, CodeEntry* entry, unsigned size) { ClearCodesInRange(addr, addr + size); unsigned index = AddCodeEntry(addr, entry); code_map_.emplace(addr, CodeEntryMapInfo{index, size}); + DCHECK(entry->instruction_start() == kNullAddress || + addr == entry->instruction_start()); } void CodeMap::ClearCodesInRange(Address start, Address end) { @@ -550,8 +552,14 @@ CodeEntry* CodeMap::FindEntry(Address addr) { auto it = code_map_.upper_bound(addr); if (it == code_map_.begin()) return nullptr; --it; - Address end_address = it->first + it->second.size; - return addr < end_address ? entry(it->second.index) : nullptr; + Address start_address = it->first; + Address end_address = start_address + it->second.size; + CodeEntry* ret = addr < end_address ? entry(it->second.index) : nullptr; + if (ret && ret->instruction_start() != kNullAddress) { + DCHECK_EQ(start_address, ret->instruction_start()); + DCHECK(addr >= start_address && addr < end_address); + } + return ret; } void CodeMap::MoveCode(Address from, Address to) { @@ -563,6 +571,9 @@ void CodeMap::MoveCode(Address from, Address to) { DCHECK(from + info.size <= to || to + info.size <= from); ClearCodesInRange(to, to + info.size); code_map_.emplace(to, info); + + CodeEntry* entry = code_entries_[info.index].entry; + entry->set_instruction_start(to); } unsigned CodeMap::AddCodeEntry(Address start, CodeEntry* entry) { @@ -693,26 +704,29 @@ void ProfileGenerator::RecordTickSample(const TickSample& sample) { if (sample.pc != nullptr) { if (sample.has_external_callback && sample.state == EXTERNAL) { // Don't use PC when in external callback code, as it can point - // inside callback's code, and we will erroneously report + // inside a callback's code, and we will erroneously report // that a callback calls itself. stack_trace.push_back( {FindEntry(reinterpret_cast
(sample.external_callback_entry)), no_line_info}); } else { - CodeEntry* pc_entry = FindEntry(reinterpret_cast
(sample.pc)); - // If there is no pc_entry we're likely in native code. - // Find out, if top of stack was pointing inside a JS function - // meaning that we have encountered a frameless invocation. + Address attributed_pc = reinterpret_cast
(sample.pc); + CodeEntry* pc_entry = FindEntry(attributed_pc); + // If there is no pc_entry, we're likely in native code. Find out if the + // top of the stack (the return address) was pointing inside a JS + // function, meaning that we have encountered a frameless invocation. if (!pc_entry && !sample.has_external_callback) { - pc_entry = FindEntry(reinterpret_cast
(sample.tos)); + attributed_pc = reinterpret_cast
(sample.tos); + pc_entry = FindEntry(attributed_pc); } // If pc is in the function code before it set up stack frame or after the - // frame was destroyed SafeStackFrameIterator incorrectly thinks that - // ebp contains return address of the current function and skips caller's - // frame. Check for this case and just skip such samples. + // frame was destroyed, SafeStackFrameIterator incorrectly thinks that + // ebp contains the return address of the current function and skips the + // caller's frame. Check for this case and just skip such samples. if (pc_entry) { - int pc_offset = static_cast(reinterpret_cast
(sample.pc) - - pc_entry->instruction_start()); + int pc_offset = + static_cast(attributed_pc - pc_entry->instruction_start()); + DCHECK_GE(pc_offset, 0); src_line = pc_entry->GetSourceLine(pc_offset); if (src_line == v8::CpuProfileNode::kNoLineNumberInfo) { src_line = pc_entry->line_number(); @@ -744,6 +758,7 @@ void ProfileGenerator::RecordTickSample(const TickSample& sample) { // Find out if the entry has an inlining stack associated. int pc_offset = static_cast(stack_pos - entry->instruction_start()); + DCHECK_GE(pc_offset, 0); const std::vector>* inline_stack = entry->GetInlineStack(pc_offset); if (inline_stack) { diff --git a/src/profiler/profile-generator.h b/src/profiler/profile-generator.h index 3e301a4082..8eef05bcdb 100644 --- a/src/profiler/profile-generator.h +++ b/src/profiler/profile-generator.h @@ -108,7 +108,9 @@ class CodeEntry { const std::vector>* GetInlineStack( int pc_offset) const; + void set_instruction_start(Address start) { instruction_start_ = start; } Address instruction_start() const { return instruction_start_; } + CodeEventListener::LogEventsAndTags tag() const { return TagField::decode(bit_field_); } diff --git a/src/profiler/profiler-listener.cc b/src/profiler/profiler-listener.cc index 9c29da9ec7..f90a2e11d3 100644 --- a/src/profiler/profiler-listener.cc +++ b/src/profiler/profiler-listener.cc @@ -24,9 +24,9 @@ ProfilerListener::~ProfilerListener() = default; void ProfilerListener::CallbackEvent(Name* name, Address entry_point) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = entry_point; + rec->instruction_start = entry_point; rec->entry = NewCodeEntry(CodeEventListener::CALLBACK_TAG, GetName(name)); - rec->size = 1; + rec->instruction_size = 1; DispatchCodeEvent(evt_rec); } @@ -34,13 +34,13 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, AbstractCode* code, const char* name) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = code->address(); + rec->instruction_start = code->InstructionStart(); rec->entry = NewCodeEntry(tag, GetName(name), CodeEntry::kEmptyResourceName, CpuProfileNode::kNoLineNumberInfo, CpuProfileNode::kNoColumnNumberInfo, nullptr, code->InstructionStart()); RecordInliningInfo(rec->entry, code); - rec->size = code->ExecutableSize(); + rec->instruction_size = code->InstructionSize(); DispatchCodeEvent(evt_rec); } @@ -48,13 +48,13 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, AbstractCode* code, Name* name) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = code->address(); + rec->instruction_start = code->InstructionStart(); rec->entry = NewCodeEntry(tag, GetName(name), CodeEntry::kEmptyResourceName, CpuProfileNode::kNoLineNumberInfo, CpuProfileNode::kNoColumnNumberInfo, nullptr, code->InstructionStart()); RecordInliningInfo(rec->entry, code); - rec->size = code->ExecutableSize(); + rec->instruction_size = code->InstructionSize(); DispatchCodeEvent(evt_rec); } @@ -64,7 +64,7 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, Name* script_name) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = code->address(); + rec->instruction_start = code->InstructionStart(); rec->entry = NewCodeEntry(tag, GetName(shared->DebugName()), GetName(InferScriptName(script_name, shared)), CpuProfileNode::kNoLineNumberInfo, @@ -72,7 +72,7 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, code->InstructionStart()); RecordInliningInfo(rec->entry, code); rec->entry->FillFunctionInfo(shared); - rec->size = code->ExecutableSize(); + rec->instruction_size = code->InstructionSize(); DispatchCodeEvent(evt_rec); } @@ -83,7 +83,7 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, int column) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = abstract_code->address(); + rec->instruction_start = abstract_code->InstructionStart(); std::unique_ptr line_table; if (shared->script()->IsScript()) { Script* script = Script::cast(shared->script()); @@ -105,7 +105,7 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, std::move(line_table), abstract_code->InstructionStart()); RecordInliningInfo(rec->entry, abstract_code); rec->entry->FillFunctionInfo(shared); - rec->size = abstract_code->ExecutableSize(); + rec->instruction_size = abstract_code->InstructionSize(); DispatchCodeEvent(evt_rec); } @@ -114,20 +114,20 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, wasm::WasmName name) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = code->instruction_start(); + rec->instruction_start = code->instruction_start(); rec->entry = NewCodeEntry( tag, GetName(name.start()), CodeEntry::kWasmResourceNamePrefix, CpuProfileNode::kNoLineNumberInfo, CpuProfileNode::kNoColumnNumberInfo, nullptr, code->instruction_start()); - rec->size = code->instructions().length(); + rec->instruction_size = code->instructions().length(); DispatchCodeEvent(evt_rec); } -void ProfilerListener::CodeMoveEvent(AbstractCode* from, Address to) { +void ProfilerListener::CodeMoveEvent(AbstractCode* from, AbstractCode* to) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_MOVE); CodeMoveEventRecord* rec = &evt_rec.CodeMoveEventRecord_; - rec->from = from->address(); - rec->to = to; + rec->from_instruction_start = from->InstructionStart(); + rec->to_instruction_start = to->InstructionStart(); DispatchCodeEvent(evt_rec); } @@ -135,7 +135,7 @@ void ProfilerListener::CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_DISABLE_OPT); CodeDisableOptEventRecord* rec = &evt_rec.CodeDisableOptEventRecord_; - rec->start = code->address(); + rec->instruction_start = code->InstructionStart(); rec->bailout_reason = GetBailoutReason(shared->disable_optimization_reason()); DispatchCodeEvent(evt_rec); } @@ -145,7 +145,7 @@ void ProfilerListener::CodeDeoptEvent(Code* code, DeoptimizeKind kind, CodeEventsContainer evt_rec(CodeEventRecord::CODE_DEOPT); CodeDeoptEventRecord* rec = &evt_rec.CodeDeoptEventRecord_; Deoptimizer::DeoptInfo info = Deoptimizer::GetDeoptInfo(code, pc); - rec->start = code->address(); + rec->instruction_start = code->InstructionStart(); rec->deopt_reason = DeoptimizeReasonToString(info.deopt_reason); rec->deopt_id = info.deopt_id; rec->pc = pc; @@ -160,10 +160,10 @@ void ProfilerListener::CodeDeoptEvent(Code* code, DeoptimizeKind kind, void ProfilerListener::GetterCallbackEvent(Name* name, Address entry_point) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = entry_point; + rec->instruction_start = entry_point; rec->entry = NewCodeEntry(CodeEventListener::CALLBACK_TAG, GetConsName("get ", name)); - rec->size = 1; + rec->instruction_size = 1; DispatchCodeEvent(evt_rec); } @@ -171,23 +171,22 @@ void ProfilerListener::RegExpCodeCreateEvent(AbstractCode* code, String* source) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = code->address(); + rec->instruction_start = code->InstructionStart(); rec->entry = NewCodeEntry( CodeEventListener::REG_EXP_TAG, GetConsName("RegExp: ", source), CodeEntry::kEmptyResourceName, CpuProfileNode::kNoLineNumberInfo, - CpuProfileNode::kNoColumnNumberInfo, nullptr, - code->raw_instruction_start()); - rec->size = code->ExecutableSize(); + CpuProfileNode::kNoColumnNumberInfo, nullptr, code->InstructionStart()); + rec->instruction_size = code->InstructionSize(); DispatchCodeEvent(evt_rec); } void ProfilerListener::SetterCallbackEvent(Name* name, Address entry_point) { CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; - rec->start = entry_point; + rec->instruction_start = entry_point; rec->entry = NewCodeEntry(CodeEventListener::CALLBACK_TAG, GetConsName("set ", name)); - rec->size = 1; + rec->instruction_size = 1; DispatchCodeEvent(evt_rec); } diff --git a/src/profiler/profiler-listener.h b/src/profiler/profiler-listener.h index 5cff7cc11d..51fba18a60 100644 --- a/src/profiler/profiler-listener.h +++ b/src/profiler/profiler-listener.h @@ -44,7 +44,7 @@ class ProfilerListener : public CodeEventListener { wasm::WasmName name) override; void CodeMovingGCEvent() override {} - void CodeMoveEvent(AbstractCode* from, Address to) override; + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override; void CodeDisableOptEvent(AbstractCode* code, SharedFunctionInfo* shared) override; void CodeDeoptEvent(Code* code, DeoptimizeKind kind, Address pc, diff --git a/src/snapshot/serializer.h b/src/snapshot/serializer.h index 6a5d1a4aac..658d37f286 100644 --- a/src/snapshot/serializer.h +++ b/src/snapshot/serializer.h @@ -28,8 +28,8 @@ class CodeAddressMap : public CodeEventLogger { isolate_->logger()->RemoveCodeEventListener(this); } - void CodeMoveEvent(AbstractCode* from, Address to) override { - address_to_name_map_.Move(from->address(), to); + void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override { + address_to_name_map_.Move(from->address(), to->address()); } void CodeDisableOptEvent(AbstractCode* code, diff --git a/test/cctest/test-cpu-profiler.cc b/test/cctest/test-cpu-profiler.cc index 4e7a70c76f..f74bdf1ede 100644 --- a/test/cctest/test-cpu-profiler.cc +++ b/test/cctest/test-cpu-profiler.cc @@ -176,27 +176,29 @@ TEST(CodeEvents) { "comment"); profiler_listener.CodeCreateEvent(i::Logger::BUILTIN_TAG, comment2_code, "comment2"); - profiler_listener.CodeMoveEvent(comment2_code, moved_code->address()); + profiler_listener.CodeMoveEvent(comment2_code, moved_code); // Enqueue a tick event to enable code events processing. - EnqueueTickSampleEvent(processor, aaa_code->address()); + EnqueueTickSampleEvent(processor, aaa_code->InstructionStart()); isolate->logger()->RemoveCodeEventListener(&profiler_listener); processor->StopSynchronously(); // Check the state of profile generator. - CodeEntry* aaa = generator->code_map()->FindEntry(aaa_code->address()); + CodeEntry* aaa = + generator->code_map()->FindEntry(aaa_code->InstructionStart()); CHECK(aaa); CHECK_EQ(0, strcmp(aaa_str, aaa->name())); CodeEntry* comment = - generator->code_map()->FindEntry(comment_code->address()); + generator->code_map()->FindEntry(comment_code->InstructionStart()); CHECK(comment); CHECK_EQ(0, strcmp("comment", comment->name())); - CHECK(!generator->code_map()->FindEntry(comment2_code->address())); + CHECK(!generator->code_map()->FindEntry(comment2_code->InstructionStart())); - CodeEntry* comment2 = generator->code_map()->FindEntry(moved_code->address()); + CodeEntry* comment2 = + generator->code_map()->FindEntry(moved_code->InstructionStart()); CHECK(comment2); CHECK_EQ(0, strcmp("comment2", comment2->name())); } @@ -298,11 +300,11 @@ TEST(Issue1398) { profiler_listener.CodeCreateEvent(i::Logger::BUILTIN_TAG, code, "bbb"); v8::TickSample* sample = processor->StartTickSample(); - sample->pc = reinterpret_cast(code->address()); + sample->pc = reinterpret_cast(code->InstructionStart()); sample->tos = nullptr; sample->frames_count = v8::TickSample::kMaxFramesCount; for (unsigned i = 0; i < sample->frames_count; ++i) { - sample->stack[i] = reinterpret_cast(code->address()); + sample->stack[i] = reinterpret_cast(code->InstructionStart()); } processor->FinishTickSample(); diff --git a/test/cctest/test-log.cc b/test/cctest/test-log.cc index 0b13a7e660..1dfa22b4cc 100644 --- a/test/cctest/test-log.cc +++ b/test/cctest/test-log.cc @@ -751,7 +751,7 @@ TEST(Issue539892) { explicit FakeCodeEventLogger(i::Isolate* isolate) : CodeEventLogger(isolate) {} - void CodeMoveEvent(i::AbstractCode* from, Address to) override {} + void CodeMoveEvent(i::AbstractCode* from, i::AbstractCode* to) override {} void CodeDisableOptEvent(i::AbstractCode* code, i::SharedFunctionInfo* shared) override {} diff --git a/test/cctest/test-profile-generator.cc b/test/cctest/test-profile-generator.cc index 9b2d7e3ab2..b53bf148e6 100644 --- a/test/cctest/test-profile-generator.cc +++ b/test/cctest/test-profile-generator.cc @@ -676,7 +676,8 @@ int GetFunctionLineNumber(CpuProfiler& profiler, LocalContext& env, i::Handle func = i::Handle::cast( v8::Utils::OpenHandle(*v8::Local::Cast( env->Global()->Get(env.local(), v8_str(name)).ToLocalChecked()))); - CodeEntry* func_entry = code_map->FindEntry(func->abstract_code()->address()); + CodeEntry* func_entry = + code_map->FindEntry(func->abstract_code()->InstructionStart()); if (!func_entry) FATAL("%s", name); return func_entry->line_number(); } -- 2.17.0 From fc1770b0d1a5762bd14ccbd8a3688a7592d96cd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 4 Sep 2018 21:24:03 +0200 Subject: [PATCH 06/11] deps: cherry-pick bf5ea81 from upstream V8 Original commit message: [tracing] allow dynamic control of tracing If the trace_buffer_ was null, we were returning a pointer to a static flag back that permanently disabled that particular trace point. This implied an assumption that tracing will be statically enabled at process startup, and once it is disabled, it will never be enabled again. On Node.js side we want to dynamically enable/disable tracing as per programmer intent. Change-Id: Ic7a7839b8450ab5c356d85e8e0826f42824907f4 Reviewed-on: https://chromium-review.googlesource.com/1161518 Reviewed-by: Yang Guo Commit-Queue: Ali Ijaz Sheikh Cr-Commit-Position: refs/heads/master@{#54903} Refs: https://github.com/v8/v8/commit/bf5ea8138c0726613c9d722a3ccb552a8f477992 PR-URL: https://github.com/nodejs/node/pull/21983 Reviewed-By: Refael Ackermann Reviewed-By: Gus Caplan Reviewed-By: Ujjwal Sharma Reviewed-By: Matteo Collina --- src/libplatform/tracing/tracing-controller.cc | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/libplatform/tracing/tracing-controller.cc b/src/libplatform/tracing/tracing-controller.cc index 647306d627..b4aa7baf72 100644 --- a/src/libplatform/tracing/tracing-controller.cc +++ b/src/libplatform/tracing/tracing-controller.cc @@ -24,18 +24,17 @@ namespace tracing { // convert internally to determine the category name from the char enabled // pointer. const char* g_category_groups[MAX_CATEGORY_GROUPS] = { - "toplevel", "tracing already shutdown", + "toplevel", "tracing categories exhausted; must increase MAX_CATEGORY_GROUPS", "__metadata"}; // The enabled flag is char instead of bool so that the API can be used from C. unsigned char g_category_group_enabled[MAX_CATEGORY_GROUPS] = {0}; // Indexes here have to match the g_category_groups array indexes above. -const int g_category_already_shutdown = 1; -const int g_category_categories_exhausted = 2; +const int g_category_categories_exhausted = 1; // Metadata category not used in V8. -// const int g_category_metadata = 3; -const int g_num_builtin_categories = 4; +// const int g_category_metadata = 2; +const int g_num_builtin_categories = 3; // Skip default categories. v8::base::AtomicWord g_category_index = g_num_builtin_categories; @@ -103,10 +102,6 @@ void TracingController::UpdateTraceEventDuration( const uint8_t* TracingController::GetCategoryGroupEnabled( const char* category_group) { - if (!trace_buffer_) { - DCHECK(!g_category_group_enabled[g_category_already_shutdown]); - return &g_category_group_enabled[g_category_already_shutdown]; - } return GetCategoryGroupEnabledInternal(category_group); } -- 2.17.0 From a3f258c7693fd76fc26edc8c9d5b67f260eb755d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 4 Sep 2018 21:24:34 +0200 Subject: [PATCH 07/11] deps: cherry-pick a8f6869 from upstream V8 Original commit message: [debug] Fully implement Debug::ArchiveDebug and Debug::RestoreDebug. I have a project that embeds V8 and uses a single `Isolate` from multiple threads. The program runs just fine, but sometimes the inspector doesn't stop on the correct line after stepping over a statement that switches threads behind the scenes, even though the original thread is restored by the time the next statement is executed. After some digging, I discovered that the `Debug::ArchiveDebug` and `Debug::RestoreDebug` methods, which should be responsible for saving/restoring this `ThreadLocal` information when switching threads, currently don't do anything. This commit implements those methods using MemCopy, in the style of other Archive/Restore methods in the V8 codebase. Related: https://groups.google.com/forum/#!topic/v8-users/_Qf2rwljRk8 Note: I believe my employer, Meteor Development Group, has previously signed the CLA using the group email address google-contrib@meteor.com. R=yangguo@chromium.org,jgruber@chromium.org CC=info@bnoordhuis.nl Bug: v8:7230 Change-Id: Id517c873eb81cd53f7216c7efd441b956cf7f943 Reviewed-on: https://chromium-review.googlesource.com/833260 Commit-Queue: Yang Guo Reviewed-by: Yang Guo Cr-Commit-Position: refs/heads/master@{#54902} Refs: https://github.com/v8/v8/commit/a8f6869177685cfb9c199c454a86f4698c260515 PR-URL: https://github.com/nodejs/node/pull/21983 Reviewed-By: Refael Ackermann Reviewed-By: Gus Caplan Reviewed-By: Ujjwal Sharma Reviewed-By: Matteo Collina --- AUTHORS | 2 + src/debug/debug.cc | 24 +++++-- src/debug/debug.h | 1 + src/v8threads.cc | 8 ++- src/v8threads.h | 1 + test/cctest/test-debug.cc | 127 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 155 insertions(+), 8 deletions(-) diff --git a/AUTHORS b/AUTHORS index 3873f0ca68..6179e2230d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -32,6 +32,7 @@ Facebook, Inc. <*@fb.com> Facebook, Inc. <*@oculus.com> Vewd Software AS <*@vewd.com> Groupon <*@groupon.com> +Meteor Development Group <*@meteor.com> Cloudflare, Inc. <*@cloudflare.com> Aaron Bieber @@ -49,6 +50,7 @@ Andrei Kashcha Anna Henningsen Bangfu Tao Ben Coe +Ben Newman Ben Noordhuis Benjamin Tan Bert Belder diff --git a/src/debug/debug.cc b/src/debug/debug.cc index 47de9523a5..3877f156ef 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -355,19 +355,31 @@ void Debug::ThreadInit() { char* Debug::ArchiveDebug(char* storage) { - // Simply reset state. Don't archive anything. - ThreadInit(); + MemCopy(storage, reinterpret_cast(&thread_local_), + ArchiveSpacePerThread()); return storage + ArchiveSpacePerThread(); } - char* Debug::RestoreDebug(char* storage) { - // Simply reset state. Don't restore anything. - ThreadInit(); + MemCopy(reinterpret_cast(&thread_local_), storage, + ArchiveSpacePerThread()); + + // Enter the debugger. + DebugScope debug_scope(this); + + // Clear any one-shot breakpoints that may have been set by the other + // thread, and reapply breakpoints for this thread. + ClearOneShot(); + + if (thread_local_.last_step_action_ != StepNone) { + // Reset the previous step action for this thread. + PrepareStep(thread_local_.last_step_action_); + } + return storage + ArchiveSpacePerThread(); } -int Debug::ArchiveSpacePerThread() { return 0; } +int Debug::ArchiveSpacePerThread() { return sizeof(ThreadLocal); } void Debug::Iterate(RootVisitor* v) { v->VisitRootPointer(Root::kDebug, nullptr, &thread_local_.return_value_); diff --git a/src/debug/debug.h b/src/debug/debug.h index 31881fe106..13844769c1 100644 --- a/src/debug/debug.h +++ b/src/debug/debug.h @@ -314,6 +314,7 @@ class Debug { static int ArchiveSpacePerThread(); void FreeThreadResources() { } void Iterate(RootVisitor* v); + void InitThread(const ExecutionAccess& lock) { ThreadInit(); } bool CheckExecutionState() { return is_active() && break_id() != 0; } diff --git a/src/v8threads.cc b/src/v8threads.cc index db927010ef..0fb333c1f3 100644 --- a/src/v8threads.cc +++ b/src/v8threads.cc @@ -45,7 +45,7 @@ void Locker::Initialize(v8::Isolate* isolate) { } else { internal::ExecutionAccess access(isolate_); isolate_->stack_guard()->ClearThread(access); - isolate_->stack_guard()->InitThread(access); + isolate_->thread_manager()->InitThread(access); } } DCHECK(isolate_->thread_manager()->IsLockedByCurrentThread()); @@ -95,6 +95,10 @@ Unlocker::~Unlocker() { namespace internal { +void ThreadManager::InitThread(const ExecutionAccess& lock) { + isolate_->stack_guard()->InitThread(lock); + isolate_->debug()->InitThread(lock); +} bool ThreadManager::RestoreThread() { DCHECK(IsLockedByCurrentThread()); @@ -127,7 +131,7 @@ bool ThreadManager::RestoreThread() { isolate_->FindPerThreadDataForThisThread(); if (per_thread == nullptr || per_thread->thread_state() == nullptr) { // This is a new thread. - isolate_->stack_guard()->InitThread(access); + InitThread(access); return false; } ThreadState* state = per_thread->thread_state(); diff --git a/src/v8threads.h b/src/v8threads.h index bb87afea7d..7fde0c9ec4 100644 --- a/src/v8threads.h +++ b/src/v8threads.h @@ -67,6 +67,7 @@ class ThreadManager { void Lock(); void Unlock(); + void InitThread(const ExecutionAccess&); void ArchiveThread(); bool RestoreThread(); void FreeThreadResources(); diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc index d3c10822ff..7430fbf06b 100644 --- a/test/cctest/test-debug.cc +++ b/test/cctest/test-debug.cc @@ -3717,6 +3717,133 @@ TEST(DebugBreakOffThreadTerminate) { CHECK(try_catch.HasTerminated()); } +class ArchiveRestoreThread : public v8::base::Thread, + public v8::debug::DebugDelegate { + public: + ArchiveRestoreThread(v8::Isolate* isolate, int spawn_count) + : Thread(Options("ArchiveRestoreThread")), + isolate_(isolate), + debug_(reinterpret_cast(isolate_)->debug()), + spawn_count_(spawn_count), + break_count_(0) {} + + virtual void Run() { + v8::Locker locker(isolate_); + isolate_->Enter(); + + v8::HandleScope scope(isolate_); + v8::Local context = v8::Context::New(isolate_); + v8::Context::Scope context_scope(context); + + v8::Local test = CompileFunction(isolate_, + "function test(n) {\n" + " debugger;\n" + " return n + 1;\n" + "}\n", + "test"); + + debug_->SetDebugDelegate(this); + v8::internal::DisableBreak enable_break(debug_, false); + + v8::Local args[1] = {v8::Integer::New(isolate_, spawn_count_)}; + + int result = test->Call(context, context->Global(), 1, args) + .ToLocalChecked() + ->Int32Value(context) + .FromJust(); + + // Verify that test(spawn_count_) returned spawn_count_ + 1. + CHECK_EQ(spawn_count_ + 1, result); + + isolate_->Exit(); + } + + void BreakProgramRequested(v8::Local context, + const std::vector&) { + auto stack_traces = v8::debug::StackTraceIterator::Create(isolate_); + if (!stack_traces->Done()) { + v8::debug::Location location = stack_traces->GetSourceLocation(); + + i::PrintF("ArchiveRestoreThread #%d hit breakpoint at line %d\n", + spawn_count_, location.GetLineNumber()); + + switch (location.GetLineNumber()) { + case 1: // debugger; + CHECK_EQ(break_count_, 0); + + // Attempt to stop on the next line after the first debugger + // statement. If debug->{Archive,Restore}Debug() improperly reset + // thread-local debug information, the debugger will fail to stop + // before the test function returns. + debug_->PrepareStep(StepNext); + + // Spawning threads while handling the current breakpoint verifies + // that the parent thread correctly archived and restored the + // state necessary to stop on the next line. If not, then control + // will simply continue past the `return n + 1` statement. + MaybeSpawnChildThread(); + + break; + + case 2: // return n + 1; + CHECK_EQ(break_count_, 1); + break; + + default: + CHECK(false); + } + } + + ++break_count_; + } + + void MaybeSpawnChildThread() { + if (spawn_count_ > 1) { + v8::Unlocker unlocker(isolate_); + + // Spawn a thread that spawns a thread that spawns a thread (and so + // on) so that the ThreadManager is forced to archive and restore + // the current thread. + ArchiveRestoreThread child(isolate_, spawn_count_ - 1); + child.Start(); + child.Join(); + + // The child thread sets itself as the debug delegate, so we need to + // usurp it after the child finishes, or else future breakpoints + // will be delegated to a destroyed ArchiveRestoreThread object. + debug_->SetDebugDelegate(this); + + // This is the most important check in this test, since + // child.GetBreakCount() will return 1 if the debugger fails to stop + // on the `return n + 1` line after the grandchild thread returns. + CHECK_EQ(child.GetBreakCount(), 2); + } + } + + int GetBreakCount() { return break_count_; } + + private: + v8::Isolate* isolate_; + v8::internal::Debug* debug_; + const int spawn_count_; + int break_count_; +}; + +TEST(DebugArchiveRestore) { + v8::Isolate::CreateParams create_params; + create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); + v8::Isolate* isolate = v8::Isolate::New(create_params); + + ArchiveRestoreThread thread(isolate, 5); + // Instead of calling thread.Start() and thread.Join() here, we call + // thread.Run() directly, to make sure we exercise archive/restore + // logic on the *current* thread as well as other threads. + thread.Run(); + CHECK_EQ(thread.GetBreakCount(), 2); + + isolate->Dispose(); +} + class DebugEventExpectNoException : public v8::debug::DebugDelegate { public: void ExceptionThrown(v8::Local paused_context, -- 2.17.0 From 3771c9abc852d73748dde22866e71cd552ec8214 Mon Sep 17 00:00:00 2001 From: Peter Marshall Date: Tue, 4 Sep 2018 15:48:15 +0200 Subject: [PATCH 08/11] deps: backport detailed line info for CPU profiler [cpu-profiler] Add flag to always generate accurate line info. https://chromium.googlesource.com/v8/v8/+/ 56baf56790de439b3f69e887e94beb3b301ed77c [cpu-profiler] Turn on detailed line info for optimized code https://chromium.googlesource.com/v8/v8/+/ 84894ce6d2af7feb9e1f5574409355120887326c [cpu-profiler] Separate the flags for generating extra line information https://chromium.googlesource.com/v8/v8/+/ 30ff6719db441cc7ef220d449970cc169067e256 PR-URL: https://github.com/nodejs/node/pull/22688 Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- src/flag-definitions.h | 3 +++ src/isolate.cc | 4 ++++ src/isolate.h | 2 ++ src/optimized-compilation-info.cc | 2 +- 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/flag-definitions.h b/src/flag-definitions.h index 052869f308..4d5aa6f885 100644 --- a/src/flag-definitions.h +++ b/src/flag-definitions.h @@ -1243,6 +1243,9 @@ DEFINE_BOOL(log_function_events, false, DEFINE_BOOL(prof, false, "Log statistical profiling information (implies --log-code).") +DEFINE_BOOL(detailed_line_info, true, + "Always generate detailed line information for CPU profiling.") + #if defined(ANDROID) // Phones and tablets have processors that are much slower than desktop // and laptop computers for which current heuristics are tuned. diff --git a/src/isolate.cc b/src/isolate.cc index b0a970305e..b33b713672 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -3247,6 +3247,10 @@ bool Isolate::use_optimizer() { !is_precise_count_code_coverage() && !is_block_count_code_coverage(); } +bool Isolate::NeedsDetailedOptimizedCodeLineInfo() const { + return NeedsSourcePositionsForProfiling() || FLAG_detailed_line_info; +} + bool Isolate::NeedsSourcePositionsForProfiling() const { return FLAG_trace_deopt || FLAG_trace_turbo || FLAG_trace_turbo_graph || FLAG_turbo_profiling || FLAG_perf_prof || is_profiling() || diff --git a/src/isolate.h b/src/isolate.h index 09aaf99684..d94c0fde14 100644 --- a/src/isolate.h +++ b/src/isolate.h @@ -1044,6 +1044,8 @@ class Isolate : private HiddenFactory { bool NeedsSourcePositionsForProfiling() const; + bool NeedsDetailedOptimizedCodeLineInfo() const; + bool is_best_effort_code_coverage() const { return code_coverage_mode() == debug::Coverage::kBestEffort; } diff --git a/src/optimized-compilation-info.cc b/src/optimized-compilation-info.cc index 09b8a0edea..77730919f1 100644 --- a/src/optimized-compilation-info.cc +++ b/src/optimized-compilation-info.cc @@ -38,7 +38,7 @@ OptimizedCompilationInfo::OptimizedCompilationInfo( // Collect source positions for optimized code when profiling or if debugger // is active, to be able to get more precise source positions at the price of // more memory consumption. - if (isolate->NeedsSourcePositionsForProfiling()) { + if (isolate->NeedsDetailedOptimizedCodeLineInfo()) { MarkAsSourcePositionsEnabled(); } -- 2.17.0 From ff05f6490aa2f568b1f26193cf1033e6392af4b4 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Tue, 11 Sep 2018 15:40:28 -0700 Subject: [PATCH 09/11] deps: cherry-pick 2363cdf from upstream V8 Original commit message: [tracing] do not add traces when disabled https://github.com/nodejs/node/issues/21038 Change-Id: Ic4c9f403b5e54a97d3170b2311dd5aab8c8357c8 Reviewed-on: https://chromium-review.googlesource.com/1217726 Commit-Queue: Ali Ijaz Sheikh Reviewed-by: Yang Guo Cr-Commit-Position: refs/heads/master@{#55809} Refs: https://github.com/v8/v8/commit/2363cdfefeb643285cbe8593b7c17d80e5d06cd9 PR-URL: https://github.com/nodejs/node/pull/22812 Reviewed-By: Refael Ackermann Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Eugene Ostroukhov --- src/libplatform/tracing/tracing-controller.cc | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/libplatform/tracing/tracing-controller.cc b/src/libplatform/tracing/tracing-controller.cc index b4aa7baf72..e0a6a1234c 100644 --- a/src/libplatform/tracing/tracing-controller.cc +++ b/src/libplatform/tracing/tracing-controller.cc @@ -63,13 +63,15 @@ uint64_t TracingController::AddTraceEvent( const uint64_t* arg_values, std::unique_ptr* arg_convertables, unsigned int flags) { - uint64_t handle; - TraceObject* trace_object = trace_buffer_->AddTraceEvent(&handle); - if (trace_object) { - trace_object->Initialize( - phase, category_enabled_flag, name, scope, id, bind_id, num_args, - arg_names, arg_types, arg_values, arg_convertables, flags, - CurrentTimestampMicroseconds(), CurrentCpuTimestampMicroseconds()); + uint64_t handle = 0; + if (mode_ != DISABLED) { + TraceObject* trace_object = trace_buffer_->AddTraceEvent(&handle); + if (trace_object) { + trace_object->Initialize( + phase, category_enabled_flag, name, scope, id, bind_id, num_args, + arg_names, arg_types, arg_values, arg_convertables, flags, + CurrentTimestampMicroseconds(), CurrentCpuTimestampMicroseconds()); + } } return handle; } @@ -81,13 +83,15 @@ uint64_t TracingController::AddTraceEventWithTimestamp( const uint64_t* arg_values, std::unique_ptr* arg_convertables, unsigned int flags, int64_t timestamp) { - uint64_t handle; - TraceObject* trace_object = trace_buffer_->AddTraceEvent(&handle); - if (trace_object) { - trace_object->Initialize(phase, category_enabled_flag, name, scope, id, - bind_id, num_args, arg_names, arg_types, - arg_values, arg_convertables, flags, timestamp, - CurrentCpuTimestampMicroseconds()); + uint64_t handle = 0; + if (mode_ != DISABLED) { + TraceObject* trace_object = trace_buffer_->AddTraceEvent(&handle); + if (trace_object) { + trace_object->Initialize(phase, category_enabled_flag, name, scope, id, + bind_id, num_args, arg_names, arg_types, + arg_values, arg_convertables, flags, timestamp, + CurrentCpuTimestampMicroseconds()); + } } return handle; } -- 2.17.0 From ab150160f8149063865729e52c6a7597133f9057 Mon Sep 17 00:00:00 2001 From: Alexey Kozyatinskiy Date: Fri, 10 Aug 2018 13:09:49 -0700 Subject: [PATCH 10/11] deps: cherry-pick dbfcc48 from upstream V8 Original commit message: ``` [inspector] added V8InspectorClient::resourceNameToUrl Some clients (see Node.js) use platform path as ScriptOrigin. Reporting platform path in protocol makes using protocol much harder. This CL introduced V8InspectorClient::resourceNameToUrl method that is called for any reported using protocol url. V8Inspector uses url internally as well so protocol client may generate pattern for blackboxing with file urls only and does not need to build complicated regexp that covers files urls and platform paths on different platforms. R=lushnikov@chromium.org TBR=yangguo@chromium.org Bug: none Change-Id: Iff302e7441df922fa5d689fe510f5a9bfd470b9b Reviewed-on: https://chromium-review.googlesource.com/1164624 Commit-Queue: Aleksey Kozyatinskiy Reviewed-by: Alexei Filippov Cr-Commit-Position: refs/heads/master@{#55029} ``` Refs: https://github.com/v8/v8/commit/dbfcc48 PR-URL: https://github.com/nodejs/node/pull/22251 Reviewed-By: Eugene Ostroukhov Reviewed-By: Tiancheng "Timothy" Gu --- include/v8-inspector.h | 5 + src/inspector/v8-debugger-agent-impl.cc | 5 +- src/inspector/v8-debugger-script.cc | 38 +++--- src/inspector/v8-debugger-script.h | 11 +- src/inspector/v8-debugger.cc | 13 +- src/inspector/v8-profiler-agent-impl.cc | 46 ++++--- src/inspector/v8-stack-trace-impl.cc | 24 +++- src/inspector/v8-stack-trace-impl.h | 4 +- .../resource-name-to-url-expected.txt | 122 ++++++++++++++++++ .../debugger/resource-name-to-url.js | 49 +++++++ test/inspector/inspector-test.cc | 15 +++ test/inspector/isolate-data.cc | 29 +++++ test/inspector/isolate-data.h | 4 + 13 files changed, 317 insertions(+), 48 deletions(-) create mode 100644 deps/v8/test/inspector/debugger/resource-name-to-url-expected.txt create mode 100644 deps/v8/test/inspector/debugger/resource-name-to-url.js diff --git a/include/v8-inspector.h b/include/v8-inspector.h index d879a94373..ad04d01bd2 100644 --- a/include/v8-inspector.h +++ b/include/v8-inspector.h @@ -215,6 +215,11 @@ class V8_EXPORT V8InspectorClient { virtual bool canExecuteScripts(int contextGroupId) { return true; } virtual void maxAsyncCallStackDepthChanged(int depth) {} + + virtual std::unique_ptr resourceNameToUrl( + const StringView& resourceName) { + return nullptr; + } }; // These stack trace ids are intended to be passed between debuggers and be diff --git a/src/inspector/v8-debugger-agent-impl.cc b/src/inspector/v8-debugger-agent-impl.cc index e4e6492b67..d9cb49b1d4 100644 --- a/src/inspector/v8-debugger-agent-impl.cc +++ b/src/inspector/v8-debugger-agent-impl.cc @@ -1396,7 +1396,7 @@ void V8DebuggerAgentImpl::didParseSource( protocol::StringUtil::parseJSON(inspected->auxData())); } bool isLiveEdit = script->isLiveEdit(); - bool hasSourceURL = script->hasSourceURL(); + bool hasSourceURLComment = script->hasSourceURLComment(); bool isModule = script->isModule(); String16 scriptId = script->scriptId(); String16 scriptURL = script->sourceURL(); @@ -1416,7 +1416,8 @@ void V8DebuggerAgentImpl::didParseSource( Maybe executionContextAuxDataParam( std::move(executionContextAuxData)); const bool* isLiveEditParam = isLiveEdit ? &isLiveEdit : nullptr; - const bool* hasSourceURLParam = hasSourceURL ? &hasSourceURL : nullptr; + const bool* hasSourceURLParam = + hasSourceURLComment ? &hasSourceURLComment : nullptr; const bool* isModuleParam = isModule ? &isModule : nullptr; std::unique_ptr stack = V8StackTraceImpl::capture(m_inspector->debugger(), contextGroupId, 1); diff --git a/src/inspector/v8-debugger-script.cc b/src/inspector/v8-debugger-script.cc index c40477ae2a..d861265e14 100644 --- a/src/inspector/v8-debugger-script.cc +++ b/src/inspector/v8-debugger-script.cc @@ -6,6 +6,7 @@ #include "src/inspector/inspected-context.h" #include "src/inspector/string-util.h" +#include "src/inspector/v8-inspector-impl.h" #include "src/inspector/wasm-translation.h" #include "src/utils.h" @@ -110,9 +111,9 @@ class ActualScript : public V8DebuggerScript { public: ActualScript(v8::Isolate* isolate, v8::Local script, - bool isLiveEdit) + bool isLiveEdit, V8InspectorClient* client) : V8DebuggerScript(isolate, String16::fromInteger(script->Id()), - GetNameOrSourceUrl(script)), + GetScriptURL(script, client)), m_isLiveEdit(isLiveEdit) { Initialize(script); } @@ -218,10 +219,18 @@ class ActualScript : public V8DebuggerScript { } private: - String16 GetNameOrSourceUrl(v8::Local script) { - v8::Local name; - if (script->Name().ToLocal(&name) || script->SourceURL().ToLocal(&name)) - return toProtocolString(name); + String16 GetScriptURL(v8::Local script, + V8InspectorClient* client) { + v8::Local sourceURL; + if (script->SourceURL().ToLocal(&sourceURL) && sourceURL->Length() > 0) + return toProtocolString(sourceURL); + v8::Local v8Name; + if (script->Name().ToLocal(&v8Name) && v8Name->Length() > 0) { + String16 name = toProtocolString(v8Name); + std::unique_ptr url = + client->resourceNameToUrl(toStringView(name)); + return url ? toString16(url->string()) : name; + } return String16(); } @@ -231,7 +240,8 @@ class ActualScript : public V8DebuggerScript { void Initialize(v8::Local script) { v8::Local tmp; - if (script->SourceURL().ToLocal(&tmp)) m_sourceURL = toProtocolString(tmp); + m_hasSourceURLComment = + script->SourceURL().ToLocal(&tmp) && tmp->Length() > 0; if (script->SourceMappingURL().ToLocal(&tmp)) m_sourceMappingURL = toProtocolString(tmp); m_startLine = script->LineOffset(); @@ -398,9 +408,9 @@ class WasmVirtualScript : public V8DebuggerScript { std::unique_ptr V8DebuggerScript::Create( v8::Isolate* isolate, v8::Local scriptObj, - bool isLiveEdit) { + bool isLiveEdit, V8InspectorClient* client) { return std::unique_ptr( - new ActualScript(isolate, scriptObj, isLiveEdit)); + new ActualScript(isolate, scriptObj, isLiveEdit, client)); } std::unique_ptr V8DebuggerScript::CreateWasm( @@ -418,12 +428,11 @@ V8DebuggerScript::V8DebuggerScript(v8::Isolate* isolate, String16 id, V8DebuggerScript::~V8DebuggerScript() {} -const String16& V8DebuggerScript::sourceURL() const { - return m_sourceURL.isEmpty() ? m_url : m_sourceURL; -} - void V8DebuggerScript::setSourceURL(const String16& sourceURL) { - m_sourceURL = sourceURL; + if (sourceURL.length() > 0) { + m_hasSourceURLComment = true; + m_url = sourceURL; + } } bool V8DebuggerScript::setBreakpoint(const String16& condition, @@ -431,5 +440,4 @@ bool V8DebuggerScript::setBreakpoint(const String16& condition, v8::HandleScope scope(m_isolate); return script()->SetBreakpoint(toV8String(m_isolate, condition), loc, id); } - } // namespace v8_inspector diff --git a/src/inspector/v8-debugger-script.h b/src/inspector/v8-debugger-script.h index e0e7d93b20..38e6448f48 100644 --- a/src/inspector/v8-debugger-script.h +++ b/src/inspector/v8-debugger-script.h @@ -40,13 +40,14 @@ namespace v8_inspector { // Forward declaration. +class V8InspectorClient; class WasmTranslation; class V8DebuggerScript { public: static std::unique_ptr Create( v8::Isolate* isolate, v8::Local script, - bool isLiveEdit); + bool isLiveEdit, V8InspectorClient* client); static std::unique_ptr CreateWasm( v8::Isolate* isolate, WasmTranslation* wasmTranslation, v8::Local underlyingScript, String16 id, @@ -55,9 +56,9 @@ class V8DebuggerScript { virtual ~V8DebuggerScript(); const String16& scriptId() const { return m_id; } - const String16& url() const { return m_url; } - bool hasSourceURL() const { return !m_sourceURL.isEmpty(); } - const String16& sourceURL() const; + bool hasSourceURLComment() const { return m_hasSourceURLComment; } + const String16& sourceURL() const { return m_url; } + virtual const String16& sourceMappingURL() const = 0; virtual const String16& source() const = 0; virtual const String16& hash() const = 0; @@ -95,7 +96,7 @@ class V8DebuggerScript { String16 m_id; String16 m_url; - String16 m_sourceURL; + bool m_hasSourceURLComment = false; int m_executionContextId = 0; v8::Isolate* m_isolate; diff --git a/src/inspector/v8-debugger.cc b/src/inspector/v8-debugger.cc index 1ceb4210f7..7d1f7cefd1 100644 --- a/src/inspector/v8-debugger.cc +++ b/src/inspector/v8-debugger.cc @@ -226,13 +226,15 @@ void V8Debugger::getCompiledScripts( v8::Local script = scripts.Get(i); if (!script->WasCompiled()) continue; if (script->IsEmbedded()) { - result.push_back(V8DebuggerScript::Create(m_isolate, script, false)); + result.push_back(V8DebuggerScript::Create(m_isolate, script, false, + m_inspector->client())); continue; } int contextId; if (!script->ContextId().To(&contextId)) continue; if (m_inspector->contextGroupId(contextId) != contextGroupId) continue; - result.push_back(V8DebuggerScript::Create(m_isolate, script, false)); + result.push_back(V8DebuggerScript::Create(m_isolate, script, false, + m_inspector->client())); } } @@ -585,13 +587,14 @@ void V8Debugger::ScriptCompiled(v8::Local script, }); } else if (m_ignoreScriptParsedEventsCounter == 0) { v8::Isolate* isolate = m_isolate; + V8InspectorClient* client = m_inspector->client(); m_inspector->forEachSession( m_inspector->contextGroupId(contextId), - [&isolate, &script, &has_compile_error, - &is_live_edited](V8InspectorSessionImpl* session) { + [&isolate, &script, &has_compile_error, &is_live_edited, + &client](V8InspectorSessionImpl* session) { if (!session->debuggerAgent()->enabled()) return; session->debuggerAgent()->didParseSource( - V8DebuggerScript::Create(isolate, script, is_live_edited), + V8DebuggerScript::Create(isolate, script, is_live_edited, client), !has_compile_error); }); } diff --git a/src/inspector/v8-profiler-agent-impl.cc b/src/inspector/v8-profiler-agent-impl.cc index 59a99d79d5..f14815fdc4 100644 --- a/src/inspector/v8-profiler-agent-impl.cc +++ b/src/inspector/v8-profiler-agent-impl.cc @@ -7,6 +7,7 @@ #include #include "src/base/atomicops.h" +#include "src/debug/debug-interface.h" #include "src/flags.h" // TODO(jgruber): Remove include and DEPS entry. #include "src/inspector/protocol/Protocol.h" #include "src/inspector/string-util.h" @@ -31,6 +32,15 @@ static const char typeProfileStarted[] = "typeProfileStarted"; namespace { +String16 resourceNameToUrl(V8InspectorImpl* inspector, + v8::Local v8Name) { + String16 name = toProtocolString(v8Name); + if (!inspector) return name; + std::unique_ptr url = + inspector->client()->resourceNameToUrl(toStringView(name)); + return url ? toString16(url->string()) : name; +} + std::unique_ptr> buildInspectorObjectForPositionTicks(const v8::CpuProfileNode* node) { unsigned lineCount = node->GetHitLineCount(); @@ -51,13 +61,14 @@ buildInspectorObjectForPositionTicks(const v8::CpuProfileNode* node) { } std::unique_ptr buildInspectorObjectFor( - v8::Isolate* isolate, const v8::CpuProfileNode* node) { + V8InspectorImpl* inspector, const v8::CpuProfileNode* node) { + v8::Isolate* isolate = inspector->isolate(); v8::HandleScope handleScope(isolate); auto callFrame = protocol::Runtime::CallFrame::create() .setFunctionName(toProtocolString(node->GetFunctionName())) .setScriptId(String16::fromInteger(node->GetScriptId())) - .setUrl(toProtocolString(node->GetScriptResourceName())) + .setUrl(resourceNameToUrl(inspector, node->GetScriptResourceName())) .setLineNumber(node->GetLineNumber() - 1) .setColumnNumber(node->GetColumnNumber() - 1) .build(); @@ -107,18 +118,19 @@ std::unique_ptr> buildInspectorObjectForTimestamps( return array; } -void flattenNodesTree(v8::Isolate* isolate, const v8::CpuProfileNode* node, +void flattenNodesTree(V8InspectorImpl* inspector, + const v8::CpuProfileNode* node, protocol::Array* list) { - list->addItem(buildInspectorObjectFor(isolate, node)); + list->addItem(buildInspectorObjectFor(inspector, node)); const int childrenCount = node->GetChildrenCount(); for (int i = 0; i < childrenCount; i++) - flattenNodesTree(isolate, node->GetChild(i), list); + flattenNodesTree(inspector, node->GetChild(i), list); } std::unique_ptr createCPUProfile( - v8::Isolate* isolate, v8::CpuProfile* v8profile) { + V8InspectorImpl* inspector, v8::CpuProfile* v8profile) { auto nodes = protocol::Array::create(); - flattenNodesTree(isolate, v8profile->GetTopDownRoot(), nodes.get()); + flattenNodesTree(inspector, v8profile->GetTopDownRoot(), nodes.get()); return protocol::Profiler::Profile::create() .setNodes(std::move(nodes)) .setStartTime(static_cast(v8profile->GetStartTime())) @@ -320,7 +332,7 @@ std::unique_ptr createCoverageRange( } Response coverageToProtocol( - v8::Isolate* isolate, const v8::debug::Coverage& coverage, + V8InspectorImpl* inspector, const v8::debug::Coverage& coverage, std::unique_ptr>* out_result) { std::unique_ptr> result = @@ -361,8 +373,10 @@ Response coverageToProtocol( } String16 url; v8::Local name; - if (script->Name().ToLocal(&name) || script->SourceURL().ToLocal(&name)) { + if (script->SourceURL().ToLocal(&name) && name->Length()) { url = toProtocolString(name); + } else if (script->Name().ToLocal(&name) && name->Length()) { + url = resourceNameToUrl(inspector, name); } result->addItem(protocol::Profiler::ScriptCoverage::create() .setScriptId(String16::fromInteger(script->Id())) @@ -384,7 +398,7 @@ Response V8ProfilerAgentImpl::takePreciseCoverage( } v8::HandleScope handle_scope(m_isolate); v8::debug::Coverage coverage = v8::debug::Coverage::CollectPrecise(m_isolate); - return coverageToProtocol(m_isolate, coverage, out_result); + return coverageToProtocol(m_session->inspector(), coverage, out_result); } Response V8ProfilerAgentImpl::getBestEffortCoverage( @@ -393,12 +407,12 @@ Response V8ProfilerAgentImpl::getBestEffortCoverage( v8::HandleScope handle_scope(m_isolate); v8::debug::Coverage coverage = v8::debug::Coverage::CollectBestEffort(m_isolate); - return coverageToProtocol(m_isolate, coverage, out_result); + return coverageToProtocol(m_session->inspector(), coverage, out_result); } namespace { std::unique_ptr> -typeProfileToProtocol(v8::Isolate* isolate, +typeProfileToProtocol(V8InspectorImpl* inspector, const v8::debug::TypeProfile& type_profile) { std::unique_ptr> result = protocol::Array::create(); @@ -426,8 +440,10 @@ typeProfileToProtocol(v8::Isolate* isolate, } String16 url; v8::Local name; - if (script->Name().ToLocal(&name) || script->SourceURL().ToLocal(&name)) { + if (script->SourceURL().ToLocal(&name) && name->Length()) { url = toProtocolString(name); + } else if (script->Name().ToLocal(&name) && name->Length()) { + url = resourceNameToUrl(inspector, name); } result->addItem(protocol::Profiler::ScriptTypeProfile::create() .setScriptId(String16::fromInteger(script->Id())) @@ -462,7 +478,7 @@ Response V8ProfilerAgentImpl::takeTypeProfile( v8::HandleScope handle_scope(m_isolate); v8::debug::TypeProfile type_profile = v8::debug::TypeProfile::Collect(m_isolate); - *out_result = typeProfileToProtocol(m_isolate, type_profile); + *out_result = typeProfileToProtocol(m_session->inspector(), type_profile); return Response::OK(); } @@ -491,7 +507,7 @@ std::unique_ptr V8ProfilerAgentImpl::stopProfiling( m_profiler->StopProfiling(toV8String(m_isolate, title)); std::unique_ptr result; if (profile) { - if (serialize) result = createCPUProfile(m_isolate, profile); + if (serialize) result = createCPUProfile(m_session->inspector(), profile); profile->Delete(); } --m_startedProfilesCount; diff --git a/src/inspector/v8-stack-trace-impl.cc b/src/inspector/v8-stack-trace-impl.cc index 75293c59af..9be0d4fa38 100644 --- a/src/inspector/v8-stack-trace-impl.cc +++ b/src/inspector/v8-stack-trace-impl.cc @@ -7,6 +7,7 @@ #include #include "src/inspector/v8-debugger.h" +#include "src/inspector/v8-inspector-impl.h" #include "src/inspector/wasm-translation.h" namespace v8_inspector { @@ -73,7 +74,10 @@ std::unique_ptr buildInspectorObjectCommon( std::unique_ptr> inspectorFrames = protocol::Array::create(); for (size_t i = 0; i < frames.size(); i++) { - inspectorFrames->addItem(frames[i]->buildInspectorObject()); + V8InspectorClient* client = nullptr; + if (debugger && debugger->inspector()) + client = debugger->inspector()->client(); + inspectorFrames->addItem(frames[i]->buildInspectorObject(client)); } std::unique_ptr stackTrace = protocol::Runtime::StackTrace::create() @@ -117,7 +121,9 @@ StackFrame::StackFrame(v8::Local v8Frame) m_scriptId(String16::fromInteger(v8Frame->GetScriptId())), m_sourceURL(toProtocolString(v8Frame->GetScriptNameOrSourceURL())), m_lineNumber(v8Frame->GetLineNumber() - 1), - m_columnNumber(v8Frame->GetColumn() - 1) { + m_columnNumber(v8Frame->GetColumn() - 1), + m_hasSourceURLComment(v8Frame->GetScriptName() != + v8Frame->GetScriptNameOrSourceURL()) { DCHECK_NE(v8::Message::kNoLineNumberInfo, m_lineNumber + 1); DCHECK_NE(v8::Message::kNoColumnInfo, m_columnNumber + 1); } @@ -137,12 +143,20 @@ int StackFrame::lineNumber() const { return m_lineNumber; } int StackFrame::columnNumber() const { return m_columnNumber; } -std::unique_ptr StackFrame::buildInspectorObject() - const { +std::unique_ptr StackFrame::buildInspectorObject( + V8InspectorClient* client) const { + String16 frameUrl = m_sourceURL; + if (client && !m_hasSourceURLComment && frameUrl.length() > 0) { + std::unique_ptr url = + client->resourceNameToUrl(toStringView(m_sourceURL)); + if (url) { + frameUrl = toString16(url->string()); + } + } return protocol::Runtime::CallFrame::create() .setFunctionName(m_functionName) .setScriptId(m_scriptId) - .setUrl(m_sourceURL) + .setUrl(frameUrl) .setLineNumber(m_lineNumber) .setColumnNumber(m_columnNumber) .build(); diff --git a/src/inspector/v8-stack-trace-impl.h b/src/inspector/v8-stack-trace-impl.h index a8f23c48b6..019fd469cd 100644 --- a/src/inspector/v8-stack-trace-impl.h +++ b/src/inspector/v8-stack-trace-impl.h @@ -33,7 +33,8 @@ class StackFrame { const String16& sourceURL() const; int lineNumber() const; // 0-based. int columnNumber() const; // 0-based. - std::unique_ptr buildInspectorObject() const; + std::unique_ptr buildInspectorObject( + V8InspectorClient* client) const; bool isEqual(StackFrame* frame) const; private: @@ -42,6 +43,7 @@ class StackFrame { String16 m_sourceURL; int m_lineNumber; // 0-based. int m_columnNumber; // 0-based. + bool m_hasSourceURLComment; }; class V8StackTraceImpl : public V8StackTrace { diff --git a/test/inspector/debugger/resource-name-to-url-expected.txt b/test/inspector/debugger/resource-name-to-url-expected.txt new file mode 100644 index 0000000000..0ecd0b82ef --- /dev/null +++ b/test/inspector/debugger/resource-name-to-url-expected.txt @@ -0,0 +1,122 @@ +Tests V8InspectorClient::resourceNameToUrl. +Check script with url: +{ + method : Debugger.scriptParsed + params : { + endColumn : 16 + endLine : 0 + executionContextId : + hasSourceURL : false + hash : 033b33d191ed51ed823355d865eb871d811403e2 + isLiveEdit : false + isModule : false + length : 16 + scriptId : + sourceMapURL : + startColumn : 0 + startLine : 0 + url : prefix://url + } +} +Check script with sourceURL comment: +{ + method : Debugger.scriptParsed + params : { + endColumn : 37 + endLine : 0 + executionContextId : + hasSourceURL : true + hash : 06c136ce206c5f505f32af524e6ec71b5baa0bbb + isLiveEdit : false + isModule : false + length : 37 + scriptId : + sourceMapURL : + startColumn : 0 + startLine : 0 + url : foo.js + } +} +Check script failed to parse: +{ + method : Debugger.scriptFailedToParse + params : { + endColumn : 15 + endLine : 0 + executionContextId : + hasSourceURL : false + hash : 033b33d191ed51ed1f44cd0465eb871d811403e2 + isModule : false + length : 15 + scriptId : + sourceMapURL : + startColumn : 0 + startLine : 0 + url : prefix://url + } +} +Check script failed to parse with sourceURL comment: +{ + method : Debugger.scriptFailedToParse + params : { + endColumn : 36 + endLine : 0 + executionContextId : + hasSourceURL : true + hash : 23a2885951475580023e2a742563d78876d8f05e + isModule : false + length : 36 + scriptId : + sourceMapURL : + startColumn : 0 + startLine : 0 + url : foo.js + } +} +Test runtime stack trace: +{ + method : Runtime.consoleAPICalled + params : { + args : [ + [0] : { + description : 42 + type : number + value : 42 + } + ] + executionContextId : + stackTrace : { + callFrames : [ + [0] : { + columnNumber : 14 + functionName : foo + lineNumber : 2 + scriptId : + url : prefix://url + } + [1] : { + columnNumber : 0 + functionName : + lineNumber : 0 + scriptId : + url : boo.js + } + [2] : { + columnNumber : 4 + functionName : + lineNumber : 4 + scriptId : + url : prefix://url + } + ] + } + timestamp : + type : log + } +} +Test debugger stack trace: +[ + [0] : prefix://url + [1] : boo.js + [2] : prefix://url +] diff --git a/test/inspector/debugger/resource-name-to-url.js b/test/inspector/debugger/resource-name-to-url.js new file mode 100644 index 0000000000..620c7a2864 --- /dev/null +++ b/test/inspector/debugger/resource-name-to-url.js @@ -0,0 +1,49 @@ +// Copyright 2018 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +let {session, contextGroup, Protocol} = InspectorTest.start( + 'Tests V8InspectorClient::resourceNameToUrl.'); + +(async function test(){ + Protocol.Runtime.enable(); + await Protocol.Debugger.enable(); + contextGroup.addScript(`inspector.setResourceNamePrefix('prefix://')`); + await Protocol.Debugger.onceScriptParsed(); + + InspectorTest.log('Check script with url:'); + contextGroup.addScript('function foo(){}', 0, 0, 'url'); + InspectorTest.logMessage(await Protocol.Debugger.onceScriptParsed()); + + InspectorTest.log('Check script with sourceURL comment:'); + contextGroup.addScript('function foo(){} //# sourceURL=foo.js', 0, 0, 'url'); + InspectorTest.logMessage(await Protocol.Debugger.onceScriptParsed()); + + InspectorTest.log('Check script failed to parse:'); + contextGroup.addScript('function foo(){', 0, 0, 'url'); + InspectorTest.logMessage(await Protocol.Debugger.onceScriptFailedToParse()); + + InspectorTest.log('Check script failed to parse with sourceURL comment:'); + contextGroup.addScript('function foo(){ //# sourceURL=foo.js', 0, 0, 'url'); + InspectorTest.logMessage(await Protocol.Debugger.onceScriptFailedToParse()); + + InspectorTest.log('Test runtime stack trace:'); + contextGroup.addScript(` + function foo() { + console.log(42); + } + eval('foo(); //# sourceURL=boo.js'); + `, 0, 0, 'url'); + InspectorTest.logMessage(await Protocol.Runtime.onceConsoleAPICalled()); + + InspectorTest.log('Test debugger stack trace:'); + contextGroup.addScript(` + function foo() { + debugger; + } + eval('foo(); //# sourceURL=boo.js'); + `, 0, 0, 'url'); + const {params:{callFrames}} = await Protocol.Debugger.oncePaused(); + InspectorTest.logMessage(callFrames.map(frame => frame.url)); + InspectorTest.completeTest(); +})(); diff --git a/test/inspector/inspector-test.cc b/test/inspector/inspector-test.cc index 668a9463d5..93a8b1d3f2 100644 --- a/test/inspector/inspector-test.cc +++ b/test/inspector/inspector-test.cc @@ -712,6 +712,9 @@ class InspectorExtension : public IsolateData::SetupGlobalTask { ToV8String(isolate, "setAllowCodeGenerationFromStrings"), v8::FunctionTemplate::New( isolate, &InspectorExtension::SetAllowCodeGenerationFromStrings)); + inspector->Set(ToV8String(isolate, "setResourceNamePrefix"), + v8::FunctionTemplate::New( + isolate, &InspectorExtension::SetResourceNamePrefix)); global->Set(ToV8String(isolate, "inspector"), inspector); } @@ -973,6 +976,18 @@ class InspectorExtension : public IsolateData::SetupGlobalTask { args.GetIsolate()->GetCurrentContext()->AllowCodeGenerationFromStrings( args[0].As()->Value()); } + + static void SetResourceNamePrefix( + const v8::FunctionCallbackInfo& args) { + if (args.Length() != 1 || !args[0]->IsString()) { + fprintf(stderr, "Internal error: setResourceNamePrefix('prefix')."); + Exit(); + } + v8::Isolate* isolate = args.GetIsolate(); + v8::Local context = isolate->GetCurrentContext(); + IsolateData* data = IsolateData::FromContext(context); + data->SetResourceNamePrefix(v8::Local::Cast(args[0])); + } }; bool RunExtraCode(v8::Isolate* isolate, v8::Local context, diff --git a/test/inspector/isolate-data.cc b/test/inspector/isolate-data.cc index 15eee89a61..a669cc41a1 100644 --- a/test/inspector/isolate-data.cc +++ b/test/inspector/isolate-data.cc @@ -423,3 +423,32 @@ void IsolateData::maxAsyncCallStackDepthChanged(int depth) { if (!log_max_async_call_stack_depth_changed_) return; fprintf(stdout, "maxAsyncCallStackDepthChanged: %d\n", depth); } + +void IsolateData::SetResourceNamePrefix(v8::Local prefix) { + resource_name_prefix_.Reset(v8::Isolate::GetCurrent(), prefix); +} + +namespace { +class StringBufferImpl : public v8_inspector::StringBuffer { + public: + StringBufferImpl(v8::Isolate* isolate, v8::Local string) + : data_(ToVector(string)), + view_(data_.start(), data_.length()) {} + const v8_inspector::StringView& string() override { return view_; } + + private: + v8::internal::Vector data_; + v8_inspector::StringView view_; +}; +} // anonymous namespace + +std::unique_ptr IsolateData::resourceNameToUrl( + const v8_inspector::StringView& resourceName) { + if (resource_name_prefix_.IsEmpty()) return nullptr; + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope handle_scope(isolate); + v8::Local name = ToString(isolate, resourceName); + v8::Local prefix = resource_name_prefix_.Get(isolate); + v8::Local url = v8::String::Concat(prefix, name); + return std::unique_ptr(new StringBufferImpl(isolate, url)); +} diff --git a/test/inspector/isolate-data.h b/test/inspector/isolate-data.h index 5eb9803a74..d0a263e573 100644 --- a/test/inspector/isolate-data.h +++ b/test/inspector/isolate-data.h @@ -76,6 +76,7 @@ class IsolateData : public v8_inspector::V8InspectorClient { void FireContextCreated(v8::Local context, int context_group_id); void FireContextDestroyed(v8::Local context); void FreeContext(v8::Local context); + void SetResourceNamePrefix(v8::Local prefix); private: struct VectorCompare { @@ -114,6 +115,8 @@ class IsolateData : public v8_inspector::V8InspectorClient { v8_inspector::V8StackTrace*) override; bool isInspectableHeapObject(v8::Local) override; void maxAsyncCallStackDepthChanged(int depth) override; + std::unique_ptr resourceNameToUrl( + const v8_inspector::StringView& resourceName) override; // The isolate gets deleted by its {Dispose} method, not by the default // deleter. Therefore we have to define a custom deleter for the unique_ptr to @@ -141,6 +144,7 @@ class IsolateData : public v8_inspector::V8InspectorClient { bool log_console_api_message_calls_ = false; bool log_max_async_call_stack_depth_changed_ = false; v8::Global not_inspectable_private_; + v8::Global resource_name_prefix_; DISALLOW_COPY_AND_ASSIGN(IsolateData); }; -- 2.17.0 From dafaa6ecb520df3761ed68115ee3e6b102963092 Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Mon, 17 Sep 2018 07:43:46 +0200 Subject: [PATCH 11/11] deps: add missing HandleScope in FieldType::PrintTo Refs: https://github.com/nodejs/node/issues/22775 PR-URL: https://github.com/nodejs/node/pull/22890 Reviewed-By: Ruben Bridgewater Reviewed-By: Anna Henningsen Reviewed-By: Refael Ackermann --- src/field-type.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/field-type.cc b/src/field-type.cc index 3b51095323..2eebebe3d6 100644 --- a/src/field-type.cc +++ b/src/field-type.cc @@ -78,6 +78,7 @@ void FieldType::PrintTo(std::ostream& os) { os << "None"; } else { DCHECK(IsClass()); + HandleScope scope(Map::cast(this)->GetIsolate()); os << "Class(" << static_cast(*AsClass()) << ")"; } } -- 2.17.0