From e12ae547cf7fc23ad1921bf4f078177dfafcd563 Mon Sep 17 00:00:00 2001 From: "wingo@igalia.com" Date: Wed, 23 Apr 2014 08:58:41 +0000 Subject: [PATCH] Avoid exposing compiler-allocated temporaries to the debugger R=yangguo@chromium.org BUG= Review URL: https://codereview.chromium.org/245963006 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@20899 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/objects.h | 4 ++ src/runtime.cc | 39 ++++++++++++------- src/scopeinfo.cc | 20 ++++++++-- .../mjsunit/bugs/harmony/debug-blockscopes.js | 8 ---- test/mjsunit/debug-scopes.js | 4 -- test/mjsunit/harmony/debug-blockscopes.js | 18 +++------ 6 files changed, 52 insertions(+), 41 deletions(-) diff --git a/src/objects.h b/src/objects.h index a190f8dfe..71eeaa2e5 100644 --- a/src/objects.h +++ b/src/objects.h @@ -4663,6 +4663,10 @@ class ScopeInfo : public FixedArray { // Return the initialization flag of the given context local. InitializationFlag ContextLocalInitFlag(int var); + // Return true if this local was introduced by the compiler, and should not be + // exposed to the user in a debugger. + bool LocalIsSynthetic(int var); + // Lookup support for serialized scope info. Returns the // the stack slot index for a given slot name if the slot is // present; otherwise returns a value < 0. The name must be an internalized diff --git a/src/runtime.cc b/src/runtime.cc index 1fa6da831..c508b4a64 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -11200,33 +11200,44 @@ RUNTIME_FUNCTION(Runtime_GetFrameDetails) { ASSERT(*scope_info != ScopeInfo::Empty(isolate)); // Get the locals names and values into a temporary array. - // - // TODO(1240907): Hide compiler-introduced stack variables - // (e.g. .result)? For users of the debugger, they will probably be - // confusing. + int local_count = scope_info->LocalCount(); + for (int slot = 0; slot < scope_info->LocalCount(); ++slot) { + // Hide compiler-introduced temporary variables, whether on the stack or on + // the context. + if (scope_info->LocalIsSynthetic(slot)) + local_count--; + } + Handle locals = - isolate->factory()->NewFixedArray(scope_info->LocalCount() * 2); + isolate->factory()->NewFixedArray(local_count * 2); // Fill in the values of the locals. + int local = 0; int i = 0; for (; i < scope_info->StackLocalCount(); ++i) { // Use the value from the stack. - locals->set(i * 2, scope_info->LocalName(i)); - locals->set(i * 2 + 1, frame_inspector.GetExpression(i)); + if (scope_info->LocalIsSynthetic(i)) + continue; + locals->set(local * 2, scope_info->LocalName(i)); + locals->set(local * 2 + 1, frame_inspector.GetExpression(i)); + local++; } - if (i < scope_info->LocalCount()) { + if (local < local_count) { // Get the context containing declarations. Handle context( Context::cast(it.frame()->context())->declaration_context()); for (; i < scope_info->LocalCount(); ++i) { + if (scope_info->LocalIsSynthetic(i)) + continue; Handle name(scope_info->LocalName(i)); VariableMode mode; InitializationFlag init_flag; - locals->set(i * 2, *name); + locals->set(local * 2, *name); int context_slot_index = scope_info->ContextSlotIndex(*name, &mode, &init_flag); Object* value = context->get(context_slot_index); - locals->set(i * 2 + 1, value); + locals->set(local * 2 + 1, value); + local++; } } @@ -11287,7 +11298,7 @@ RUNTIME_FUNCTION(Runtime_GetFrameDetails) { // Calculate the size of the result. int details_size = kFrameDetailsFirstDynamicIndex + - 2 * (argument_count + scope_info->LocalCount()) + + 2 * (argument_count + local_count) + (at_return ? 1 : 0); Handle details = isolate->factory()->NewFixedArray(details_size); @@ -11302,7 +11313,7 @@ RUNTIME_FUNCTION(Runtime_GetFrameDetails) { // Add the locals count details->set(kFrameDetailsLocalCountIndex, - Smi::FromInt(scope_info->LocalCount())); + Smi::FromInt(local_count)); // Add the source position. if (position != RelocInfo::kNoPosition) { @@ -11353,7 +11364,7 @@ RUNTIME_FUNCTION(Runtime_GetFrameDetails) { } // Add locals name and value from the temporary copy from the function frame. - for (int i = 0; i < scope_info->LocalCount() * 2; i++) { + for (int i = 0; i < local_count * 2; i++) { details->set(details_index++, locals->get(i)); } @@ -11434,6 +11445,7 @@ static MaybeHandle MaterializeStackLocalsWithFrameInspector( // Second fill all stack locals. for (int i = 0; i < scope_info->StackLocalCount(); ++i) { + if (scope_info->LocalIsSynthetic(i)) continue; Handle name(scope_info->StackLocalName(i)); Handle value(frame_inspector->GetExpression(i), isolate); if (value->IsTheHole()) continue; @@ -11478,6 +11490,7 @@ static void UpdateStackLocalsFromMaterializedObject(Isolate* isolate, // Stack locals. for (int i = 0; i < scope_info->StackLocalCount(); ++i) { + if (scope_info->LocalIsSynthetic(i)) continue; if (frame->GetExpression(i)->IsTheHole()) continue; HandleScope scope(isolate); Handle value = Object::GetPropertyOrElement( diff --git a/src/scopeinfo.cc b/src/scopeinfo.cc index 157cdfa4b..564565ff3 100644 --- a/src/scopeinfo.cc +++ b/src/scopeinfo.cc @@ -278,6 +278,17 @@ InitializationFlag ScopeInfo::ContextLocalInitFlag(int var) { } +bool ScopeInfo::LocalIsSynthetic(int var) { + ASSERT(0 <= var && var < LocalCount()); + // There's currently no flag stored on the ScopeInfo to indicate that a + // variable is a compiler-introduced temporary. However, to avoid conflict + // with user declarations, the current temporaries like .generator_object and + // .result start with a dot, so we can use that as a flag. It's a hack! + Handle name(LocalName(var)); + return name->length() > 0 && name->Get(0) == '.'; +} + + int ScopeInfo::StackSlotIndex(String* name) { ASSERT(name->IsInternalizedString()); if (length() > 0) { @@ -368,16 +379,17 @@ bool ScopeInfo::CopyContextLocalsToScopeObject(Handle scope_info, int local_count = scope_info->ContextLocalCount(); if (local_count == 0) return true; // Fill all context locals to the context extension. + int first_context_var = scope_info->StackLocalCount(); int start = scope_info->ContextLocalNameEntriesIndex(); - int end = start + local_count; - for (int i = start; i < end; ++i) { - int context_index = Context::MIN_CONTEXT_SLOTS + i - start; + for (int i = 0; i < local_count; ++i) { + if (scope_info->LocalIsSynthetic(first_context_var + i)) continue; + int context_index = Context::MIN_CONTEXT_SLOTS + i; RETURN_ON_EXCEPTION_VALUE( isolate, Runtime::SetObjectProperty( isolate, scope_object, - Handle(String::cast(scope_info->get(i))), + Handle(String::cast(scope_info->get(i + start))), Handle(context->get(context_index), isolate), ::NONE, SLOPPY), diff --git a/test/mjsunit/bugs/harmony/debug-blockscopes.js b/test/mjsunit/bugs/harmony/debug-blockscopes.js index fda32eb3d..9ef8efbc0 100644 --- a/test/mjsunit/bugs/harmony/debug-blockscopes.js +++ b/test/mjsunit/bugs/harmony/debug-blockscopes.js @@ -144,18 +144,10 @@ function CheckScopeContent(content, number, exec_state) { if (!scope.scopeObject().property('arguments').isUndefined()) { scope_size--; } - // Also ignore synthetic variable from catch block. - if (!scope.scopeObject().property('.catch-var').isUndefined()) { - scope_size--; - } // Skip property with empty name. if (!scope.scopeObject().property('').isUndefined()) { scope_size--; } - // Also ignore synthetic variable from block scopes. - if (!scope.scopeObject().property('.block').isUndefined()) { - scope_size--; - } if (count != scope_size) { print('Names found in scope:'); diff --git a/test/mjsunit/debug-scopes.js b/test/mjsunit/debug-scopes.js index f5b5ec913..ce37d2402 100644 --- a/test/mjsunit/debug-scopes.js +++ b/test/mjsunit/debug-scopes.js @@ -166,10 +166,6 @@ function CheckScopeContent(content, number, exec_state) { if (!scope.scopeObject().property('arguments').isUndefined()) { scope_size--; } - // Also ignore synthetic variable from catch block. - if (!scope.scopeObject().property('.catch-var').isUndefined()) { - scope_size--; - } // Skip property with empty name. if (!scope.scopeObject().property('').isUndefined()) { scope_size--; diff --git a/test/mjsunit/harmony/debug-blockscopes.js b/test/mjsunit/harmony/debug-blockscopes.js index ca2ab9e5a..f56a306b6 100644 --- a/test/mjsunit/harmony/debug-blockscopes.js +++ b/test/mjsunit/harmony/debug-blockscopes.js @@ -147,18 +147,10 @@ function CheckScopeContent(content, number, exec_state) { if (!scope.scopeObject().property('arguments').isUndefined()) { scope_size--; } - // Also ignore synthetic variable from catch block. - if (!scope.scopeObject().property('.catch-var').isUndefined()) { - scope_size--; - } // Skip property with empty name. if (!scope.scopeObject().property('').isUndefined()) { scope_size--; } - // Also ignore synthetic variable from block scopes. - if (!scope.scopeObject().property('.block').isUndefined()) { - scope_size--; - } if (count != scope_size) { print('Names found in scope:'); @@ -375,8 +367,9 @@ listener_delegate = function(exec_state) { debug.ScopeType.Local, debug.ScopeType.Global], exec_state); CheckScopeContent({x:'y'}, 0, exec_state); - // The function scope contains a temporary iteration variable. - CheckScopeContent({'.for.x':'y'}, 1, exec_state); + // The function scope contains a temporary iteration variable, but it is + // hidden to the debugger. + CheckScopeContent({}, 1, exec_state); }; for_loop_1(); EndTest(); @@ -400,8 +393,9 @@ listener_delegate = function(exec_state) { debug.ScopeType.Global], exec_state); CheckScopeContent({x:3}, 0, exec_state); CheckScopeContent({x:'y'}, 1, exec_state); - // The function scope contains a temporary iteration variable. - CheckScopeContent({'.for.x':'y'}, 2, exec_state); + // The function scope contains a temporary iteration variable, hidden to the + // debugger. + CheckScopeContent({}, 2, exec_state); }; for_loop_2(); EndTest(); -- 2.34.1