Issue 149542: Support stepping into getters and setters

Issue 149542: Support stepping into getters and setters (Closed)

Created:
11 years, 5 months ago by yurys

Modified:
9 years, 7 months ago

Reviewers:
Søren Thygesen Gjesse, Mads Ager (chromium)

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Support stepping into getters and setters. Related Chromium bug: http://code.google.com/p/chromium/issues/detail?id=16427 Committed: http://code.google.com/p/v8/source/detail?r=2458

Patch Set 10 : '' #

Created: 11 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+289 lines, -6 lines)			Patch
M	src/debug.cc	View	1 2 3 4 5 6 7 8 9	4 chunks	+28 lines, -6 lines	0 comments	Download
M	src/objects.cc	View	1 2 3 4 5 6 7 8 9	2 chunks	+12 lines, -0 lines	0 comments	Download
A	test/mjsunit/debug-stepin-accessor.js	View	1 2 5 6 7 8	1 chunk	+248 lines, -0 lines	0 comments	Download
M	test/mjsunit/mjsunit.status	View	3 4 5 6 7 8 9	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Mads Ager (chromium)

http://codereview.chromium.org/149542/diff/7/1008 File src/objects.cc (right): http://codereview.chromium.org/149542/diff/7/1008#newcode212 Line 212: return Object::GetPropertyWithDefinedGetter(receiver, Since the debugger can cause GCs, ...

11 years, 5 months ago (2009-07-13 20:12:19 UTC) #2

yurys

11 years, 5 months ago (2009-07-14 09:55:17 UTC) #3

yurys

On 2009/07/14 09:55:17, Yury Semikhatsky wrote: > http://codereview.chromium.org/149542/diff/7/1008 > File src/objects.cc (right): > > http://codereview.chromium.org/149542/diff/7/1008#newcode212 ...

11 years, 5 months ago (2009-07-14 16:20:26 UTC) #4

On 2009/07/14 09:55:17, Yury Semikhatsky wrote:
> http://codereview.chromium.org/149542/diff/7/1008
> File src/objects.cc (right):
> 
> http://codereview.chromium.org/149542/diff/7/1008#newcode212
> Line 212: return Object::GetPropertyWithDefinedGetter(receiver,
> On 2009/07/13 20:12:19, Mads Ager wrote:
> > Since the debugger can cause GCs, you have to wrap 'receiver' in a handle as
> > well and have a call to GetPropertyWithDefinedGetter inside the if (using
the
> > handles).
> > 
> Moved the code into SetPropertyWithDefinedSetter where the receiver and the
> setter are already protected. This also covers some more cases of
getter/setterr
> definition, e.g. accessors for indexed properties (tests for those cases are
> also added).
> 
> > Would it be possible to add a test that forces a GC in the HandleStepIn
code?
> I wonder if there is a way to enforce GC, say, on each allocation or something
> like this so that I could run the test with a flag enabling such behavior and
> check that all objects that need to be wrapped in handles are wrapped in them?
> 
> http://codereview.chromium.org/149542/diff/7/1008#newcode1627
> Line 1627: return SetPropertyWithDefinedSetter(JSFunction::cast(setter),
value);
> On 2009/07/13 20:12:19, Mads Ager wrote:
> > Same as above.  You have to wrap value in a handle as well for the debugger
> case
> > and have a call to SetPropertyWIthDefinedSetter inside of the if (using the
> > handles).
> 
> This code as well as its counterpart for getters has been moved into
> SetPropertyWithDefinedSetter where the receiver is already protected by a
> handle.

The problem with calling SetDebugBreakAtIC from PrepareStepIn was that the rinfo
and original_rinfo for IC loads/stores were already patched by call to
FloodWithOneShot in PrepareStep so there is no need to call it once again from
PrepareStepIn. I've reverted the change to SetDebugBreakAtIC and do not call it
from PrepareStepIn anymore.

Mads Ager (chromium)

LGTM! http://codereview.chromium.org/149542/diff/1045/34 File src/debug.cc (right): http://codereview.chromium.org/149542/diff/1045/34#newcode337 Line 337: // Step in through constructs call requires ...

11 years, 5 months ago (2009-07-14 16:36:47 UTC) #5

yurys

11 years, 5 months ago (2009-07-14 16:41:06 UTC) #6

http://codereview.chromium.org/149542/diff/1045/34
File src/debug.cc (right):

http://codereview.chromium.org/149542/diff/1045/34#newcode337
Line 337: // Step in through constructs call requires no changes to the running
code.
On 2009/07/14 16:36:47, Mads Ager wrote:
> Not your change, but: constructs -> construct

Done.

http://codereview.chromium.org/149542/diff/1045/34#newcode339
Line 339: // because caller of this function(Debug::PrepareStep) is expected to
On 2009/07/14 16:36:47, Mads Ager wrote:
> Add space before '('.

Done.

http://codereview.chromium.org/149542/diff/1045/34#newcode340
Line 340: // flood top frame's function with one shot breakpoints.
On 2009/07/14 16:36:47, Mads Ager wrote:
> top frame's -> the top frame's

Done.

http://codereview.chromium.org/149542/diff/1045/33
File src/objects.cc (right):

http://codereview.chromium.org/149542/diff/1045/33#newcode222
Line 222: // Protect the receiver and getter in handles as the debugger can
allocate
On 2009/07/14 16:36:47, Mads Ager wrote:
> Comment no longer needed since everything is in handles here already.

Done.

http://codereview.chromium.org/149542/diff/1045/33#newcode1638
Line 1638: // Protect the receiver and setter in handles as the debugger can
allocate
On 2009/07/14 16:36:47, Mads Ager wrote:
> Comment no longer needed.

Done.

Expand Messages | Collapse Messages