AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

Revert "[debug] Immediately step-in for 'stack check triggered' debug breaks"

Browse Source 
This reverts commit 3297ccca23.

Reason for revert: V8 roll is failing https://luci-milo.appspot.com/ui/inv/build-8804330987023399745/test-results?q=DevToolsTest.TestPauseWhenScriptIsRunning

Original change's description:
> [debug] Immediately step-in for 'stack check triggered' debug breaks
>
> This CL changes debug breaks that are triggered via interrupts (i.e.
> via stack check). One client of this behavior is the `Debugger.pause`
> CDP method.
>
> The problem is that when we pause so early, the JSFunction didn't have
> time yet to create and push it's context. This requires special
> handling in the ScopeIterator and makes an upcoming change unnecessary
> complex.
>
> Another (minor) problem is that local debug-evaluate can't change
> context-allocated local variables (see changed regression bug). Since
> the context is not yet created and pushed, variables are written to
> the DebugEvaluateContext that goes away after the evaluation.
>
> The solution is to mirror what `BreakOnNextFunction` does. Instead
> of staying paused in the middle of the function entry, we trigger
> a "step in" and pause at the first valid breakable position instead.
> This ensures that the function context is already created and pushed.
>
> Note that we do this only in case for JSFunctions. In all other cases
> we keep the existing behavior and stay paused in the entry.
>
> R=​jgruber@chromium.org
>
> Fixed: chromium:1246907
> Change-Id: I0cd8ae6e049a3b55bdd44858e769682a1ca47064
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3854501
> Reviewed-by: Jakob Linke <jgruber@chromium.org>
> Commit-Queue: Simon Zünd <szuend@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#82817}

Change-Id: I0c34b7b4a788572a73ca380b3d767223fb6e7ea1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3867311
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82855}
This commit is contained in:
Leszek Swirski 2022-08-31 09:36:13 +00:00 committed by V8 LUCI CQ
parent 22485d7c45
commit 5e6278b2bd
4 changed files with 16 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
src/debug/debug.cc
View File

