From 1719a1499a296d58e1a8bcf33be23a02da02be0a Mon Sep 17 00:00:00 2001 From: "peter.rybin@gmail.com" Date: Thu, 3 May 2012 17:31:34 +0000 Subject: [PATCH] Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. Review URL: https://chromiumcodereview.appspot.com/10263002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@11502 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/debug-arm.cc | 4 +- src/debug.cc | 7 +++ src/debug.h | 44 ++++++++++++++ src/frames.h | 3 + src/ia32/debug-ia32.cc | 31 +++++++++- src/liveedit.cc | 61 +++++++++++++++++-- src/mips/debug-mips.cc | 4 +- src/x64/debug-x64.cc | 4 +- test/mjsunit/debug-liveedit-stack-padding.js | 88 ++++++++++++++++++++++++++++ test/mjsunit/mjsunit.status | 1 + 10 files changed, 237 insertions(+), 10 deletions(-) create mode 100644 test/mjsunit/debug-liveedit-stack-padding.js diff --git a/src/arm/debug-arm.cc b/src/arm/debug-arm.cc index 96139a2..3e7a1e9 100644 --- a/src/arm/debug-arm.cc +++ b/src/arm/debug-arm.cc @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -125,6 +125,8 @@ void BreakLocationIterator::ClearDebugBreakAtSlot() { Assembler::kDebugBreakSlotInstructions); } +const bool Debug::FramePaddingLayout::kIsSupported = false; + #define __ ACCESS_MASM(masm) diff --git a/src/debug.cc b/src/debug.cc index 99256ba..88a976f 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -2227,6 +2227,13 @@ void Debug::FramesHaveBeenDropped(StackFrame::Id new_break_frame_id, } +const int Debug::FramePaddingLayout::kInitialSize = 1; + + +// Any even value bigger than kInitialSize as needed for stack scanning. +const int Debug::FramePaddingLayout::kPaddingValue = kInitialSize + 1; + + bool Debug::IsDebugGlobal(GlobalObject* global) { return IsLoaded() && global == debug_context()->global(); } diff --git a/src/debug.h b/src/debug.h index 7ec7801..2adbd24 100644 --- a/src/debug.h +++ b/src/debug.h @@ -457,6 +457,50 @@ class Debug { // Architecture-specific constant. static const bool kFrameDropperSupported; + /** + * Defines layout of a stack frame that supports padding. This is a regular + * internal frame that has a flexible stack structure. LiveEdit can shift + * its lower part up the stack, taking up the 'padding' space when additional + * stack memory is required. + * Such frame is expected immediately above the topmost JavaScript frame. + * + * Stack Layout: + * --- Top + * LiveEdit routine frames + * --- + * C frames of debug handler + * --- + * ... + * --- + * An internal frame that has n padding words: + * - any number of words as needed by code -- upper part of frame + * - padding size: a Smi storing n -- current size of padding + * - padding: n words filled with kPaddingValue in form of Smi + * - 3 context/type words of a regular InternalFrame + * - fp + * --- + * Topmost JavaScript frame + * --- + * ... + * --- Bottom + */ + class FramePaddingLayout : public AllStatic { + public: + // Architecture-specific constant. + static const bool kIsSupported; + + // A size of frame base including fp. Padding words starts right above + // the base. + static const int kFrameBaseSize = 4; + + // A number of words that should be reserved on stack for the LiveEdit use. + // Normally equals 1. Stored on stack in form of Smi. + static const int kInitialSize; + // A value that padding words are filled with (in form of Smi). Going + // bottom-top, the first word not having this value is a counter word. + static const int kPaddingValue; + }; + private: explicit Debug(Isolate* isolate); ~Debug(); diff --git a/src/frames.h b/src/frames.h index 7178bd4..78cdd0c 100644 --- a/src/frames.h +++ b/src/frames.h @@ -211,6 +211,9 @@ class StackFrame BASE_EMBEDDED { virtual void SetCallerFp(Address caller_fp) = 0; + // Manually changes value of fp in this object. + void UpdateFp(Address fp) { state_.fp = fp; } + Address* pc_address() const { return state_.pc_address; } // Get the id of this stack frame. diff --git a/src/ia32/debug-ia32.cc b/src/ia32/debug-ia32.cc index 710cbaf..901e38b 100644 --- a/src/ia32/debug-ia32.cc +++ b/src/ia32/debug-ia32.cc @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -91,9 +91,11 @@ void BreakLocationIterator::ClearDebugBreakAtSlot() { rinfo()->PatchCode(original_rinfo()->pc(), Assembler::kDebugBreakSlotLength); } +// All debug break stubs support padding for LiveEdit. +const bool Debug::FramePaddingLayout::kIsSupported = true; -#define __ ACCESS_MASM(masm) +#define __ ACCESS_MASM(masm) static void Generate_DebugBreakCallHelper(MacroAssembler* masm, RegList object_regs, @@ -103,6 +105,13 @@ static void Generate_DebugBreakCallHelper(MacroAssembler* masm, { FrameScope scope(masm, StackFrame::INTERNAL); + // Load padding words on stack. + for (int i = 0; i < Debug::FramePaddingLayout::kInitialSize; i++) { + __ push(Immediate(Smi::FromInt( + Debug::FramePaddingLayout::kPaddingValue))); + } + __ push(Immediate(Smi::FromInt(Debug::FramePaddingLayout::kInitialSize))); + // Store the registers containing live values on the expression stack to // make sure that these are correctly updated during GC. Non object values // are stored as a smi causing it to be untouched by GC. @@ -134,6 +143,10 @@ static void Generate_DebugBreakCallHelper(MacroAssembler* masm, CEntryStub ceb(1); __ CallStub(&ceb); + // Automatically find register that could be used after register restore. + // We need one register for padding skip instructions. + Register unused_reg = { -1 }; + // Restore the register values containing object pointers from the // expression stack. for (int i = kNumJSCallerSaved; --i >= 0;) { @@ -142,15 +155,29 @@ static void Generate_DebugBreakCallHelper(MacroAssembler* masm, if (FLAG_debug_code) { __ Set(reg, Immediate(kDebugZapValue)); } + bool taken = reg.code() == esi.code(); if ((object_regs & (1 << r)) != 0) { __ pop(reg); + taken = true; } if ((non_object_regs & (1 << r)) != 0) { __ pop(reg); __ SmiUntag(reg); + taken = true; + } + if (!taken) { + unused_reg = reg; } } + ASSERT(unused_reg.code() != -1); + + // Read current padding counter and skip corresponding number of words. + __ pop(unused_reg); + // We divide stored value by 2 (untagging) and multiply it by word's size. + STATIC_ASSERT(kSmiTagSize == 1); + __ lea(esp, Operand(esp, unused_reg, times_half_pointer_size, 0)); + // Get rid of the internal frame. } diff --git a/src/liveedit.cc b/src/liveedit.cc index 9c5294a..2bab88f 100644 --- a/src/liveedit.cc +++ b/src/liveedit.cc @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -30,6 +30,7 @@ #include "liveedit.h" +#include "code-stubs.h" #include "compilation-cache.h" #include "compiler.h" #include "debug.h" @@ -1475,26 +1476,36 @@ static const char* DropFrames(Vector frames, // Check the nature of the top frame. Isolate* isolate = Isolate::Current(); Code* pre_top_frame_code = pre_top_frame->LookupCode(); + bool frame_has_padding; if (pre_top_frame_code->is_inline_cache_stub() && pre_top_frame_code->ic_state() == DEBUG_BREAK) { // OK, we can drop inline cache calls. *mode = Debug::FRAME_DROPPED_IN_IC_CALL; + frame_has_padding = Debug::FramePaddingLayout::kIsSupported; } else if (pre_top_frame_code == isolate->debug()->debug_break_slot()) { // OK, we can drop debug break slot. *mode = Debug::FRAME_DROPPED_IN_DEBUG_SLOT_CALL; + frame_has_padding = Debug::FramePaddingLayout::kIsSupported; } else if (pre_top_frame_code == isolate->builtins()->builtin( Builtins::kFrameDropper_LiveEdit)) { // OK, we can drop our own code. *mode = Debug::FRAME_DROPPED_IN_DIRECT_CALL; + frame_has_padding = false; } else if (pre_top_frame_code == isolate->builtins()->builtin(Builtins::kReturn_DebugBreak)) { *mode = Debug::FRAME_DROPPED_IN_RETURN_CALL; + frame_has_padding = Debug::FramePaddingLayout::kIsSupported; } else if (pre_top_frame_code->kind() == Code::STUB && - pre_top_frame_code->major_key()) { - // Entry from our unit tests, it's fine, we support this case. + pre_top_frame_code->major_key() == CodeStub::CEntry) { + // Entry from our unit tests on 'debugger' statement. + // It's fine, we support this case. *mode = Debug::FRAME_DROPPED_IN_DIRECT_CALL; + // We don't have a padding from 'debugger' statement call. + // Here the stub is CEntry, it's not debug-only and can't be padded. + // If anyone would complain, a proxy padded stub could be added. + frame_has_padding = false; } else { return "Unknown structure of stack above changing function"; } @@ -1504,8 +1515,48 @@ static const char* DropFrames(Vector frames, - Debug::kFrameDropperFrameSize * kPointerSize // Size of the new frame. + kPointerSize; // Bigger address end is exclusive. + Address* top_frame_pc_address = top_frame->pc_address(); + + // top_frame may be damaged below this point. Do not used it. + ASSERT(!(top_frame = NULL)); + if (unused_stack_top > unused_stack_bottom) { - return "Not enough space for frame dropper frame"; + if (frame_has_padding) { + int shortage_bytes = unused_stack_top - unused_stack_bottom; + + Address padding_start = pre_top_frame->fp() - + Debug::FramePaddingLayout::kFrameBaseSize * kPointerSize; + + Address padding_pointer = padding_start; + Smi* padding_object = + Smi::FromInt(Debug::FramePaddingLayout::kPaddingValue); + while (Memory::Object_at(padding_pointer) == padding_object) { + padding_pointer -= kPointerSize; + } + int padding_counter = + Smi::cast(Memory::Object_at(padding_pointer))->value(); + if (padding_counter * kPointerSize < shortage_bytes) { + return "Not enough space for frame dropper frame " + "(even with padding frame)"; + } + Memory::Object_at(padding_pointer) = + Smi::FromInt(padding_counter - shortage_bytes / kPointerSize); + + StackFrame* pre_pre_frame = frames[top_frame_index - 2]; + + memmove(padding_start + kPointerSize - shortage_bytes, + padding_start + kPointerSize, + Debug::FramePaddingLayout::kFrameBaseSize * kPointerSize); + + pre_top_frame->UpdateFp(pre_top_frame->fp() - shortage_bytes); + pre_pre_frame->SetCallerFp(pre_top_frame->fp()); + unused_stack_top -= shortage_bytes; + + STATIC_ASSERT(sizeof(Address) == kPointerSize); + top_frame_pc_address -= shortage_bytes / kPointerSize; + } else { + return "Not enough space for frame dropper frame"; + } } // Committing now. After this point we should return only NULL value. @@ -1515,7 +1566,7 @@ static const char* DropFrames(Vector frames, ASSERT(!FixTryCatchHandler(pre_top_frame, bottom_js_frame)); Handle code = Isolate::Current()->builtins()->FrameDropper_LiveEdit(); - top_frame->set_pc(code->entry()); + *top_frame_pc_address = code->entry(); pre_top_frame->SetCallerFp(bottom_js_frame->fp()); *restarter_frame_function_pointer = diff --git a/src/mips/debug-mips.cc b/src/mips/debug-mips.cc index 83f5f50..3be1e4d 100644 --- a/src/mips/debug-mips.cc +++ b/src/mips/debug-mips.cc @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -116,6 +116,8 @@ void BreakLocationIterator::ClearDebugBreakAtSlot() { Assembler::kDebugBreakSlotInstructions); } +const bool Debug::FramePaddingLayout::kIsSupported = false; + #define __ ACCESS_MASM(masm) diff --git a/src/x64/debug-x64.cc b/src/x64/debug-x64.cc index eec83d9..94a50eb 100644 --- a/src/x64/debug-x64.cc +++ b/src/x64/debug-x64.cc @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -91,6 +91,8 @@ void BreakLocationIterator::ClearDebugBreakAtSlot() { rinfo()->PatchCode(original_rinfo()->pc(), Assembler::kDebugBreakSlotLength); } +const bool Debug::FramePaddingLayout::kIsSupported = false; + #define __ ACCESS_MASM(masm) diff --git a/test/mjsunit/debug-liveedit-stack-padding.js b/test/mjsunit/debug-liveedit-stack-padding.js new file mode 100644 index 0000000..36de356 --- /dev/null +++ b/test/mjsunit/debug-liveedit-stack-padding.js @@ -0,0 +1,88 @@ +// Copyright 2012 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Flags: --expose-debug-as debug +// Get the Debug object exposed from the debug context global object. + +Debug = debug.Debug; + +SlimFunction = eval( + "(function() {\n " + + " return 'Cat';\n" + + "})\n" +); + +var script = Debug.findScript(SlimFunction); + +Debug.setScriptBreakPointById(script.id, 1, 0); + +var orig_animal = "'Cat'"; +var patch_pos = script.source.indexOf(orig_animal); +var new_animal_patch = "'Capybara'"; + +debugger_handler = (function() { + var already_called = false; + return function() { + if (already_called) { + return; + } + already_called = true; + + var change_log = new Array(); + try { + Debug.LiveEdit.TestApi.ApplySingleChunkPatch(script, patch_pos, + orig_animal.length, new_animal_patch, change_log); + } finally { + print("Change log: " + JSON.stringify(change_log) + "\n"); + } + }; +})(); + +var saved_exception = null; + +function listener(event, exec_state, event_data, data) { + if (event == Debug.DebugEvent.Break) { + try { + debugger_handler(); + } catch (e) { + saved_exception = e; + } + } else { + print("Other: " + event); + } +} + +Debug.setListener(listener); + +var animal = SlimFunction(); + +if (saved_exception) { + print("Exception: " + saved_exception); + assertUnreachable(); +} + +assertEquals("Capybara", animal); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index a1b9270..b3f52a4 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -64,6 +64,7 @@ regress/regress-524: (PASS || TIMEOUT), SKIP if $mode == debug # Stack manipulations in LiveEdit are buggy - see bug 915 debug-liveedit-check-stack: SKIP debug-liveedit-patch-positions-replace: SKIP +debug-liveedit-stack-padding.js: SKIP # Test Crankshaft compilation time. Expected to take too long in debug mode. regress/regress-1969: PASS, SKIP if $mode == debug -- 2.7.4