Reviewers: Yang,

Message:
PTAL :)  "LocalIsSynthetic" might not be the right name.

Description:
Avoid exposing compiler-allocated temporaries to the debugger

R=
BUG=

Please review this at https://codereview.chromium.org/245963006/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files (+52, -41 lines):
  M src/objects.h
  M src/runtime.cc
  M src/scopeinfo.cc
  M test/mjsunit/bugs/harmony/debug-blockscopes.js
  M test/mjsunit/debug-scopes.js
  M test/mjsunit/harmony/debug-blockscopes.js


Index: src/objects.h
diff --git a/src/objects.h b/src/objects.h
index a0b4982588bc9878745fae97083365995c5f2edf..5b1b77c2fa41b142793e396b76c97cf799541291 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
Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index 2d4ab93015e4f8a749fc8f4b61fbfe3976933869..57d204fda40f89f3d191f0ff643bbd2328ad002a 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -11199,33 +11199,44 @@ RUNTIME_FUNCTION(MaybeObject*, 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<FixedArray> 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(
         Context::cast(it.frame()->context())->declaration_context());
     for (; i < scope_info->LocalCount(); ++i) {
+      if (scope_info->LocalIsSynthetic(i))
+        continue;
       Handle<String> 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++;
     }
   }

@@ -11286,7 +11297,7 @@ RUNTIME_FUNCTION(MaybeObject*, 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<FixedArray> details = isolate->factory()->NewFixedArray(details_size);

@@ -11301,7 +11312,7 @@ RUNTIME_FUNCTION(MaybeObject*, 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) {
@@ -11352,7 +11363,7 @@ RUNTIME_FUNCTION(MaybeObject*, 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));
   }

@@ -11433,6 +11444,7 @@ static MaybeHandle<JSObject> MaterializeStackLocalsWithFrameInspector(

   // Second fill all stack locals.
   for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
+    if (scope_info->LocalIsSynthetic(i)) continue;
     Handle<String> name(scope_info->StackLocalName(i));
     Handle<Object> value(frame_inspector->GetExpression(i), isolate);
     if (value->IsTheHole()) continue;
@@ -11477,6 +11489,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<Object> value = Object::GetPropertyOrElement(
Index: src/scopeinfo.cc
diff --git a/src/scopeinfo.cc b/src/scopeinfo.cc
index 157cdfa4b6c54fd17cc503ce0c6dd80e467e8b59..564565ff32a3cdaf09d39ffabe47b9a157741fd7 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<String> 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<ScopeInfo> 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>(String::cast(scope_info->get(i))),
+            Handle<String>(String::cast(scope_info->get(i + start))),
             Handle<Object>(context->get(context_index), isolate),
             ::NONE,
             SLOPPY),
Index: test/mjsunit/bugs/harmony/debug-blockscopes.js
diff --git a/test/mjsunit/bugs/harmony/debug-blockscopes.js b/test/mjsunit/bugs/harmony/debug-blockscopes.js index fda32eb3d30acfc98a8839714450afa2834cf92c..9ef8efbc0ce513abea2cc292078a1c5fcc1a08dd 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:');
Index: test/mjsunit/debug-scopes.js
diff --git a/test/mjsunit/debug-scopes.js b/test/mjsunit/debug-scopes.js
index f5b5ec913ed6634dacface1cc90d176646d92513..ce37d2402305f5200023794475f1e74a770eb450 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--;
Index: test/mjsunit/harmony/debug-blockscopes.js
diff --git a/test/mjsunit/harmony/debug-blockscopes.js b/test/mjsunit/harmony/debug-blockscopes.js index ca2ab9e5a68d90d362e6eb37b02a1cfc7493bb3c..f56a306b6faace323ac8249132adbf1a6e386d4d 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();


--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
--- You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to