@ -387,7 +387,6 @@ void Debug::ThreadInit() {
base::Relaxed_Store(&thread_local_.current_debug_scope_,
static_cast<base::AtomicWord>(0));
thread_local_.break_on_next_function_call_ = false;
thread_local_.scheduled_break_on_next_function_call_ = false;
UpdateHookOnFunctionCall();
thread_local_.promise_stack_ = Smi::zero();
}
@ -514,22 +513,15 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
bool has_break_points;
MaybeHandle<FixedArray> break_points_hit =
CheckBreakPoints(debug_info, &location, &has_break_points);
if (!break_points_hit.is_null() || break_on_next_function_call() ||
scheduled_break_on_function_call()) {
if (!break_points_hit.is_null() || break_on_next_function_call()) {
StepAction lastStepAction = last_step_action();
DCHECK_IMPLIES(scheduled_break_on_function_call(),
lastStepAction == StepNone);
debug::BreakReasons break_reasons;
if (scheduled_break_on_function_call()) {
break_reasons.Add(debug::BreakReason::kScheduled);
}
// Clear all current stepping setup.
ClearStepping();
// Notify the debug event listeners.
OnDebugBreak(!break_points_hit.is_null()
? break_points_hit.ToHandleChecked()
: isolate_->factory()->empty_fixed_array(),
lastStepAction, break_reasons);
lastStepAction);
return;
}
@ -1076,8 +1068,7 @@ void Debug::ClearBreakOnNextFunctionCall() {
void Debug::PrepareStepIn(Handle<JSFunction> function) {
RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
CHECK(last_step_action() >= StepInto || break_on_next_function_call() ||
scheduled_break_on_function_call());
CHECK(last_step_action() >= StepInto || break_on_next_function_call());
if (ignore_events()) return;
if (in_debug_scope()) return;
if (break_disabled()) return;
@ -1387,7 +1378,6 @@ void Debug::ClearStepping() {
thread_local_.last_frame_count_ = -1;
thread_local_.target_frame_count_ = -1;
thread_local_.break_on_next_function_call_ = false;
thread_local_.scheduled_break_on_next_function_call_ = false;
clear_restart_frame();
UpdateHookOnFunctionCall();
}
@ -2504,28 +2494,13 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode,
HandleScope scope(isolate_);
MaybeHandle<FixedArray> break_points;
{
StackTraceFrameIterator it(isolate_);
JavaScriptFrameIterator it(isolate_);
DCHECK(!it.done());
JavaScriptFrame* frame = it.frame()->is_java_script()
? JavaScriptFrame::cast(it.frame())
: nullptr;
if (frame && frame->function().IsJSFunction()) {
Handle<JSFunction> function(frame->function(), isolate_);
Handle<SharedFunctionInfo> shared(function->shared(), isolate_);
// kScheduled breaks are triggered by the stack check. While we could
// pause here, the JSFunction didn't have time yet to create and push
// it's context. Instead, we step into the function and pause at the
// first official breakable position.
// This behavior mirrors "BreakOnNextFunctionCall".
if (break_reasons.contains(v8::debug::BreakReason::kScheduled)) {
CHECK_EQ(last_step_action(), StepAction::StepNone);
thread_local_.scheduled_break_on_next_function_call_ = true;
PrepareStepIn(function);
return;
}
Object fun = it.frame()->function();
if (fun.IsJSFunction()) {
Handle<JSFunction> function(JSFunction::cast(fun), isolate_);
// Don't stop in builtin and blackboxed functions.
Handle<SharedFunctionInfo> shared(function->shared(), isolate_);
bool ignore_break = ignore_break_mode == kIgnoreIfTopFrameBlackboxed
? IsBlackboxed(shared)
: AllFramesOnStackAreBlackboxed();
@ -2537,7 +2512,7 @@ void Debug::HandleDebugBreak(IgnoreBreakMode ignore_break_mode,
DebugScope debug_scope(this);
std::vector<BreakLocation> break_locations;
BreakLocation::AllAtCurrentStatement(debug_info, frame,
BreakLocation::AllAtCurrentStatement(debug_info, it.frame(),
&break_locations);
for (size_t i = 0; i < break_locations.size(); i++) {

9
src/debug/debug.h
View File

@ -395,10 +395,6 @@ class V8_EXPORT_PRIVATE Debug {
return thread_local_.break_on_next_function_call_;
}
bool scheduled_break_on_function_call() const {
return thread_local_.scheduled_break_on_next_function_call_;
}
bool IsRestartFrameScheduled() const {
return thread_local_.restart_frame_id_ != StackFrameId::NO_ID;
}
@ -588,11 +584,6 @@ class V8_EXPORT_PRIVATE Debug {
// debugger to break on next function call.
bool break_on_next_function_call_;
// This flag is true when we break via stack check (BreakReason::kScheduled)
// We don't stay paused there but instead "step in" to the function similar
// to what "BreakOnNextFunctionCall" does.
bool scheduled_break_on_next_function_call_;
// Throwing an exception may cause a Promise rejection. For this purpose
// we keep track of a stack of nested promises.
Object promise_stack_;

6
test/debugger/debug/debug-stack-check-position.js
View File

@ -2,8 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Check that even though we schedule and break on function entry, we still
// pause on the first breakable position.
var Debug = debug.Debug;
var exception = null;
@ -18,8 +16,8 @@ function listener(event, exec_state, event_data, data) {
}
}
function f() {
return 1; // BREAK
function f() { // BREAK
return 1;
}
Debug.setListener(listener);

9
test/debugger/debug/regress/regress-10319.js
View File

@ -17,10 +17,11 @@ Debug.setListener(function (event, exec_state, event_data, data) {
});
function makeCounter() {
// The debug-evaluate should be able to set this to 3. This was fixed with
// https://crbug.com/1246907 which delays debug breaks triggered by stack
// checks until the function context is properly set up.
assertEquals(3, result);
// If the variable `result` were stack-allocated, it would be 3 at this point
// due to the debugging activity during function entry. However, for a
// heap-allocated variable, the debugger evaluated `result = 3` in a temporary
// scope instead and had no effect on this variable.
assertEquals(undefined, result);
var result = 0;