Commit 426eda02 authored by Kim-Anh Tran's avatar Kim-Anh Tran Committed by V8 LUCI CQ

[debugger] Explicitly encode 'other' as a reason when stepping

Previously, we would encode 'other' as a reason for pausing when
stepping too, however, it would not show as such in case it would
overlap with another reason. This CL makes sure that we always report
'other' as a reason if we are stepping.

Drive-by: only encode 'other' as a reason once

Bug: chromium:1229541
Change-Id: Id73822dff68d1d54a2f1fafdf2a097e1377ece75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3295346Reviewed-by: 's avatarJaroslav Sevcik <jarin@chromium.org>
Reviewed-by: 's avatarBenedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78118}
parent 40055508
......@@ -221,6 +221,8 @@ enum ExceptionType { kException, kPromiseRejection };
class DebugDelegate {
public:
// Encodes whether a requested break is (also) due to a step action.
enum StepBreak { kIsStepBreak, kIsNoStepBreak };
virtual ~DebugDelegate() = default;
virtual void ScriptCompiled(v8::Local<Script> script, bool is_live_edited,
bool has_compile_error) {}
......@@ -228,7 +230,8 @@ class DebugDelegate {
// debug::Script::SetBreakpoint API.
virtual void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<debug::BreakpointId>& inspector_break_points_hit) {}
const std::vector<debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) {}
virtual void ExceptionThrown(v8::Local<v8::Context> paused_context,
v8::Local<v8::Value> exception,
v8::Local<v8::Value> promise, bool is_uncaught,
......
......@@ -2152,8 +2152,11 @@ void Debug::OnDebugBreak(Handle<FixedArray> break_points_hit,
{
RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebuggerCallback);
Handle<Context> native_context(isolate_->native_context());
debug_delegate_->BreakProgramRequested(v8::Utils::ToLocal(native_context),
inspector_break_points_hit);
debug_delegate_->BreakProgramRequested(
v8::Utils::ToLocal(native_context), inspector_break_points_hit,
lastStepAction != StepAction::StepNone
? debug::DebugDelegate::StepBreak::kIsStepBreak
: debug::DebugDelegate::StepBreak::kIsNoStepBreak);
}
}
......
......@@ -419,7 +419,8 @@ void GdbServer::DebugDelegate::ScriptCompiled(Local<debug::Script> script,
void GdbServer::DebugDelegate::BreakProgramRequested(
// Executed in the isolate thread.
Local<v8::Context> paused_context,
const std::vector<debug::BreakpointId>& inspector_break_points_hit) {
const std::vector<debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) {
gdb_server_->GetTarget().OnProgramBreak(
isolate_, WasmModuleDebug::GetCallStack(id_, isolate_));
gdb_server_->RunMessageLoopOnPause();
......
......@@ -150,9 +150,10 @@ class GdbServer {
// debug::DebugDelegate
void ScriptCompiled(Local<debug::Script> script, bool is_live_edited,
bool has_compile_error) override;
void BreakProgramRequested(Local<v8::Context> paused_context,
const std::vector<debug::BreakpointId>&
inspector_break_points_hit) override;
void BreakProgramRequested(
Local<v8::Context> paused_context,
const std::vector<debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override;
void ExceptionThrown(Local<v8::Context> paused_context,
Local<Value> exception, Local<Value> promise,
bool is_uncaught,
......
......@@ -382,7 +382,7 @@ void V8DebuggerAgentImpl::enableImpl() {
if (isPaused()) {
didPause(0, v8::Local<v8::Value>(), std::vector<v8::debug::BreakpointId>(),
v8::debug::kException, false, false, false);
v8::debug::kException, false, false, false, false);
}
}
......@@ -1745,7 +1745,7 @@ void V8DebuggerAgentImpl::didPause(
int contextId, v8::Local<v8::Value> exception,
const std::vector<v8::debug::BreakpointId>& hitBreakpoints,
v8::debug::ExceptionType exceptionType, bool isUncaught, bool isOOMBreak,
bool isAssert) {
bool isAssert, bool isStepAction) {
v8::HandleScope handles(m_isolate);
std::vector<BreakReason> hitReasons;
......@@ -1816,15 +1816,21 @@ void V8DebuggerAgentImpl::didPause(
}
}
if (hitRegularBreakpoint) {
hitReasons.push_back(
std::make_pair(protocol::Debugger::Paused::ReasonEnum::Other, nullptr));
}
for (size_t i = 0; i < m_breakReason.size(); ++i) {
hitReasons.push_back(std::move(m_breakReason[i]));
}
clearBreakDetails();
// Make sure that we only include (other: nullptr) once.
const BreakReason otherHitReason =
std::make_pair(protocol::Debugger::Paused::ReasonEnum::Other, nullptr);
if ((hitRegularBreakpoint || isStepAction) &&
std::find(hitReasons.begin(), hitReasons.end(), otherHitReason) ==
hitReasons.end()) {
hitReasons.push_back(
std::make_pair(protocol::Debugger::Paused::ReasonEnum::Other, nullptr));
}
// TODO(kimanh): We should always know why we pause. Print a
// warning if we don't and fall back to Other.
String16 breakReason = protocol::Debugger::Paused::ReasonEnum::Other;
......
......@@ -152,7 +152,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
void didPause(int contextId, v8::Local<v8::Value> exception,
const std::vector<v8::debug::BreakpointId>& hitBreakpoints,
v8::debug::ExceptionType exceptionType, bool isUncaught,
bool isOOMBreak, bool isAssert);
bool isOOMBreak, bool isAssert, bool isStepAction);
void didContinue();
void didParseSource(std::unique_ptr<V8DebuggerScript>, bool success);
......
......@@ -397,7 +397,8 @@ void V8Debugger::clearContinueToLocation() {
void V8Debugger::handleProgramBreak(
v8::Local<v8::Context> pausedContext, v8::Local<v8::Value> exception,
const std::vector<v8::debug::BreakpointId>& breakpointIds,
v8::debug::ExceptionType exceptionType, bool isUncaught) {
StepBreak isStepAction, v8::debug::ExceptionType exceptionType,
bool isUncaught) {
// Don't allow nested breaks.
if (isPaused()) return;
......@@ -471,7 +472,7 @@ void V8Debugger::handleProgramBreak(
session->debuggerAgent()->didPause(
InspectedContext::contextId(pausedContext), {},
instrumentationBreakpointIds,
v8::debug::ExceptionType::kException, false, false, false);
v8::debug::ExceptionType::kException, false, false, false, false);
});
{
v8::Context::Scope scope(pausedContext);
......@@ -504,14 +505,16 @@ void V8Debugger::handleProgramBreak(
// want to trigger two pause events if we only break because of an
// instrumentation.
m_inspector->forEachSession(
contextGroupId, [&pausedContext, &exception, &regularBreakpointIds,
&exceptionType, &isUncaught, &scheduledOOMBreak,
&scheduledAssertBreak](V8InspectorSessionImpl* session) {
contextGroupId,
[&pausedContext, &exception, &regularBreakpointIds, &exceptionType,
&isUncaught, &scheduledOOMBreak, &scheduledAssertBreak,
&isStepAction](V8InspectorSessionImpl* session) {
if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak)) {
session->debuggerAgent()->didPause(
InspectedContext::contextId(pausedContext), exception,
regularBreakpointIds, exceptionType, isUncaught,
scheduledOOMBreak, scheduledAssertBreak);
scheduledOOMBreak, scheduledAssertBreak,
isStepAction == kIsStepBreak);
}
});
{
......@@ -580,8 +583,10 @@ void V8Debugger::ScriptCompiled(v8::Local<v8::debug::Script> script,
void V8Debugger::BreakProgramRequested(
v8::Local<v8::Context> pausedContext,
const std::vector<v8::debug::BreakpointId>& break_points_hit) {
handleProgramBreak(pausedContext, v8::Local<v8::Value>(), break_points_hit);
const std::vector<v8::debug::BreakpointId>& break_points_hit,
StepBreak is_step_break) {
handleProgramBreak(pausedContext, v8::Local<v8::Value>(), break_points_hit,
is_step_break);
}
void V8Debugger::ExceptionThrown(v8::Local<v8::Context> pausedContext,
......@@ -589,8 +594,8 @@ void V8Debugger::ExceptionThrown(v8::Local<v8::Context> pausedContext,
v8::Local<v8::Value> promise, bool isUncaught,
v8::debug::ExceptionType exceptionType) {
std::vector<v8::debug::BreakpointId> break_points_hit;
handleProgramBreak(pausedContext, exception, break_points_hit, exceptionType,
isUncaught);
handleProgramBreak(pausedContext, exception, break_points_hit, kIsNoStepBreak,
exceptionType, isUncaught);
}
bool V8Debugger::IsFunctionBlackboxed(v8::Local<v8::debug::Script> script,
......
......@@ -142,6 +142,7 @@ class V8Debugger : public v8::debug::DebugDelegate,
void handleProgramBreak(
v8::Local<v8::Context> pausedContext, v8::Local<v8::Value> exception,
const std::vector<v8::debug::BreakpointId>& hitBreakpoints,
StepBreak isStepBreak = StepBreak::kIsNoStepBreak,
v8::debug::ExceptionType exception_type = v8::debug::kException,
bool isUncaught = false);
......@@ -178,7 +179,8 @@ class V8Debugger : public v8::debug::DebugDelegate,
bool has_compile_error) override;
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& break_points_hit) override;
const std::vector<v8::debug::BreakpointId>& break_points_hit,
StepBreak is_step_break) override;
void ExceptionThrown(v8::Local<v8::Context> paused_context,
v8::Local<v8::Value> exception,
v8::Local<v8::Value> promise, bool is_uncaught,
......
......@@ -2921,7 +2921,8 @@ TEST(BytecodeGraphBuilderIllegalConstDeclaration) {
class CountBreakDebugDelegate : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<int>&) override {
const std::vector<int>&,
StepBreak is_step_break) override {
debug_break_count++;
}
int debug_break_count = 0;
......
......@@ -195,9 +195,9 @@ int break_point_hit_count = 0;
int break_point_hit_count_deoptimize = 0;
class DebugEventCounter : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(
v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&) override {
void BreakProgramRequested(v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&,
StepBreak is_step_break) override {
break_point_hit_count++;
// Perform a full deoptimization when the specified number of
// breaks have been hit.
......@@ -218,9 +218,9 @@ class DebugEventCounter : public v8::debug::DebugDelegate {
// Debug event handler which performs a garbage collection.
class DebugEventBreakPointCollectGarbage : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(
v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&) override {
void BreakProgramRequested(v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&,
StepBreak is_step_break) override {
// Perform a garbage collection when break point is hit and continue. Based
// on the number of break points hit either scavenge or mark compact
// collector is used.
......@@ -239,9 +239,9 @@ class DebugEventBreakPointCollectGarbage : public v8::debug::DebugDelegate {
// collector to have the heap verified.
class DebugEventBreak : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(
v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&) override {
void BreakProgramRequested(v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&,
StepBreak is_step_break) override {
// Count the number of breaks.
break_point_hit_count++;
......@@ -264,9 +264,9 @@ int max_break_point_hit_count = 0;
bool terminate_after_max_break_point_hit = false;
class DebugEventBreakMax : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(
v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&) override {
void BreakProgramRequested(v8::Local<v8::Context>,
const std::vector<v8::debug::BreakpointId>&,
StepBreak is_step_break) override {
v8::Isolate* v8_isolate = CcTest::isolate();
v8::internal::Isolate* isolate = CcTest::i_isolate();
if (break_point_hit_count < max_break_point_hit_count) {
......@@ -3309,9 +3309,10 @@ static v8::Local<v8::Value> expected_context_data;
class ContextCheckEventListener : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
CheckContext();
}
void ScriptCompiled(v8::Local<v8::debug::Script> script, bool is_live_edited,
......@@ -3824,9 +3825,10 @@ TEST(DebugBreakInForCondition2) {
class DebugBreakInlineListener : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
int expected_frame_count = 4;
int expected_line_number[] = {1, 4, 7, 13};
......@@ -3906,9 +3908,10 @@ TEST(Regress131642) {
class DebugBreakStackTraceListener : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
v8::StackTrace::CurrentStackTrace(CcTest::isolate(), 10);
}
};
......@@ -3946,9 +3949,10 @@ v8::base::Semaphore terminate_fired_semaphore(0);
class DebugBreakTriggerTerminate : public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
if (terminate_already_fired_) return;
terminate_requested_semaphore.Signal();
// Wait for at most 2 seconds for the terminate request.
......@@ -4056,9 +4060,9 @@ class ArchiveRestoreThread : public v8::base::Thread,
}
}
void BreakProgramRequested(
v8::Local<v8::Context> context,
const std::vector<v8::debug::BreakpointId>&) override {
void BreakProgramRequested(v8::Local<v8::Context> context,
const std::vector<v8::debug::BreakpointId>&,
StepBreak is_step_break) override {
auto stack_traces = v8::debug::StackTraceIterator::Create(isolate_);
if (!stack_traces->Done()) {
v8::debug::Location location = stack_traces->GetSourceLocation();
......@@ -4276,9 +4280,10 @@ TEST(BreakLocationIterator) {
class DebugStepOverFunctionWithCaughtExceptionListener
: public v8::debug::DebugDelegate {
public:
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
++break_point_hit_count;
if (break_point_hit_count >= 3) return;
PrepareStep(StepOver);
......@@ -4777,9 +4782,10 @@ class SetBreakpointOnScriptCompiled : public v8::debug::DebugDelegate {
CHECK_EQ(loc.GetColumnNumber(), 10);
}
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
++break_count_;
CHECK_EQ(inspector_break_points_hit[0], id_);
}
......@@ -5203,9 +5209,10 @@ class SetTerminateOnResumeDelegate : public v8::debug::DebugDelegate {
};
explicit SetTerminateOnResumeDelegate(Options options = kNone)
: options_(options) {}
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
break_count_++;
v8::Isolate* isolate = paused_context->GetIsolate();
v8::debug::SetTerminateOnResume(isolate);
......@@ -5580,9 +5587,10 @@ namespace {
class SemaphoreTriggerOnBreak : public v8::debug::DebugDelegate {
public:
SemaphoreTriggerOnBreak() : enter_(0), exit_(0) {}
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>&
inspector_break_points_hit) override {
void BreakProgramRequested(
v8::Local<v8::Context> paused_context,
const std::vector<v8::debug::BreakpointId>& inspector_break_points_hit,
StepBreak is_step_break) override {
break_count_++;
enter_.Signal();
exit_.Wait();
......
......@@ -110,7 +110,8 @@ class BreakHandler : public debug::DebugDelegate {
std::vector<BreakPoint> expected_breaks_;
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<int>&) override {
const std::vector<int>&,
StepBreak is_step_break) override {
printf("Break #%d\n", count_);
CHECK_GT(expected_breaks_.size(), count_);
......@@ -221,7 +222,8 @@ class CollectValuesBreakHandler : public debug::DebugDelegate {
std::vector<BreakpointValues> expected_values_;
void BreakProgramRequested(v8::Local<v8::Context> paused_context,
const std::vector<int>&) override {
const std::vector<int>&,
StepBreak is_step_break) override {
printf("Break #%d\n", count_);
CHECK_GT(expected_values_.size(), count_);
auto& expected = expected_values_[count_];
......
......@@ -6,3 +6,12 @@ Paused with reason: other and data: {}.
Running test: testTriggeredPausePauseReason
Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"4"}},{"reason":"other"}]}.
Running test: testSteppingPauseReason
Paused with reason: instrumentation and data: {"url":"foo.js","scriptId":"5"}.
Paused with reason: other and data: {}.
Paused with reason: other and data: {}.
Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"bar.js","scriptId":"6"}},{"reason":"other"}]}.
Running test: testOnlyReportOtherWithEmptyDataOnce
Paused with reason: other and data: {}.
......@@ -5,10 +5,11 @@
const { session, contextGroup, Protocol } = InspectorTest.start(
`Test that all 'other' reasons are explicitly encoded on a pause event if they overlap with another reason`);
Protocol.Debugger.onPaused(({params: {reason, data}}) => {
InspectorTest.log(`Paused with reason: ${reason} and data: ${data ? JSON.stringify(data) : '{}'}.`)
function resumeOnPause({params: {reason, data}}) {
InspectorTest.log(`Paused with reason: ${reason} and data: ${
data ? JSON.stringify(data) : '{}'}.`)
Protocol.Debugger.resume();
});
}
async function setUpEnvironment() {
await Protocol.Debugger.enable();
......@@ -23,6 +24,7 @@ async function tearDownEnvironment() {
InspectorTest.runAsyncTestSuite([
async function testBreakpointPauseReason() {
await setUpEnvironment()
Protocol.Debugger.onPaused(resumeOnPause);
await Protocol.Debugger .setInstrumentationBreakpoint({
instrumentation: 'beforeScriptExecution'
});
......@@ -38,6 +40,7 @@ InspectorTest.runAsyncTestSuite([
},
async function testTriggeredPausePauseReason() {
await setUpEnvironment();
Protocol.Debugger.onPaused(resumeOnPause);
await Protocol.Debugger.setInstrumentationBreakpoint({
instrumentation: 'beforeScriptExecution'
});
......@@ -45,5 +48,50 @@ InspectorTest.runAsyncTestSuite([
await Protocol.Runtime.evaluate({
expression: `console.log('foo');//# sourceURL=foo.js`});
tearDownEnvironment();
}]
);
},
async function testSteppingPauseReason() {
await setUpEnvironment();
await Protocol.Debugger.setInstrumentationBreakpoint(
{instrumentation: 'beforeScriptExecution'});
const stepOnPause = (({params: {reason, data}}) => {
InspectorTest.log(`Paused with reason: ${reason} and data: ${
data ? JSON.stringify(data) : '{}'}.`);
if (reason === 'instrumentation') {
Protocol.Debugger.resume();
} else {
Protocol.Debugger.stepInto();
}
});
Protocol.Debugger.onPaused(stepOnPause);
const {result: {scriptId}} = await Protocol.Runtime.compileScript({
expression: `setTimeout('console.log(3);//# sourceURL=bar.js', 0);`,
sourceURL: 'foo.js',
persistScript: true
});
await Protocol.Debugger.setBreakpointByUrl({
lineNumber: 0,
url: 'foo.js',
});
await Protocol.Runtime.runScript({scriptId});
await tearDownEnvironment();
},
async function testOnlyReportOtherWithEmptyDataOnce() {
await setUpEnvironment();
Protocol.Debugger.onPaused(resumeOnPause);
Protocol.Debugger.pause();
const {result: {scriptId}} = await Protocol.Runtime.compileScript({
expression: 'console.log(foo);',
sourceURL: 'foo.js',
persistScript: true
});
await Protocol.Debugger.setBreakpointByUrl({
lineNumber: 0,
url: 'foo.js',
});
await Protocol.Runtime.runScript({scriptId});
await tearDownEnvironment();
}
]);
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment