From 6f6a540456794ab39fb71f711ff7bee34ba4d7f9 Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Tue, 29 Dec 2020 10:03:46 +0100 Subject: [PATCH] [wasm][debug] Compile source only once for Debug-Evaluate. We accidentally compiled the source for Debug-Evaluate on Wasm frames twice, and used the first script result as outer function info for the second, actual compile (which goes via the `eval` machinery). Also unify DebugEvaluate::Local and DebugEvaluate::WebAssembly, since they are essentially very similar, except for the context. Bug: chromium:1127914 Change-Id: I8eb85c128c05ab1d4fcb73061de89b0942d1e5c8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2605198 Commit-Queue: Benedikt Meurer Auto-Submit: Benedikt Meurer Reviewed-by: Yang Guo Cr-Commit-Position: refs/heads/master@{#71890} --- src/debug/debug-evaluate.cc | 79 +++++++++---------------- src/debug/debug-evaluate.h | 7 +-- src/debug/debug-stack-trace-iterator.cc | 21 ++----- 3 files changed, 36 insertions(+), 71 deletions(-) diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc index 832de6a58f..1b96233a16 100644 --- a/src/debug/debug-evaluate.cc +++ b/src/debug/debug-evaluate.cc @@ -78,58 +78,37 @@ MaybeHandle DebugEvaluate::Local(Isolate* isolate, // Get the frame where the debugging is performed. StackTraceFrameIterator it(isolate, frame_id); - if (!it.is_javascript()) return isolate->factory()->undefined_value(); - JavaScriptFrame* frame = it.javascript_frame(); + if (it.is_javascript()) { + JavaScriptFrame* frame = it.javascript_frame(); + // This is not a lot different than DebugEvaluate::Global, except that + // variables accessible by the function we are evaluating from are + // materialized and included on top of the native context. Changes to + // the materialized object are written back afterwards. + // Note that the native context is taken from the original context chain, + // which may not be the current native context of the isolate. + ContextBuilder context_builder(isolate, frame, inlined_jsframe_index); + if (isolate->has_pending_exception()) return {}; - // This is not a lot different than DebugEvaluate::Global, except that - // variables accessible by the function we are evaluating from are - // materialized and included on top of the native context. Changes to - // the materialized object are written back afterwards. - // Note that the native context is taken from the original context chain, - // which may not be the current native context of the isolate. - ContextBuilder context_builder(isolate, frame, inlined_jsframe_index); - if (isolate->has_pending_exception()) return MaybeHandle(); - - Handle context = context_builder.evaluation_context(); - Handle receiver(context->global_proxy(), isolate); - MaybeHandle maybe_result = - Evaluate(isolate, context_builder.outer_info(), context, receiver, source, - throw_on_side_effect); - if (!maybe_result.is_null()) context_builder.UpdateValues(); - return maybe_result; -} - -V8_EXPORT MaybeHandle DebugEvaluate::WebAssembly( - Handle instance, StackFrameId frame_id, - Handle source, bool throw_on_side_effect) { - Isolate* isolate = instance->GetIsolate(); - - StackTraceFrameIterator it(isolate, frame_id); - if (!it.is_wasm()) return isolate->factory()->undefined_value(); - WasmFrame* frame = WasmFrame::cast(it.frame()); - - Handle context_extension = WasmJs::GetJSDebugProxy(frame); - - DisableBreak disable_break_scope(isolate->debug(), /*disable=*/true); - - Handle shared_info; - if (!GetFunctionInfo(isolate, source, REPLMode::kNo).ToHandle(&shared_info)) { - return {}; + Handle context = context_builder.evaluation_context(); + Handle receiver(context->global_proxy(), isolate); + MaybeHandle maybe_result = + Evaluate(isolate, context_builder.outer_info(), context, receiver, + source, throw_on_side_effect); + if (!maybe_result.is_null()) context_builder.UpdateValues(); + return maybe_result; + } else { + CHECK(it.is_wasm()); + WasmFrame* frame = WasmFrame::cast(it.frame()); + Handle outer_info( + isolate->native_context()->empty_function().shared(), isolate); + Handle context_extension = WasmJs::GetJSDebugProxy(frame); + Handle scope_info = + ScopeInfo::CreateForWithScope(isolate, Handle::null()); + Handle context = isolate->factory()->NewWithContext( + isolate->native_context(), scope_info, context_extension); + return Evaluate(isolate, outer_info, context, context_extension, source, + throw_on_side_effect); } - - Handle scope_info = - ScopeInfo::CreateForWithScope(isolate, Handle::null()); - Handle context = isolate->factory()->NewWithContext( - isolate->native_context(), scope_info, context_extension); - - Handle result; - if (!DebugEvaluate::Evaluate(isolate, shared_info, context, context_extension, - source, throw_on_side_effect) - .ToHandle(&result)) { - return {}; - } - - return result; } MaybeHandle DebugEvaluate::WithTopmostArguments(Isolate* isolate, diff --git a/src/debug/debug-evaluate.h b/src/debug/debug-evaluate.h index 2f4cc2da4e..03836dd6da 100644 --- a/src/debug/debug-evaluate.h +++ b/src/debug/debug-evaluate.h @@ -32,15 +32,14 @@ class DebugEvaluate : public AllStatic { // - Parameters and stack-allocated locals need to be materialized. Altered // values need to be written back to the stack afterwards. // - The arguments object needs to materialized. + // The stack frame can be either a JavaScript stack frame or a Wasm + // stack frame. In the latter case, a special Debug Proxy API is + // provided to peek into the Wasm state. static MaybeHandle Local(Isolate* isolate, StackFrameId frame_id, int inlined_jsframe_index, Handle source, bool throw_on_side_effect); - static V8_EXPORT MaybeHandle WebAssembly( - Handle instance, StackFrameId frame_id, - Handle source, bool throw_on_side_effect); - // This is used for break-at-entry for builtins and API functions. // Evaluate a piece of JavaScript in the native context, but with the // materialized arguments object and receiver of the current call. diff --git a/src/debug/debug-stack-trace-iterator.cc b/src/debug/debug-stack-trace-iterator.cc index f482c51172..2a47bb6f97 100644 --- a/src/debug/debug-stack-trace-iterator.cc +++ b/src/debug/debug-stack-trace-iterator.cc @@ -178,23 +178,10 @@ v8::MaybeLocal DebugStackTraceIterator::Evaluate( Handle value; i::SafeForInterruptsScope safe_for_interrupt_scope(isolate_); - bool success = false; - if (iterator_.is_wasm()) { - FrameSummary summary = FrameSummary::Get(iterator_.frame(), 0); - const FrameSummary::WasmFrameSummary& wasmSummary = summary.AsWasm(); - Handle instance = wasmSummary.wasm_instance(); - - success = DebugEvaluate::WebAssembly(instance, iterator_.frame()->id(), - Utils::OpenHandle(*source), - throw_on_side_effect) - .ToHandle(&value); - } else { - success = DebugEvaluate::Local( - isolate_, iterator_.frame()->id(), inlined_frame_index_, - Utils::OpenHandle(*source), throw_on_side_effect) - .ToHandle(&value); - } - if (!success) { + if (!DebugEvaluate::Local(isolate_, iterator_.frame()->id(), + inlined_frame_index_, Utils::OpenHandle(*source), + throw_on_side_effect) + .ToHandle(&value)) { isolate_->OptionalRescheduleException(false); return v8::MaybeLocal(); }