Skip to content

Commit 307ca3f

Browse files
creliercommit-bot@chromium.org
crelier
authored and
[email protected]
committed
[vm/bytecode] Improve single stepping and breakpoint setting in bytecode.
Modified list of bytecodes that check for debug breaks (wip). Make sure source positions are not interpreted as return addresses. Fix skipping of bytecode stub frames when stepping. Fix DEBUG_CHECK macro to call debugger twice for breakpoint and following step. Various minor bug fixes and printing improvements. Change-Id: I36713de0548d060eddb7cbfd6fadb05c731e881a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108205 Commit-Queue: Régis Crelier <[email protected]> Reviewed-by: Ryan Macnak <[email protected]>
1 parent 855830f commit 307ca3f

File tree

10 files changed

+145
-59
lines changed

10 files changed

+145
-59
lines changed

‎pkg/vm/lib/bytecode/assembler.dart‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,6 @@ class BytecodeAssembler {
390390
}
391391

392392
void emitNativeCall(int rd) {
393-
emitSourcePosition();
394393
_emitInstructionD(Opcode.kNativeCall, rd);
395394
}
396395

‎pkg/vm/lib/bytecode/gen_bytecode.dart‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1534,6 +1534,12 @@ class BytecodeGenerator extends RecursiveVisitor<Null> {
15341534
} else {
15351535
asm.emitEntry(locals.frameSize);
15361536
}
1537+
// TODO(alexmarkov): Introduce a new bytecode triggering a debug check in
1538+
// the interpreter. Its token position should correspond to the declaration
1539+
// position of the last parameter, which the debugger can inspect at the
1540+
// point of the debug check.
1541+
// TODO(regis): Support the new bytecode in the interpreter and dissociate
1542+
// the debug check from the CheckStack bytecode.
15371543
asm.emitCheckStack(0);
15381544

15391545
if (isClosure) {
@@ -1667,8 +1673,8 @@ class BytecodeGenerator extends RecursiveVisitor<Null> {
16671673
_genPushContextForVariable(variable);
16681674
asm.emitPush(locals.getOriginalParamSlotIndex(variable));
16691675
_genStoreVar(variable);
1670-
// TODO(alexmarkov): Do we need to store null at the original parameter
1671-
// location?
1676+
// TODO(alexmarkov): We need to store null at the original parameter
1677+
// location, because the original value may need to be GC'ed.
16721678
}
16731679
}
16741680

‎runtime/vm/compiler/assembler/disassembler_kbc.cc‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,11 @@ static void Fmttgt(char** buf,
5757
intptr_t* size,
5858
const KBCInstr* instr,
5959
int32_t value) {
60-
FormatOperand(buf, size, "-> %" Px, instr + value);
60+
if (FLAG_disassemble_relative) {
61+
FormatOperand(buf, size, "-> %" Pd, value);
62+
} else {
63+
FormatOperand(buf, size, "-> %" Px, instr + value);
64+
}
6165
}
6266

