From 3d4f85ab9f38340082519dea318bfe2ad967a5a5 Mon Sep 17 00:00:00 2001 From: yangguo Date: Mon, 4 Apr 2016 02:38:41 -0700 Subject: [PATCH] [debugger] fix step-next for tail calls. R=ishell@chromium.org BUG=v8:4698 LOG=N Review URL: https://codereview.chromium.org/1847373002 Cr-Commit-Position: refs/heads/master@{#35230} --- src/arm64/assembler-arm64.cc | 2 +- src/assembler.cc | 3 + src/assembler.h | 6 +- src/debug/debug.cc | 25 ++++++-- src/debug/debug.h | 10 +++- src/full-codegen/arm/full-codegen-arm.cc | 2 +- src/full-codegen/arm64/full-codegen-arm64.cc | 2 +- src/full-codegen/full-codegen.cc | 9 ++- src/full-codegen/full-codegen.h | 3 +- src/full-codegen/ia32/full-codegen-ia32.cc | 2 +- src/full-codegen/mips/full-codegen-mips.cc | 2 +- .../mips64/full-codegen-mips64.cc | 2 +- src/full-codegen/ppc/full-codegen-ppc.cc | 2 +- src/full-codegen/s390/full-codegen-s390.cc | 2 +- src/full-codegen/x64/full-codegen-x64.cc | 2 +- src/full-codegen/x87/full-codegen-x87.cc | 2 +- src/mips/assembler-mips.cc | 2 +- src/mips64/assembler-mips64.cc | 2 +- test/cctest/test-debug.cc | 58 +++++++++++++++++++ 19 files changed, 115 insertions(+), 23 deletions(-) diff --git a/src/arm64/assembler-arm64.cc b/src/arm64/assembler-arm64.cc index 067be1baad..2471d5eebd 100644 --- a/src/arm64/assembler-arm64.cc +++ b/src/arm64/assembler-arm64.cc @@ -2875,7 +2875,7 @@ void Assembler::RecordRelocInfo(RelocInfo::Mode rmode, intptr_t data) { // We do not try to reuse pool constants. RelocInfo rinfo(isolate(), reinterpret_cast(pc_), rmode, data, NULL); if (((rmode >= RelocInfo::COMMENT) && - (rmode <= RelocInfo::DEBUG_BREAK_SLOT_AT_CALL)) || + (rmode <= RelocInfo::DEBUG_BREAK_SLOT_AT_TAIL_CALL)) || (rmode == RelocInfo::INTERNAL_REFERENCE) || (rmode == RelocInfo::CONST_POOL) || (rmode == RelocInfo::VENEER_POOL) || (rmode == RelocInfo::DEOPT_REASON) || diff --git a/src/assembler.cc b/src/assembler.cc index 6bcd2fef29..0ca865bdd0 100644 --- a/src/assembler.cc +++ b/src/assembler.cc @@ -838,6 +838,8 @@ const char* RelocInfo::RelocModeName(RelocInfo::Mode rmode) { return "debug break slot at return"; case DEBUG_BREAK_SLOT_AT_CALL: return "debug break slot at call"; + case DEBUG_BREAK_SLOT_AT_TAIL_CALL: + return "debug break slot at tail call"; case CODE_AGE_SEQUENCE: return "code age sequence"; case GENERATOR_CONTINUATION: @@ -936,6 +938,7 @@ void RelocInfo::Verify(Isolate* isolate) { case DEBUG_BREAK_SLOT_AT_POSITION: case DEBUG_BREAK_SLOT_AT_RETURN: case DEBUG_BREAK_SLOT_AT_CALL: + case DEBUG_BREAK_SLOT_AT_TAIL_CALL: case GENERATOR_CONTINUATION: case WASM_MEMORY_REFERENCE: case NONE32: diff --git a/src/assembler.h b/src/assembler.h index 46566ffe6f..192d16b64d 100644 --- a/src/assembler.h +++ b/src/assembler.h @@ -398,6 +398,7 @@ class RelocInfo { DEBUG_BREAK_SLOT_AT_POSITION, DEBUG_BREAK_SLOT_AT_RETURN, DEBUG_BREAK_SLOT_AT_CALL, + DEBUG_BREAK_SLOT_AT_TAIL_CALL, EXTERNAL_REFERENCE, // The address of an external C++ function. INTERNAL_REFERENCE, // An address inside the same function. @@ -491,7 +492,7 @@ class RelocInfo { } static inline bool IsDebugBreakSlot(Mode mode) { return IsDebugBreakSlotAtPosition(mode) || IsDebugBreakSlotAtReturn(mode) || - IsDebugBreakSlotAtCall(mode); + IsDebugBreakSlotAtCall(mode) || IsDebugBreakSlotAtTailCall(mode); } static inline bool IsDebugBreakSlotAtPosition(Mode mode) { return mode == DEBUG_BREAK_SLOT_AT_POSITION; @@ -502,6 +503,9 @@ class RelocInfo { static inline bool IsDebugBreakSlotAtCall(Mode mode) { return mode == DEBUG_BREAK_SLOT_AT_CALL; } + static inline bool IsDebugBreakSlotAtTailCall(Mode mode) { + return mode == DEBUG_BREAK_SLOT_AT_TAIL_CALL; + } static inline bool IsDebuggerStatement(Mode mode) { return mode == DEBUGGER_STATEMENT; } diff --git a/src/debug/debug.cc b/src/debug/debug.cc index 3a6e0ac0de..7c76742bb9 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -108,6 +108,9 @@ int BreakLocation::CodeIterator::GetModeMask(BreakLocatorType type) { mask |= RelocInfo::ModeMask(RelocInfo::STATEMENT_POSITION); mask |= RelocInfo::ModeMask(RelocInfo::DEBUG_BREAK_SLOT_AT_RETURN); mask |= RelocInfo::ModeMask(RelocInfo::DEBUG_BREAK_SLOT_AT_CALL); + if (isolate()->is_tail_call_elimination_enabled()) { + mask |= RelocInfo::ModeMask(RelocInfo::DEBUG_BREAK_SLOT_AT_TAIL_CALL); + } if (type == ALL_BREAK_LOCATIONS) { mask |= RelocInfo::ModeMask(RelocInfo::DEBUG_BREAK_SLOT_AT_POSITION); mask |= RelocInfo::ModeMask(RelocInfo::DEBUGGER_STATEMENT); @@ -161,6 +164,10 @@ BreakLocation BreakLocation::CodeIterator::GetBreakLocation() { type = DEBUG_BREAK_SLOT_AT_RETURN; } else if (RelocInfo::IsDebugBreakSlotAtCall(rmode())) { type = DEBUG_BREAK_SLOT_AT_CALL; + } else if (RelocInfo::IsDebugBreakSlotAtTailCall(rmode())) { + type = isolate()->is_tail_call_elimination_enabled() + ? DEBUG_BREAK_SLOT_AT_TAIL_CALL + : DEBUG_BREAK_SLOT_AT_CALL; } else if (RelocInfo::IsDebuggerStatement(rmode())) { type = DEBUGGER_STATEMENT; } else if (RelocInfo::IsDebugBreakSlot(rmode())) { @@ -226,6 +233,10 @@ BreakLocation::BytecodeArrayIterator::GetDebugBreakType() { return DEBUGGER_STATEMENT; } else if (bytecode == interpreter::Bytecode::kReturn) { return DEBUG_BREAK_SLOT_AT_RETURN; + } else if (bytecode == interpreter::Bytecode::kTailCall) { + return isolate()->is_tail_call_elimination_enabled() + ? DEBUG_BREAK_SLOT_AT_TAIL_CALL + : DEBUG_BREAK_SLOT_AT_CALL; } else if (interpreter::Bytecodes::IsCallOrNew(bytecode)) { return DEBUG_BREAK_SLOT_AT_CALL; } else if (source_position_iterator_.is_statement()) { @@ -604,22 +615,26 @@ void Debug::Break(JavaScriptFrame* frame) { Address target_fp = thread_local_.target_fp_; Address last_fp = thread_local_.last_fp_; - bool step_break = true; + bool step_break = false; switch (step_action) { case StepNone: return; case StepOut: // Step out has not reached the target frame yet. if (current_fp < target_fp) return; + step_break = true; break; case StepNext: // Step next should not break in a deeper frame. if (current_fp < target_fp) return; + // For step-next, a tail call is like a return and should break. + step_break = location.IsTailCall(); // Fall through. case StepIn: { FrameSummary summary = GetFirstFrameSummary(frame); int offset = summary.code_offset(); - step_break = location.IsReturn() || (current_fp != last_fp) || + step_break = step_break || location.IsReturn() || + (current_fp != last_fp) || (thread_local_.last_statement_position_ != location.abstract_code()->SourceStatementPosition(offset)); break; @@ -1015,8 +1030,10 @@ void Debug::PrepareStep(StepAction step_action) { BreakLocation location = BreakLocation::FromCodeOffset(debug_info, call_offset); - // At a return statement we will step out either way. + // Any step at a return is a step-out. if (location.IsReturn()) step_action = StepOut; + // A step-next at a tail call is a step-out. + if (location.IsTailCall() && step_action == StepNext) step_action = StepOut; thread_local_.last_statement_position_ = debug_info->abstract_code()->SourceStatementPosition( @@ -1592,7 +1609,7 @@ bool Debug::IsBreakAtReturn(JavaScriptFrame* frame) { Handle debug_info(shared->GetDebugInfo()); BreakLocation location = BreakLocation::FromCodeOffset(debug_info, summary.code_offset()); - return location.IsReturn(); + return location.IsReturn() || location.IsTailCall(); } diff --git a/src/debug/debug.h b/src/debug/debug.h index 3e59adea1d..35aa8db049 100644 --- a/src/debug/debug.h +++ b/src/debug/debug.h @@ -81,6 +81,9 @@ class BreakLocation { inline bool IsReturn() const { return type_ == DEBUG_BREAK_SLOT_AT_RETURN; } inline bool IsCall() const { return type_ == DEBUG_BREAK_SLOT_AT_CALL; } + inline bool IsTailCall() const { + return type_ == DEBUG_BREAK_SLOT_AT_TAIL_CALL; + } inline bool IsDebugBreakSlot() const { return type_ >= DEBUG_BREAK_SLOT; } inline bool IsDebuggerStatement() const { return type_ == DEBUGGER_STATEMENT; @@ -113,7 +116,8 @@ class BreakLocation { DEBUGGER_STATEMENT, DEBUG_BREAK_SLOT, DEBUG_BREAK_SLOT_AT_CALL, - DEBUG_BREAK_SLOT_AT_RETURN + DEBUG_BREAK_SLOT_AT_RETURN, + DEBUG_BREAK_SLOT_AT_TAIL_CALL, }; BreakLocation(Handle debug_info, DebugBreakType type, @@ -140,6 +144,8 @@ class BreakLocation { explicit Iterator(Handle debug_info); int ReturnPosition(); + Isolate* isolate() { return debug_info_->GetIsolate(); } + Handle debug_info_; int break_index_; int position_; @@ -166,7 +172,7 @@ class BreakLocation { } private: - static int GetModeMask(BreakLocatorType type); + int GetModeMask(BreakLocatorType type); RelocInfo::Mode rmode() { return reloc_iterator_.rinfo()->rmode(); } RelocInfo* rinfo() { return reloc_iterator_.rinfo(); } diff --git a/src/full-codegen/arm/full-codegen-arm.cc b/src/full-codegen/arm/full-codegen-arm.cc index e32f41507c..27985fbe49 100644 --- a/src/full-codegen/arm/full-codegen-arm.cc +++ b/src/full-codegen/arm/full-codegen-arm.cc @@ -2638,7 +2638,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/arm64/full-codegen-arm64.cc b/src/full-codegen/arm64/full-codegen-arm64.cc index 36305721ae..eb044dbfd9 100644 --- a/src/full-codegen/arm64/full-codegen-arm64.cc +++ b/src/full-codegen/arm64/full-codegen-arm64.cc @@ -2436,7 +2436,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/full-codegen.cc b/src/full-codegen/full-codegen.cc index 54f1fff977..8c0cddc0d0 100644 --- a/src/full-codegen/full-codegen.cc +++ b/src/full-codegen/full-codegen.cc @@ -671,13 +671,16 @@ void FullCodeGenerator::SetExpressionAsStatementPosition(Expression* expr) { } } - -void FullCodeGenerator::SetCallPosition(Expression* expr) { +void FullCodeGenerator::SetCallPosition(Expression* expr, + TailCallMode tail_call_mode) { if (expr->position() == RelocInfo::kNoPosition) return; RecordPosition(masm_, expr->position()); if (info_->is_debug()) { + RelocInfo::Mode mode = (tail_call_mode == TailCallMode::kAllow) + ? RelocInfo::DEBUG_BREAK_SLOT_AT_TAIL_CALL + : RelocInfo::DEBUG_BREAK_SLOT_AT_CALL; // Always emit a debug break slot before a call. - DebugCodegen::GenerateSlot(masm_, RelocInfo::DEBUG_BREAK_SLOT_AT_CALL); + DebugCodegen::GenerateSlot(masm_, mode); } } diff --git a/src/full-codegen/full-codegen.h b/src/full-codegen/full-codegen.h index 16753088e4..b41de6dbed 100644 --- a/src/full-codegen/full-codegen.h +++ b/src/full-codegen/full-codegen.h @@ -677,7 +677,8 @@ class FullCodeGenerator: public AstVisitor { // This is used in loop headers where we want to break for each iteration. void SetExpressionAsStatementPosition(Expression* expr); - void SetCallPosition(Expression* expr); + void SetCallPosition(Expression* expr, + TailCallMode tail_call_mode = TailCallMode::kDisallow); void SetConstructCallPosition(Expression* expr) { // Currently call and construct calls are treated the same wrt debugging. diff --git a/src/full-codegen/ia32/full-codegen-ia32.cc b/src/full-codegen/ia32/full-codegen-ia32.cc index 7148d47e1c..4dfe902386 100644 --- a/src/full-codegen/ia32/full-codegen-ia32.cc +++ b/src/full-codegen/ia32/full-codegen-ia32.cc @@ -2523,7 +2523,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/mips/full-codegen-mips.cc b/src/full-codegen/mips/full-codegen-mips.cc index bde7f6eb46..d84678040e 100644 --- a/src/full-codegen/mips/full-codegen-mips.cc +++ b/src/full-codegen/mips/full-codegen-mips.cc @@ -2635,7 +2635,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); // Record source position of the IC call. - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/mips64/full-codegen-mips64.cc b/src/full-codegen/mips64/full-codegen-mips64.cc index ff5cf96291..c9075f5f51 100644 --- a/src/full-codegen/mips64/full-codegen-mips64.cc +++ b/src/full-codegen/mips64/full-codegen-mips64.cc @@ -2636,7 +2636,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); // Record source position of the IC call. - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/ppc/full-codegen-ppc.cc b/src/full-codegen/ppc/full-codegen-ppc.cc index ff2be4fac8..b0855053ac 100644 --- a/src/full-codegen/ppc/full-codegen-ppc.cc +++ b/src/full-codegen/ppc/full-codegen-ppc.cc @@ -2639,7 +2639,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/s390/full-codegen-s390.cc b/src/full-codegen/s390/full-codegen-s390.cc index 2cfddd8423..29a7f761b2 100644 --- a/src/full-codegen/s390/full-codegen-s390.cc +++ b/src/full-codegen/s390/full-codegen-s390.cc @@ -2574,7 +2574,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/x64/full-codegen-x64.cc b/src/full-codegen/x64/full-codegen-x64.cc index fb412f8bdb..b7e7cdd2ef 100644 --- a/src/full-codegen/x64/full-codegen-x64.cc +++ b/src/full-codegen/x64/full-codegen-x64.cc @@ -2513,7 +2513,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/full-codegen/x87/full-codegen-x87.cc b/src/full-codegen/x87/full-codegen-x87.cc index 4452fa1af6..8d2114340d 100644 --- a/src/full-codegen/x87/full-codegen-x87.cc +++ b/src/full-codegen/x87/full-codegen-x87.cc @@ -2515,7 +2515,7 @@ void FullCodeGenerator::EmitCall(Call* expr, ConvertReceiverMode mode) { } PrepareForBailoutForId(expr->CallId(), NO_REGISTERS); - SetCallPosition(expr); + SetCallPosition(expr, expr->tail_call_mode()); if (expr->tail_call_mode() == TailCallMode::kAllow) { if (FLAG_trace) { __ CallRuntime(Runtime::kTraceTailCall); diff --git a/src/mips/assembler-mips.cc b/src/mips/assembler-mips.cc index 2c524daf3b..bfa232892a 100644 --- a/src/mips/assembler-mips.cc +++ b/src/mips/assembler-mips.cc @@ -2966,7 +2966,7 @@ void Assembler::RecordRelocInfo(RelocInfo::Mode rmode, intptr_t data) { // We do not try to reuse pool constants. RelocInfo rinfo(isolate(), pc_, rmode, data, NULL); if (rmode >= RelocInfo::COMMENT && - rmode <= RelocInfo::DEBUG_BREAK_SLOT_AT_CALL) { + rmode <= RelocInfo::DEBUG_BREAK_SLOT_AT_TAIL_CALL) { // Adjust code for new modes. DCHECK(RelocInfo::IsDebugBreakSlot(rmode) || RelocInfo::IsComment(rmode) diff --git a/src/mips64/assembler-mips64.cc b/src/mips64/assembler-mips64.cc index b1eb4ce3d4..5a8dd2cd37 100644 --- a/src/mips64/assembler-mips64.cc +++ b/src/mips64/assembler-mips64.cc @@ -3219,7 +3219,7 @@ void Assembler::RecordRelocInfo(RelocInfo::Mode rmode, intptr_t data) { // We do not try to reuse pool constants. RelocInfo rinfo(isolate(), pc_, rmode, data, NULL); if (rmode >= RelocInfo::COMMENT && - rmode <= RelocInfo::DEBUG_BREAK_SLOT_AT_CALL) { + rmode <= RelocInfo::DEBUG_BREAK_SLOT_AT_TAIL_CALL) { // Adjust code for new modes. DCHECK(RelocInfo::IsDebugBreakSlot(rmode) || RelocInfo::IsComment(rmode) diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc index 253842d152..ab27f394e9 100644 --- a/test/cctest/test-debug.cc +++ b/test/cctest/test-debug.cc @@ -8127,3 +8127,61 @@ TEST(DisableTailCallElimination) { ExpectInt32("h();", 2); ExpectInt32("h(); %OptimizeFunctionOnNextCall(g); h();", 2); } + +TEST(DebugStepNextTailCallEliminiation) { + i::FLAG_allow_natives_syntax = true; + i::FLAG_harmony_tailcalls = true; + // TODO(ishell, 4698): Investigate why TurboFan in --always-opt mode makes + // stack[2].getFunctionName() return null. + i::FLAG_turbo_inlining = false; + + DebugLocalContext env; + env.ExposeDebug(); + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope scope(isolate); + CHECK(v8::Debug::IsTailCallEliminationEnabled(isolate)); + + const char* source = + "'use strict'; \n" + "var Debug = debug.Debug; \n" + "var exception = null; \n" + "var breaks = 0; \n" + "var log = []; \n" + "function f(x) { \n" + " if (x == 2) { \n" + " debugger; // Break a \n" + " } \n" + " if (x-- > 0) { // Break b \n" + " return f(x); // Break c \n" + " } \n" + "} // Break e \n" + "function listener(event, exec_state, event_data, data) {\n" + " if (event != Debug.DebugEvent.Break) return; \n" + " try { \n" + " var line = exec_state.frame(0).sourceLineText(); \n" + " var col = exec_state.frame(0).sourceColumn(); \n" + " var match = line.match(/\\/\\/ Break (\\w)/); \n" + " log.push(match[1] + col); \n" + " exec_state.prepareStep(Debug.StepAction.StepNext); \n" + " } catch (e) { \n" + " exception = e; \n" + " }; \n" + "}; \n" + "Debug.setListener(listener); \n" + "f(4); \n" + "Debug.setListener(null); // Break d \n"; + + CompileRun(source); + ExpectNull("exception"); + ExpectString("JSON.stringify(log)", "[\"a4\",\"b2\",\"c4\",\"c11\",\"d0\"]"); + + v8::Debug::SetTailCallEliminationEnabled(isolate, false); + CompileRun( + "log = []; \n" + "Debug.setListener(listener); \n" + "f(5); \n" + "Debug.setListener(null); // Break f \n"); + ExpectNull("exception"); + ExpectString("JSON.stringify(log)", + "[\"a4\",\"b2\",\"c4\",\"e0\",\"e0\",\"e0\",\"e0\",\"f0\"]"); +}