From 0aa1d2af3744d8ace96308082c80a12cd2e55d4c Mon Sep 17 00:00:00 2001 From: yangguo Date: Fri, 20 Feb 2015 06:47:20 -0800 Subject: [PATCH] Fix GC-unsafe use of BreakLocationIterator. R=svenpanne@chromium.org BUG=v8:3776 LOG=N Review URL: https://codereview.chromium.org/942003002 Cr-Commit-Position: refs/heads/master@{#26779} --- src/debug.cc | 134 +++++++++++++++++++------------------------- src/debug.h | 1 - test/mjsunit/mjsunit.status | 6 -- 3 files changed, 57 insertions(+), 84 deletions(-) diff --git a/src/debug.cc b/src/debug.cc index 324d96f..46c9e1e 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -387,40 +387,6 @@ bool BreakLocationIterator::IsStepInLocation(Isolate* isolate) { } -void BreakLocationIterator::PrepareStepIn(Isolate* isolate) { -#ifdef DEBUG - HandleScope scope(isolate); - // Step in can only be prepared if currently positioned on an IC call, - // construct call or CallFunction stub call. - Address target = rinfo()->target_address(); - Handle target_code(Code::GetCodeFromTargetAddress(target)); - // All the following stuff is needed only for assertion checks so the code - // is wrapped in ifdef. - Handle maybe_call_function_stub = target_code; - if (IsDebugBreak()) { - Address original_target = original_rinfo()->target_address(); - maybe_call_function_stub = - Handle(Code::GetCodeFromTargetAddress(original_target)); - } - bool is_call_function_stub = - (maybe_call_function_stub->kind() == Code::STUB && - CodeStub::GetMajorKey(*maybe_call_function_stub) == - CodeStub::CallFunction); - - // Step in through construct call requires no changes to the running code. - // Step in through getters/setters should already be prepared as well - // because caller of this function (Debug::PrepareStep) is expected to - // flood the top frame's function with one shot breakpoints. - // Step in through CallFunction stub should also be prepared by caller of - // this function (Debug::PrepareStep) which should flood target function - // with breakpoints. - DCHECK(RelocInfo::IsConstructCall(rmode()) || - target_code->is_inline_cache_stub() || - is_call_function_stub); -#endif -} - - // Check whether the break point is at a position which will exit the function. bool BreakLocationIterator::IsExit() const { return (RelocInfo::IsJSReturn(rmode())); @@ -1371,62 +1337,68 @@ void Debug::PrepareStep(StepAction step_action, } Handle debug_info = GetDebugInfo(shared); - // Find the break location where execution has stopped. - BreakLocationIterator it(debug_info, ALL_BREAK_LOCATIONS); - // pc points to the instruction after the current one, possibly a break - // location as well. So the "- 1" to exclude it from the search. - it.FindBreakLocationFromAddress(frame->pc() - 1); - // Compute whether or not the target is a call target. bool is_load_or_store = false; bool is_inline_cache_stub = false; bool is_at_restarted_function = false; + bool is_exit = false; + bool is_construct_call = false; Handle call_function_stub; - if (thread_local_.restarter_frame_function_pointer_ == NULL) { - if (RelocInfo::IsCodeTarget(it.rinfo()->rmode())) { - bool is_call_target = false; - Address target = it.rinfo()->target_address(); - Code* code = Code::GetCodeFromTargetAddress(target); - if (code->is_call_stub()) { - is_call_target = true; - } - if (code->is_inline_cache_stub()) { - is_inline_cache_stub = true; - is_load_or_store = !is_call_target; - } - - // Check if target code is CallFunction stub. - Code* maybe_call_function_stub = code; - // If there is a breakpoint at this line look at the original code to - // check if it is a CallFunction stub. - if (it.IsDebugBreak()) { - Address original_target = it.original_rinfo()->target_address(); - maybe_call_function_stub = - Code::GetCodeFromTargetAddress(original_target); - } - if ((maybe_call_function_stub->kind() == Code::STUB && - CodeStub::GetMajorKey(maybe_call_function_stub) == - CodeStub::CallFunction) || - maybe_call_function_stub->is_call_stub()) { - // Save reference to the code as we may need it to find out arguments - // count for 'step in' later. - call_function_stub = Handle(maybe_call_function_stub); + { + // Find the break location where execution has stopped. + DisallowHeapAllocation no_gc; + BreakLocationIterator it(debug_info, ALL_BREAK_LOCATIONS); + + // pc points to the instruction after the current one, possibly a break + // location as well. So the "- 1" to exclude it from the search. + it.FindBreakLocationFromAddress(frame->pc() - 1); + + is_exit = it.IsExit(); + + if (thread_local_.restarter_frame_function_pointer_ == NULL) { + if (RelocInfo::IsCodeTarget(it.rinfo()->rmode())) { + bool is_call_target = false; + Address target = it.rinfo()->target_address(); + Code* code = Code::GetCodeFromTargetAddress(target); + + is_call_target = code->is_call_stub(); + is_construct_call = RelocInfo::IsConstructCall(it.rmode()); + is_inline_cache_stub = code->is_inline_cache_stub(); + is_load_or_store = is_inline_cache_stub && !is_call_target; + + // Check if target code is CallFunction stub. + Code* maybe_call_function_stub = code; + // If there is a breakpoint at this line look at the original code to + // check if it is a CallFunction stub. + if (it.IsDebugBreak()) { + Address original_target = it.original_rinfo()->target_address(); + maybe_call_function_stub = + Code::GetCodeFromTargetAddress(original_target); + } + if ((maybe_call_function_stub->kind() == Code::STUB && + CodeStub::GetMajorKey(maybe_call_function_stub) == + CodeStub::CallFunction) || + maybe_call_function_stub->is_call_stub()) { + // Save reference to the code as we may need it to find out arguments + // count for 'step in' later. + call_function_stub = Handle(maybe_call_function_stub); + } } + } else { + is_at_restarted_function = true; } - } else { - is_at_restarted_function = true; } // If this is the last break code target step out is the only possibility. - if (it.IsExit() || step_action == StepOut) { + if (is_exit || step_action == StepOut) { if (step_action == StepOut) { // Skip step_count frames starting with the current one. while (step_count-- > 0 && !frames_it.done()) { frames_it.Advance(); } } else { - DCHECK(it.IsExit()); + DCHECK(is_exit); frames_it.Advance(); } // Skip builtin functions on the stack. @@ -1443,9 +1415,9 @@ void Debug::PrepareStep(StepAction step_action, // Set target frame pointer. ActivateStepOut(frames_it.frame()); } - } else if (!(is_inline_cache_stub || RelocInfo::IsConstructCall(it.rmode()) || - !call_function_stub.is_null() || is_at_restarted_function) - || step_action == StepNext || step_action == StepMin) { + } else if (!(is_inline_cache_stub || is_construct_call || + !call_function_stub.is_null() || is_at_restarted_function) || + step_action == StepNext || step_action == StepMin) { // Step next or step min. // Fill the current function with one-shot break points. @@ -1532,7 +1504,15 @@ void Debug::PrepareStep(StepAction step_action, } // Step in or Step in min - it.PrepareStepIn(isolate_); + // Step in through construct call requires no changes to the running code. + // Step in through getters/setters should already be prepared as well + // because caller of this function (Debug::PrepareStep) is expected to + // flood the top frame's function with one shot breakpoints. + // Step in through CallFunction stub should also be prepared by caller of + // this function (Debug::PrepareStep) which should flood target function + // with breakpoints. + DCHECK(is_construct_call || is_inline_cache_stub || + !call_function_stub.is_null()); ActivateStepIn(frame); } } diff --git a/src/debug.h b/src/debug.h index 0ec9024..3e2db60 100644 --- a/src/debug.h +++ b/src/debug.h @@ -86,7 +86,6 @@ class BreakLocationIterator { void SetOneShot(); void ClearOneShot(); bool IsStepInLocation(Isolate* isolate); - void PrepareStepIn(Isolate* isolate); bool IsExit() const; bool HasBreakPoint(); bool IsDebugBreak(); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index 9443a6d..5daeee4 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -244,12 +244,6 @@ # Issue 3723. 'regress/regress-3717': [SKIP], - # Issue 3776. - 'debug-constructor': [SKIP], - 'debug-stepframe': [SKIP], - 'debug-stepout-scope-part2': [SKIP], - 'debug-stepout-scope-part5': [SKIP], - 'debug-stepout-scope-part6': [SKIP], }], # 'gc_stress == True' ############################################################################## -- 2.7.4