From ab3afc5722077e59577ea13b0058b1e28d431817 Mon Sep 17 00:00:00 2001 From: "yangguo@chromium.org" Date: Thu, 22 May 2014 07:32:59 +0000 Subject: [PATCH] Reland "Prevent liveedit on or under generators with open activations" The change relative to the previous CL is a logic change in DropActivationsInActiveThreadImpl. The previous CL skipped the matcher unless the frame was a JS frame; this was correct for MultipleFunctionTarget but not for SingleFrameTarget. I have not been able to reproduce the original failures on either architecture (ia32 or x64; stack frame dropping is unsupported on other architectures). R=yangguo@chromium.org LOG=N TEST=mjsunit/harmony/generators-debug-liveedit.js BUG= Review URL: https://codereview.chromium.org/270283002 Patch from Andy Wingo . git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21419 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/liveedit-debugger.js | 4 +- src/liveedit.cc | 96 +++++++++++++---- src/liveedit.h | 9 +- src/objects-inl.h | 8 ++ src/objects.h | 2 + test/mjsunit/harmony/generators-debug-liveedit.js | 119 ++++++++++++++++++++++ test/mjsunit/mjsunit.status | 4 + 7 files changed, 221 insertions(+), 21 deletions(-) create mode 100644 test/mjsunit/harmony/generators-debug-liveedit.js diff --git a/src/liveedit-debugger.js b/src/liveedit-debugger.js index 021a4f0..07214f9 100644 --- a/src/liveedit-debugger.js +++ b/src/liveedit-debugger.js @@ -946,7 +946,9 @@ Debug.LiveEdit = new function() { BLOCKED_ON_ACTIVE_STACK: 2, BLOCKED_ON_OTHER_STACK: 3, BLOCKED_UNDER_NATIVE_CODE: 4, - REPLACED_ON_ACTIVE_STACK: 5 + REPLACED_ON_ACTIVE_STACK: 5, + BLOCKED_UNDER_GENERATOR: 6, + BLOCKED_ACTIVE_GENERATOR: 7 }; FunctionPatchabilityStatus.SymbolName = function(code) { diff --git a/src/liveedit.cc b/src/liveedit.cc index e6bb4b2..0158577 100644 --- a/src/liveedit.cc +++ b/src/liveedit.cc @@ -1682,11 +1682,6 @@ static const char* DropFrames(Vector frames, } -static bool IsDropableFrame(StackFrame* frame) { - return !frame->is_exit(); -} - - // Describes a set of call frames that execute any of listed functions. // Finding no such frames does not mean error. class MultipleFunctionTarget { @@ -1740,12 +1735,20 @@ static const char* DropActivationsInActiveThreadImpl( bool target_frame_found = false; int bottom_js_frame_index = top_frame_index; - bool c_code_found = false; + bool non_droppable_frame_found = false; + LiveEdit::FunctionPatchabilityStatus non_droppable_reason; for (; frame_index < frames.length(); frame_index++) { StackFrame* frame = frames[frame_index]; - if (!IsDropableFrame(frame)) { - c_code_found = true; + if (frame->is_exit()) { + non_droppable_frame_found = true; + non_droppable_reason = LiveEdit::FUNCTION_BLOCKED_UNDER_NATIVE_CODE; + break; + } + if (frame->is_java_script() && + JavaScriptFrame::cast(frame)->function()->shared()->is_generator()) { + non_droppable_frame_found = true; + non_droppable_reason = LiveEdit::FUNCTION_BLOCKED_UNDER_GENERATOR; break; } if (target.MatchActivation( @@ -1755,15 +1758,15 @@ static const char* DropActivationsInActiveThreadImpl( } } - if (c_code_found) { - // There is a C frames on stack. Check that there are no target frames - // below them. + if (non_droppable_frame_found) { + // There is a C or generator frame on stack. We can't drop C frames, and we + // can't restart generators. Check that there are no target frames below + // them. for (; frame_index < frames.length(); frame_index++) { StackFrame* frame = frames[frame_index]; if (frame->is_java_script()) { - if (target.MatchActivation( - frame, LiveEdit::FUNCTION_BLOCKED_UNDER_NATIVE_CODE)) { - // Cannot drop frame under C frames. + if (target.MatchActivation(frame, non_droppable_reason)) { + // Fail. return NULL; } } @@ -1833,6 +1836,47 @@ static const char* DropActivationsInActiveThread( } +bool LiveEdit::FindActiveGenerators(Handle shared_info_array, + Handle result, + int len) { + Isolate* isolate = shared_info_array->GetIsolate(); + Heap* heap = isolate->heap(); + heap->EnsureHeapIsIterable(); + bool found_suspended_activations = false; + + ASSERT_LE(len, result->length()); + + DisallowHeapAllocation no_allocation; + + FunctionPatchabilityStatus active = FUNCTION_BLOCKED_ACTIVE_GENERATOR; + + HeapIterator iterator(heap); + HeapObject* obj = NULL; + while ((obj = iterator.next()) != NULL) { + if (!obj->IsJSGeneratorObject()) continue; + + JSGeneratorObject* gen = JSGeneratorObject::cast(obj); + if (gen->is_closed()) continue; + + HandleScope scope(isolate); + + for (int i = 0; i < len; i++) { + Handle jsvalue = + Handle::cast(FixedArray::get(shared_info_array, i)); + Handle shared = + UnwrapSharedFunctionInfoFromJSValue(jsvalue); + + if (gen->function()->shared() == *shared) { + result->set(i, Smi::FromInt(active)); + found_suspended_activations = true; + } + } + } + + return found_suspended_activations; +} + + class InactiveThreadActivationsChecker : public ThreadVisitor { public: InactiveThreadActivationsChecker(Handle shared_info_array, @@ -1863,18 +1907,29 @@ Handle LiveEdit::CheckAndDropActivations( Isolate* isolate = shared_info_array->GetIsolate(); int len = GetArrayLength(shared_info_array); + CHECK(shared_info_array->HasFastElements()); + Handle shared_info_array_elements( + FixedArray::cast(shared_info_array->elements())); + Handle result = isolate->factory()->NewJSArray(len); + Handle result_elements = + JSObject::EnsureWritableFastElements(result); // Fill the default values. for (int i = 0; i < len; i++) { - SetElementSloppy( - result, - i, - Handle(Smi::FromInt(FUNCTION_AVAILABLE_FOR_PATCH), isolate)); + FunctionPatchabilityStatus status = FUNCTION_AVAILABLE_FOR_PATCH; + result_elements->set(i, Smi::FromInt(status)); } + // Scan the heap for active generators -- those that are either currently + // running (as we wouldn't want to restart them, because we don't know where + // to restart them from) or suspended. Fail if any one corresponds to the set + // of functions being edited. + if (FindActiveGenerators(shared_info_array_elements, result_elements, len)) { + return result; + } - // First check inactive threads. Fail if some functions are blocked there. + // Check inactive threads. Fail if some functions are blocked there. InactiveThreadActivationsChecker inactive_threads_checker(shared_info_array, result); isolate->thread_manager()->IterateArchivedThreads( @@ -1937,6 +1992,9 @@ const char* LiveEdit::RestartFrame(JavaScriptFrame* frame) { if (target.saved_status() == LiveEdit::FUNCTION_BLOCKED_UNDER_NATIVE_CODE) { return "Function is blocked under native code"; } + if (target.saved_status() == LiveEdit::FUNCTION_BLOCKED_UNDER_GENERATOR) { + return "Function is blocked under a generator activation"; + } return NULL; } diff --git a/src/liveedit.h b/src/liveedit.h index e06688a..b22d01a 100644 --- a/src/liveedit.h +++ b/src/liveedit.h @@ -89,6 +89,11 @@ class LiveEdit : AllStatic { Handle orig_function_shared, Handle subst_function_shared); + // Find open generator activations, and set corresponding "result" elements to + // FUNCTION_BLOCKED_ACTIVE_GENERATOR. + static bool FindActiveGenerators(Handle shared_info_array, + Handle result, int len); + // Checks listed functions on stack and return array with corresponding // FunctionPatchabilityStatus statuses; extra array element may // contain general error message. Modifies the current stack and @@ -107,7 +112,9 @@ class LiveEdit : AllStatic { FUNCTION_BLOCKED_ON_ACTIVE_STACK = 2, FUNCTION_BLOCKED_ON_OTHER_STACK = 3, FUNCTION_BLOCKED_UNDER_NATIVE_CODE = 4, - FUNCTION_REPLACED_ON_ACTIVE_STACK = 5 + FUNCTION_REPLACED_ON_ACTIVE_STACK = 5, + FUNCTION_BLOCKED_UNDER_GENERATOR = 6, + FUNCTION_BLOCKED_ACTIVE_GENERATOR = 7 }; // Compares 2 strings line-by-line, then token-wise and returns diff in form diff --git a/src/objects-inl.h b/src/objects-inl.h index dca3eca..319e6a8 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -5715,6 +5715,14 @@ bool JSGeneratorObject::is_suspended() { return continuation() > 0; } +bool JSGeneratorObject::is_closed() { + return continuation() == kGeneratorClosed; +} + +bool JSGeneratorObject::is_executing() { + return continuation() == kGeneratorExecuting; +} + JSGeneratorObject* JSGeneratorObject::cast(Object* obj) { ASSERT(obj->IsJSGeneratorObject()); ASSERT(HeapObject::cast(obj)->Size() == JSGeneratorObject::kSize); diff --git a/src/objects.h b/src/objects.h index 944efb9..a161b0b 100644 --- a/src/objects.h +++ b/src/objects.h @@ -7495,6 +7495,8 @@ class JSGeneratorObject: public JSObject { // cannot be resumed. inline int continuation(); inline void set_continuation(int continuation); + inline bool is_closed(); + inline bool is_executing(); inline bool is_suspended(); // [operand_stack]: Saved operand stack. diff --git a/test/mjsunit/harmony/generators-debug-liveedit.js b/test/mjsunit/harmony/generators-debug-liveedit.js new file mode 100644 index 0000000..341ef48 --- /dev/null +++ b/test/mjsunit/harmony/generators-debug-liveedit.js @@ -0,0 +1,119 @@ +// Copyright 2014 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --expose-debug-as debug --harmony-generators + +var Debug = debug.Debug; +var LiveEdit = Debug.LiveEdit; + +unique_id = 0; + +var Generator = (function*(){}).constructor; + +function assertIteratorResult(value, done, result) { + assertEquals({value: value, done: done}, result); +} + +function MakeGenerator() { + // Prevents eval script caching. + unique_id++; + return Generator('callback', + "/* " + unique_id + "*/\n" + + "yield callback();\n" + + "return 'Cat';\n"); +} + +function MakeFunction() { + // Prevents eval script caching. + unique_id++; + return Function('callback', + "/* " + unique_id + "*/\n" + + "callback();\n" + + "return 'Cat';\n"); +} + +// First, try MakeGenerator with no perturbations. +(function(){ + var generator = MakeGenerator(); + function callback() {}; + var iter = generator(callback); + assertIteratorResult(undefined, false, iter.next()); + assertIteratorResult("Cat", true, iter.next()); +})(); + +function patch(fun, from, to) { + function debug() { + var log = new Array(); + var script = Debug.findScript(fun); + var pos = script.source.indexOf(from); + try { + LiveEdit.TestApi.ApplySingleChunkPatch(script, pos, from.length, to, + log); + } finally { + print("Change log: " + JSON.stringify(log) + "\n"); + } + } + Debug.ExecuteInDebugContext(debug, false); +} + +// Try to edit a MakeGenerator while it's running, then again while it's +// stopped. +(function(){ + var generator = MakeGenerator(); + + var gen_patch_attempted = false; + function attempt_gen_patch() { + assertFalse(gen_patch_attempted); + gen_patch_attempted = true; + assertThrows(function() { patch(generator, "'Cat'", "'Capybara'") }, + LiveEdit.Failure); + }; + var iter = generator(attempt_gen_patch); + assertIteratorResult(undefined, false, iter.next()); + // Patch should not succeed because there is a live generator activation on + // the stack. + assertIteratorResult("Cat", true, iter.next()); + assertTrue(gen_patch_attempted); + + // At this point one iterator is live, but closed, so the patch will succeed. + patch(generator, "'Cat'", "'Capybara'"); + iter = generator(function(){}); + assertIteratorResult(undefined, false, iter.next()); + // Patch successful. + assertIteratorResult("Capybara", true, iter.next()); + + // Patching will fail however when a live iterator is suspended. + iter = generator(function(){}); + assertIteratorResult(undefined, false, iter.next()); + assertThrows(function() { patch(generator, "'Capybara'", "'Tapir'") }, + LiveEdit.Failure); + assertIteratorResult("Capybara", true, iter.next()); + + // Try to patch functions with activations inside and outside generator + // function activations. We should succeed in the former case, but not in the + // latter. + var fun_outside = MakeFunction(); + var fun_inside = MakeFunction(); + var fun_patch_attempted = false; + var fun_patch_restarted = false; + function attempt_fun_patches() { + if (fun_patch_attempted) { + assertFalse(fun_patch_restarted); + fun_patch_restarted = true; + return; + } + fun_patch_attempted = true; + // Patching outside a generator activation must fail. + assertThrows(function() { patch(fun_outside, "'Cat'", "'Cobra'") }, + LiveEdit.Failure); + // Patching inside a generator activation may succeed. + patch(fun_inside, "'Cat'", "'Koala'"); + } + iter = generator(function() { return fun_inside(attempt_fun_patches) }); + assertEquals('Cat', + fun_outside(function () { + assertIteratorResult('Koala', false, iter.next()); + assertTrue(fun_patch_restarted); + })); +})(); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index e460e7c..2df8747 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -206,6 +206,7 @@ 'debug-liveedit-stack-padding': [SKIP], 'debug-liveedit-restart-frame': [SKIP], 'debug-liveedit-double-call': [SKIP], + 'harmony/generators-debug-liveedit': [SKIP], # BUG(v8:3147). It works on other architectures by accident. 'regress/regress-conditional-position': [FAIL], @@ -302,6 +303,7 @@ 'debug-liveedit-stack-padding': [SKIP], 'debug-liveedit-restart-frame': [SKIP], 'debug-liveedit-double-call': [SKIP], + 'harmony/generators-debug-liveedit': [SKIP], # Currently always deopt on minus zero 'math-floor-of-div-minus-zero': [SKIP], @@ -353,6 +355,7 @@ 'debug-liveedit-stack-padding': [SKIP], 'debug-liveedit-restart-frame': [SKIP], 'debug-liveedit-double-call': [SKIP], + 'harmony/generators-debug-liveedit': [SKIP], # Currently always deopt on minus zero 'math-floor-of-div-minus-zero': [SKIP], @@ -373,6 +376,7 @@ 'debug-liveedit-stack-padding': [SKIP], 'debug-liveedit-restart-frame': [SKIP], 'debug-liveedit-double-call': [SKIP], + 'harmony/generators-debug-liveedit': [SKIP], # NaCl builds have problems with this test since Pepper_28. # V8 Issue 2786 -- 2.7.4