Issue 2081: Expose function's (closure's) inner context in debugger.

Issue 2081: Expose function's (closure's) inner context in debugger.

This is against the correct branch (bleeding_edge).

Committed: https://code.google.com/p/v8/source/detail?r=11458

[Peter Rybin, 2012-04-20 16:38:04]

This is a replica of http://codereview.chromium.org/10073032/ that was targeted
to incorrect branch.

[Peter Rybin, 2012-04-23 16:22:05]

ping

[ulan, 2012-04-24 08:57:49]

Sorry for the delay. Does this patch depend on other uncommitted patches?

I am getting an error when running tests with this patch on top of r11422:


>>> running tests for ia32.release
=== release mirror-unresolved-function ===                                     
Path: mjsunit/mirror-unresolved-function
/usr/local/google/home/ulan/v8/test/mjsunit/mirror-unresolved-function.js:45:
illegal access
var json = JSON.stringify(serializer.serializeValue(mirror));
                                     ^
Command: /usr/local/google/home/ulan/v8/out/ia32.release/d8 --nobreak-on-abort
--expose-debug-as debug /usr/local/google/home/ulan/v8/test/mjsunit/mjsunit.js
/usr/local/google/home/ulan/v8/test/mjsunit/mirror-unresolved-function.js --test

[Peter Rybin, 2012-04-24 23:05:08]

Yes, unfortunately I missed check for unresolved function.

I added guards to debug-debugger.js and rebased all to the recent revision. The
tests should be ok now.

[ulan, 2012-04-25 14:14:55]

Could you please add more tests that exercise different combinations of stack
allocated and context allocated variables? And also tests with "let" variables
(this requires --harmony flag and "use strict").

For example, the following script outputs
0: {"visible2":20}
1: {"visible1":10}

because all other variables are stack allocated. Would Dev-tools work correctly
in such cases?

===========
var Debug = debug.Debug;

var f = (function F1(invisible_parameter) {
  var invisible1 = 1;
  var visible1 = 10;
  return (function F2() {
    var invisible2 = 2;
    return (function F3() {
      var visible2 = 20;
      var invisible2 = 3;
      return (function () {return visible1 + visible2;});
    })();
  })();
})(5);

var mirror = Debug.MakeMirror(f);
var count = mirror.scopeCount() - 1;
for (var i = 0; i < count; i++) {
  print(i + ": " + JSON.stringify(mirror.scope(i).scopeObject().value()))
}
===========

https://chromiumcodereview.appspot.com/10171003/diff/16001/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/10171003/diff/16001/src/runtime.cc#new...
src/runtime.cc:11420: for (ScopeIterator it(isolate, fun);
This fits in one line.

[Peter Rybin, 2012-04-25 14:34:17]

> Could you please add more tests that exercise different combinations of stack
> allocated and context allocated variables?

> For example, the following script outputs
> 0: {"visible2":20}
> 1: {"visible1":10}
> 
> because all other variables are stack allocated. Would Dev-tools work
correctly
> in such cases?

I think it's quite ok in this case. I expect that function scopes will be
consistent with scopes that are visible on stack when you debug inside this
function. For example "invisible1" won't be visible in both cases.

My main intent here is that there is very well defined static data, that is
visible to programmer only in certain circumstances (when function is on stack).
This same data display should be available to user in debugger at any moment.

[Peter Rybin, 2012-04-25 22:56:49]

https://chromiumcodereview.appspot.com/10171003/diff/16001/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/10171003/diff/16001/src/runtime.cc#new...
src/runtime.cc:11420: for (ScopeIterator it(isolate, fun);
On 2012/04/25 14:14:55, ulan wrote:
> This fits in one line.

Done.

[Peter Rybin, 2012-04-25 22:59:27]

> Could you please add more tests that exercise different combinations of stack
> allocated and context allocated variables? And also tests with "let" variables
> (this requires --harmony flag and "use strict").

I added several more test cases. I don't know how to properly prepare test with
"--harmony" flag enabled, but I tried it manually.

I think we shouldn't really try all possible scope combinations, because there
is no new code submitted about it. It only reuses the standard code for scopes.

[Yang, 2012-04-26 06:12:11]

On 2012/04/25 22:59:27, Peter Rybin wrote:
> > Could you please add more tests that exercise different combinations of
stack
> > allocated and context allocated variables? And also tests with "let"
variables
> > (this requires --harmony flag and "use strict").
> 
> I added several more test cases. I don't know how to properly prepare test
with
> "--harmony" flag enabled, but I tried it manually.
> 
> I think we shouldn't really try all possible scope combinations, because there
> is no new code submitted about it. It only reuses the standard code for
scopes.

You can add flags at the begin of a .js test file following "// Flags: ", the
way it's already being done in debug-function-scopes.js. While you are at this,
you can also remove the --allow-natives-syntax flag since it's not necessary.

[Peter Rybin, 2012-04-26 14:13:24]

> You can add flags at the begin of a .js test file following "// Flags: ", the
> way it's already being done in debug-function-scopes.js. While you are at
this,

I put harmony-specific test into harmony directory.

> you can also remove the --allow-natives-syntax flag since it's not necessary.
Done

[ulan, 2012-04-26 14:31:15]

This looks good to me if you remove the trailing space.

Please wait for Yang approval of changes in src/mirror-debugger.js and
src/debug-debugger.js before landing.

https://chromiumcodereview.appspot.com/10171003/diff/26001/test/mjsunit/harmo...
File test/mjsunit/harmony/debug-function-scopes.js (right):

https://chromiumcodereview.appspot.com/10171003/diff/26001/test/mjsunit/harmo...
test/mjsunit/harmony/debug-function-scopes.js:82: var f2 = (function() {
Trailing space.

[Yang, 2012-04-26 14:38:29]

On 2012/04/26 14:31:15, ulan wrote:
> This looks good to me if you remove the trailing space.
> 
> Please wait for Yang approval of changes in src/mirror-debugger.js and
> src/debug-debugger.js before landing.
> 
>
https://chromiumcodereview.appspot.com/10171003/diff/26001/test/mjsunit/harmo...
> File test/mjsunit/harmony/debug-function-scopes.js (right):
> 
>
https://chromiumcodereview.appspot.com/10171003/diff/26001/test/mjsunit/harmo...
> test/mjsunit/harmony/debug-function-scopes.js:82: var f2 = (function() {
> Trailing space.

LGTM as well.