6367
static void Fmtlit(char** buf,
@@ -376,11 +380,16 @@ void KernelBytecodeDisassembler::Disassemble(const Function& function) {
376380
const int addr_width = (kBitsPerWord / 4) + 2;
377381
// "*" in a printf format specifier tells it to read the field width from
378382
// the printf argument list.
379-
THR_Print("%-*s\tpos\n", addr_width, "pc");
383+
THR_Print("%-*s\tpos\tline\tcolumn\tyield\n", addr_width, "pc");
384+
const Script& script = Script::Handle(zone, function.script());
380385
kernel::BytecodeSourcePositionsIterator iter(zone, bytecode);
381386
while (iter.MoveNext()) {
382-
THR_Print("%#-*" Px "\t%s\n", addr_width, base + iter.PcOffset(),
383-
iter.TokenPos().ToCString());
387+
TokenPosition pos = iter.TokenPos();
388+
intptr_t line = -1, column = -1;
389+
script.GetTokenLocation(pos, &line, &column);
390+
THR_Print("%#-*" Px "\t%s\t%" Pd "\t%" Pd "\t%s\n", addr_width,
391+
base + iter.PcOffset(), pos.ToCString(), line, column,
392+
iter.IsYieldPoint() ? "yield" : "");
384393
}
385394
THR_Print("}\n");
386395
}

‎runtime/vm/constants_kbc.h‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -860,6 +860,11 @@ class KernelBytecode {
860860
return bc + kInstructionSize[DecodeOpcode(bc)];
861861
}
862862

863+
DART_FORCE_INLINE static uword Next(uword pc) {
864+
return pc + kInstructionSize[DecodeOpcode(
865+
reinterpret_cast<const KBCInstr*>(pc))];
866+
}
867+
863868
DART_FORCE_INLINE static bool IsJumpOpcode(const KBCInstr* instr) {
864869
switch (DecodeOpcode(instr)) {
865870
case KernelBytecode::kJump:
@@ -949,10 +954,15 @@ class KernelBytecode {
949954
}
950955
}
951956

952-
// The interpreter and this function must agree on the opcodes.
957+
// The interpreter, the bytecode generator, and this function must agree on
958+
// this list of opcodes.
959+
// The interpreter checks for a debug break at each instruction with listed
960+
// opcode and the bytecode generator emits a source position at each
961+
// instruction with listed opcode.
953962
DART_FORCE_INLINE static bool IsDebugBreakCheckedOpcode(
954963
const KBCInstr* instr) {
955964
switch (DecodeOpcode(instr)) {
965+
case KernelBytecode::kAllocate:
956966
case KernelBytecode::kPopLocal:
957967
case KernelBytecode::kPopLocal_Wide:
958968
case KernelBytecode::kStoreLocal:
@@ -972,6 +982,33 @@ class KernelBytecode {
972982
case KernelBytecode::kThrow:
973983
case KernelBytecode::kJump:
974984
case KernelBytecode::kJump_Wide:
985+
case KernelBytecode::kEqualsNull:
986+
case KernelBytecode::kNegateInt:
987+
case KernelBytecode::kNegateDouble:
988+
case KernelBytecode::kAddInt:
989+
case KernelBytecode::kSubInt:
990+
case KernelBytecode::kMulInt:
991+
case KernelBytecode::kTruncDivInt:
992+
case KernelBytecode::kModInt:
993+
case KernelBytecode::kBitAndInt:
994+
case KernelBytecode::kBitOrInt:
995+
case KernelBytecode::kBitXorInt:
996+
case KernelBytecode::kShlInt:
997+
case KernelBytecode::kShrInt:
998+
case KernelBytecode::kCompareIntEq:
999+
case KernelBytecode::kCompareIntGt:
1000+
case KernelBytecode::kCompareIntLt:
1001+
case KernelBytecode::kCompareIntGe:
1002+
case KernelBytecode::kCompareIntLe:
1003+
case KernelBytecode::kAddDouble:
1004+
case KernelBytecode::kSubDouble:
1005+
case KernelBytecode::kMulDouble:
1006+
case KernelBytecode::kDivDouble:
1007+
case KernelBytecode::kCompareDoubleEq:
1008+
case KernelBytecode::kCompareDoubleGt:
1009+
case KernelBytecode::kCompareDoubleLt:
1010+
case KernelBytecode::kCompareDoubleGe:
1011+
case KernelBytecode::kCompareDoubleLe:
9751012
return true;
9761013
default:
9771014
return false;

‎runtime/vm/debugger.cc‎

Lines changed: 33 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,8 @@ ActivationFrame::ActivationFrame(uword pc,
247247
var_descriptors_(LocalVarDescriptors::ZoneHandle()),
248248
desc_indices_(8),
249249
pc_desc_(PcDescriptors::ZoneHandle()) {
250-
ASSERT(!function_.IsNull()); // Frames with bytecode stubs should be skipped.
250+
// The frame of a bytecode stub has a null function. It may be encountered
251+
// when single stepping.
251252
}
252253
#endif // !defined(DART_PRECOMPILED_RUNTIME)
253254

@@ -676,7 +677,8 @@ void ActivationFrame::GetVarDescriptors() {
676677
}
677678

678679
bool ActivationFrame::IsDebuggable() const {
679-
return Debugger::IsDebuggable(function());
680+
// When stepping in bytecode stub, function is null.
681+
return !function().IsNull() && Debugger::IsDebuggable(function());
680682
}
681683

682684
void ActivationFrame::PrintDescriptorsError(const char* message) {
@@ -996,8 +998,9 @@ void ActivationFrame::ExtractTokenPositionFromAsyncClosure() {
996998
if (yield_point_index == await_jump_var) {
997999
token_pos_ = iter.TokenPos();
9981000
token_pos_initialized_ = true;
999-
try_index_ = bytecode.GetTryIndexAtPc(bytecode.PayloadStart() +
1000-
iter.PcOffset());
1001+
const uword return_address =
1002+
KernelBytecode::Next(bytecode.PayloadStart() + iter.PcOffset());
1003+
try_index_ = bytecode.GetTryIndexAtPc(return_address);
10011004
return;
10021005
}
10031006
++yield_point_index;
@@ -1561,15 +1564,18 @@ const char* ActivationFrame::ToCString() {
15611564
intptr_t line = LineNumber();
15621565
const char* func_name = Debugger::QualifiedFunctionName(function());
15631566
return Thread::Current()->zone()->PrintToString(
1564-
"[ Frame pc(0x%" Px ") fp(0x%" Px ") sp(0x%" Px
1567+
"[ Frame pc(0x%" Px " offset:0x%" Px ") fp(0x%" Px ") sp(0x%" Px
15651568
")\n"
15661569
"\tfunction = %s\n"
15671570
"\turl = %s\n"
15681571
"\tline = %" Pd
15691572
"\n"
15701573
"\tcontext = %s\n"
15711574
"\tcontext level = %" Pd " ]\n",
1572-
pc(), fp(), sp(), func_name, url.ToCString(), line, ctx_.ToCString(),
1575+
pc(),
1576+
pc() -
1577+
(IsInterpreted() ? bytecode().PayloadStart() : code().PayloadStart()),
1578+
fp(), sp(), func_name, url.ToCString(), line, ctx_.ToCString(),
15731579
ContextLevel());
15741580
}
15751581

@@ -2580,9 +2586,11 @@ ActivationFrame* Debugger::TopDartFrame() const {
25802586
#if !defined(DART_PRECOMPILED_RUNTIME)
25812587
if (frame->is_interpreted()) {
25822588
Bytecode& bytecode = Bytecode::Handle(frame->LookupDartBytecode());
2583-
if (bytecode.function() == Function::null()) {
2584-
continue; // Skip bytecode stub frame.
2585-
}
2589+
// Note that we do not skip bytecode stub frame (with a null function),
2590+
// so that we can ignore a single stepping breakpoint in such a frame.
2591+
// A bytecode stub contains a VM internal bytecode followed by a
2592+
// ReturnTOS bytecode. The single step on the ReturnTOS bytecode
2593+
// needs to be skipped.
25862594
ActivationFrame* activation =
25872595
new ActivationFrame(frame->pc(), frame->fp(), frame->sp(), bytecode);
25882596
return activation;
@@ -3053,19 +3061,20 @@ TokenPosition Debugger::ResolveBreakpointPos(bool in_bytecode,
30533061

30543062
#if !defined(DART_PRECOMPILED_RUNTIME)
30553063
// Find a 'debug break checked' bytecode in the range [pc..end_pc[ and return
3056-
// the pc after it or nullptr.
3057-
static const KBCInstr* FindBreakpointCheckedInstr(const KBCInstr* pc,
3058-
const KBCInstr* end_pc) {
3059-
while ((pc < end_pc) && !KernelBytecode::IsDebugBreakCheckedOpcode(pc)) {
3064+
// the pc after it or 0 if not found.
3065+
static uword FindBreakpointCheckedInstr(uword pc, uword end_pc) {
3066+
while ((pc < end_pc) && !KernelBytecode::IsDebugBreakCheckedOpcode(
3067+
reinterpret_cast<const KBCInstr*>(pc))) {
30603068
pc = KernelBytecode::Next(pc);
30613069
}
30623070
if (pc < end_pc) {
3063-
ASSERT(KernelBytecode::IsDebugBreakCheckedOpcode(pc));
3071+
ASSERT(KernelBytecode::IsDebugBreakCheckedOpcode(
3072+
reinterpret_cast<const KBCInstr*>(pc)));
30643073
// The checked debug break pc must point to the next bytecode.
30653074
return KernelBytecode::Next(pc);
30663075
}
30673076
// No 'debug break checked' bytecode in the range.
3068-
return nullptr;
3077+
return 0;
30693078
}
30703079
#endif // !defined(DART_PRECOMPILED_RUNTIME)
30713080

@@ -3079,45 +3088,40 @@ void Debugger::MakeCodeBreakpointAt(const Function& func,
30793088
if (func.HasBytecode()) {
30803089
Bytecode& bytecode = Bytecode::Handle(func.bytecode());
30813090
ASSERT(!bytecode.IsNull());
3082-
const KBCInstr* pc = nullptr;
3091+
uword pc = 0;
30833092
if (bytecode.HasSourcePositions()) {
30843093
kernel::BytecodeSourcePositionsIterator iter(Thread::Current()->zone(),
30853094
bytecode);
30863095
bool check_range = false;
30873096
while (iter.MoveNext()) {
30883097
if (check_range) {
3089-
const KBCInstr* end_pc =
3090-
reinterpret_cast<const KBCInstr*>(bytecode.PayloadStart()) +
3091-
iter.PcOffset();
3098+
uword end_pc = bytecode.PayloadStart() + iter.PcOffset();
30923099
check_range = false;
30933100
// Find a 'debug break checked' bytecode in the range [pc..end_pc[.
30943101
pc = FindBreakpointCheckedInstr(pc, end_pc);
3095-
if (pc != nullptr) {
3102+
if (pc != 0) {
30963103
// TODO(regis): We may want to find all PCs for a token position,
30973104
// e.g. in the case of duplicated bytecode in finally clauses.
30983105
break;
30993106
}
31003107
}
31013108
if (iter.TokenPos() == loc->token_pos_) {
3102-
pc = reinterpret_cast<const KBCInstr*>(bytecode.PayloadStart()) +
3103-
iter.PcOffset();
3109+
pc = bytecode.PayloadStart() + iter.PcOffset();
31043110
check_range = true;
31053111
}
31063112
}
31073113
if (check_range) {
3108-
ASSERT(pc != nullptr);
3114+
ASSERT(pc != 0);
31093115
// Use the end of the bytecode as the end of the range to check.
31103116
pc = FindBreakpointCheckedInstr(
3111-
pc, reinterpret_cast<const KBCInstr*>(bytecode.PayloadStart()) +
3112-
bytecode.Size());
3117+
pc, bytecode.PayloadStart() + bytecode.Size());
31133118
}
31143119
}
3115-
if (pc != nullptr) {
3116-
CodeBreakpoint* code_bpt = GetCodeBreakpoint(reinterpret_cast<uword>(pc));
3120+
if (pc != 0) {
3121+
CodeBreakpoint* code_bpt = GetCodeBreakpoint(pc);
31173122
if (code_bpt == NULL) {
31183123
// No code breakpoint for this code exists; create one.
3119-
code_bpt = new CodeBreakpoint(bytecode, loc->token_pos_,
3120-
reinterpret_cast<uword>(pc));
3124+
code_bpt = new CodeBreakpoint(bytecode, loc->token_pos_, pc);
31213125
RegisterCodeBreakpoint(code_bpt);
31223126
}
31233127
code_bpt->set_bpt_location(loc);

‎runtime/vm/debugger.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,6 @@ class ActivationFrame : public ZoneAllocated {
297297
uword fp() const { return fp_; }
298298
uword sp() const { return sp_; }
299299
const Function& function() const {
300-
ASSERT(!function_.IsNull());
301300
return function_;
302301
}
303302
const Code& code() const {

0 commit comments

Comments
 (0)