LiveEdit: patch positions in function

LiveEdit: patch positions in function

Committed: http://code.google.com/p/v8/source/detail?r=4139

[Peter Rybin, 2010-03-15 01:00:36]

Hi Soren

I'm working on updating all positions in functions after source has been
changed.
I need to update rinfo, which seems to be tricky. This approach with creating
new Code instance is the best I could find.

Peter

[Søren Thygesen Gjesse, 2010-03-15 08:41:16]

LGTM, with comments addressed

http://codereview.chromium.org/914003/diff/12001/13002
File src/factory.h (right):

http://codereview.chromium.org/914003/diff/12001/13002#newcode233
src/factory.h:233: static Handle<Code> CopyCode(Handle<Code> code,
patched_relocation_info -> reloc_info.

http://codereview.chromium.org/914003/diff/12001/13003
File src/heap.cc (right):

http://codereview.chromium.org/914003/diff/12001/13003#newcode2284
src/heap.cc:2284: // Copy header and instructions
End comment with period (times 3).

http://codereview.chromium.org/914003/diff/12001/13004
File src/heap.h (right):

http://codereview.chromium.org/914003/diff/12001/13004#newcode616
src/heap.h:616: // Copies code but takes alternative relocation_info.
alternative -> an alternative, relocation_info -> relocation info. 

How about something like "Copy the code and scope inof part of the code object,
but insert the provided data as the relocation information."?

Rename argument name patched_relocation_info -> reloc_info.

http://codereview.chromium.org/914003/diff/12001/13006
File src/liveedit.cc (right):

http://codereview.chromium.org/914003/diff/12001/13006#newcode347
src/liveedit.cc:347: 
Substitute -> Replace to match the naming of the other functions. Why not move
the full function definition here to save the separate declaration?

http://codereview.chromium.org/914003/diff/12001/13006#newcode411
src/liveedit.cc:411: // Auto-growing buffer for writing relocation info code
section.
Please extend the comment with why an alternative buffer (to the one in the
assembler) is needed here.

http://codereview.chromium.org/914003/diff/12001/13006#newcode425
src/liveedit.cc:425: void Write(const RelocInfo* rinfo) {
Please add a comment here and in GetResult on how the buffer is filled from the
bottom.

http://codereview.chromium.org/914003/diff/12001/13006#newcode441
src/liveedit.cc:441: if (buffer_size_ < 2*KB) {
Spaces around binary ops.

http://codereview.chromium.org/914003/diff/12001/13006#newcode455
src/liveedit.cc:455: // Clear the buffer in debug mode. Use 'int3' instructions
to make
in3 and opcode 0xCC is not very platform independent.

http://codereview.chromium.org/914003/diff/12001/13006#newcode483
src/liveedit.cc:483: // Patch positions in code (changes relocation_info
section) and possibly
relocation_info -> relocation info

http://codereview.chromium.org/914003/diff/12001/13006#newcode512
src/liveedit.cc:512: // Simply patch relocation area of code
. at the end of a comment.

http://codereview.chromium.org/914003/diff/12001/13006#newcode515
src/liveedit.cc:515: } else {
Please rephrase this comment.

Regarding the TODO, then shorting the code object in-place probably will not
work, as I don't think it is possible to have non-code fillers in the code
space, but I am not sure. Shorting objects in-place can be done by adjusting
their size and writing fillers (one pointer filler map, two pointer filler map
or a byte array of the right size) in the freed-up space after the shorter
object. You can try it out, but otherwise I suggest that you add some fillers to
the reloc info instead.

http://codereview.chromium.org/914003/diff/12001/13006#newcode580
src/liveedit.cc:580: 
Please add some comments to what the different iterators takes care of. Isn't it
possible that the same object will be visited twice?

http://codereview.chromium.org/914003/diff/12001/13006#newcode630
src/liveedit.cc:630: Handle<FixedArray>
breakPointInfos(debug_info->break_points());
breakPointInfos -> break_point_infos

http://codereview.chromium.org/914003/diff/12001/13006#newcode637
src/liveedit.cc:637: int new_position =
TranslatePosition(info->source_position()->value(),
Indentation.

http://codereview.chromium.org/914003/diff/12001/13006#newcode642
src/liveedit.cc:642: // Todo(635): Also patch breakpoint objects in JS.
Todo -> TODO

http://codereview.chromium.org/914003/diff/12001/13007
File src/runtime.cc (right):

http://codereview.chromium.org/914003/diff/12001/13007#newcode8869
src/runtime.cc:8869: }
Remove one of the empty lines.

http://codereview.chromium.org/914003/diff/12001/13009
File test/mjsunit/debug-liveedit-patch-positions-replace.js (right):

http://codereview.chromium.org/914003/diff/12001/13009#newcode31
test/mjsunit/debug-liveedit-patch-positions-replace.js:31: // Scenario: some
function is being changed, which causes enclosing function to
some function -> a function

http://codereview.chromium.org/914003/diff/12001/13009#newcode56
test/mjsunit/debug-liveedit-patch-positions-replace.js:56: // line long enough
to change rinfo encoding
Uppercase first letter + ending period in comments, and move comment one line up
before the "var".

http://codereview.chromium.org/914003/diff/12001/13009#newcode69
test/mjsunit/debug-liveedit-patch-positions-replace.js:69: function
CallM(changer) {
Please be more precise in this comment. It is a call IC you are expecting?

http://codereview.chromium.org/914003/diff/12001/13010
File test/mjsunit/debug-liveedit-patch-positions.js (right):

http://codereview.chromium.org/914003/diff/12001/13010#newcode92
test/mjsunit/debug-liveedit-patch-positions.js:92:
assertEquals(pcArrayBefore.toString(), pcArrayAfter.toString());
Can't you use assertArrayEquals here?

[Peter Rybin, 2010-03-15 20:56:10]

http://codereview.chromium.org/914003/diff/12001/13002
File src/factory.h (right):

http://codereview.chromium.org/914003/diff/12001/13002#newcode233
src/factory.h:233: static Handle<Code> CopyCode(Handle<Code> code,
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> patched_relocation_info -> reloc_info.

Done.

http://codereview.chromium.org/914003/diff/12001/13003
File src/heap.cc (right):

http://codereview.chromium.org/914003/diff/12001/13003#newcode2284
src/heap.cc:2284: // Copy header and instructions
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> End comment with period (times 3).

Done*3.

http://codereview.chromium.org/914003/diff/12001/13004
File src/heap.h (right):

http://codereview.chromium.org/914003/diff/12001/13004#newcode616
src/heap.h:616: // Copies code but takes alternative relocation_info.
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> alternative -> an alternative, relocation_info -> relocation info. 
> 
> How about something like "Copy the code and scope inof part of the code
object,
> but insert the provided data as the relocation information."?
> 
> Rename argument name patched_relocation_info -> reloc_info.

Cool.
Done.

http://codereview.chromium.org/914003/diff/12001/13006
File src/liveedit.cc (right):

http://codereview.chromium.org/914003/diff/12001/13006#newcode347
src/liveedit.cc:347: 
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Substitute -> Replace to match the naming of the other functions. Why not move
> the full function definition here to save the separate declaration?

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode411
src/liveedit.cc:411: // Auto-growing buffer for writing relocation info code
section.
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Please extend the comment with why an alternative buffer (to the one in the
> assembler) is needed here.

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode425
src/liveedit.cc:425: void Write(const RelocInfo* rinfo) {
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Please add a comment here and in GetResult on how the buffer is filled from
the
> bottom.

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode441
src/liveedit.cc:441: if (buffer_size_ < 2*KB) {
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Spaces around binary ops.

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode455
src/liveedit.cc:455: // Clear the buffer in debug mode. Use 'int3' instructions
to make
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> in3 and opcode 0xCC is not very platform independent.

I simply removed it.
Do we anything better, e.g. any API for platform-independent memory filler?

http://codereview.chromium.org/914003/diff/12001/13006#newcode483
src/liveedit.cc:483: // Patch positions in code (changes relocation_info
section) and possibly
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> relocation_info -> relocation info

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode512
src/liveedit.cc:512: // Simply patch relocation area of code
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> . at the end of a comment.

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode515
src/liveedit.cc:515: } else {
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Please rephrase this comment.
> 
> Regarding the TODO, then shorting the code object in-place probably will not
> work, as I don't think it is possible to have non-code fillers in the code
> space, but I am not sure. Shorting objects in-place can be done by adjusting
> their size and writing fillers (one pointer filler map, two pointer filler map
> or a byte array of the right size) in the freed-up space after the shorter
> object. You can try it out, but otherwise I suggest that you add some fillers
to
> the reloc info instead.

Done
Speaking about TODO I think I can safely drop it, because it would make the code
too complicated for now real benefit.

http://codereview.chromium.org/914003/diff/12001/13006#newcode580
src/liveedit.cc:580: 
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Please add some comments to what the different iterators takes care of. Isn't
it
> possible that the same object will be visited twice?

Done.

I'm not sure. However, I need to iterate over all pointers. I thought that there
are roots and there are pointers inside objects and that is roughly all pointers
in the system and these 2 sets do not overlap.
Do you think it is also worth making into a comment?

http://codereview.chromium.org/914003/diff/12001/13006#newcode630
src/liveedit.cc:630: Handle<FixedArray>
breakPointInfos(debug_info->break_points());
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> breakPointInfos -> break_point_infos

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode637
src/liveedit.cc:637: int new_position =
TranslatePosition(info->source_position()->value(),
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/914003/diff/12001/13006#newcode642
src/liveedit.cc:642: // Todo(635): Also patch breakpoint objects in JS.
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Todo -> TODO

Done.

http://codereview.chromium.org/914003/diff/12001/13007
File src/runtime.cc (right):

http://codereview.chromium.org/914003/diff/12001/13007#newcode8869
src/runtime.cc:8869: }
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Remove one of the empty lines.

Done.

http://codereview.chromium.org/914003/diff/12001/13009
File test/mjsunit/debug-liveedit-patch-positions-replace.js (right):

http://codereview.chromium.org/914003/diff/12001/13009#newcode31
test/mjsunit/debug-liveedit-patch-positions-replace.js:31: // Scenario: some
function is being changed, which causes enclosing function to
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> some function -> a function

Done.

http://codereview.chromium.org/914003/diff/12001/13009#newcode56
test/mjsunit/debug-liveedit-patch-positions-replace.js:56: // line long enough
to change rinfo encoding
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Uppercase first letter + ending period in comments, and move comment one line
up
> before the "var".

Done.

http://codereview.chromium.org/914003/diff/12001/13009#newcode69
test/mjsunit/debug-liveedit-patch-positions-replace.js:69: function
CallM(changer) {
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Please be more precise in this comment. It is a call IC you are expecting?

Done.

http://codereview.chromium.org/914003/diff/12001/13010
File test/mjsunit/debug-liveedit-patch-positions.js (right):

http://codereview.chromium.org/914003/diff/12001/13010#newcode92
test/mjsunit/debug-liveedit-patch-positions.js:92:
assertEquals(pcArrayBefore.toString(), pcArrayAfter.toString());
On 2010/03/15 08:41:16, Søren Gjesse wrote:
> Can't you use assertArrayEquals here?

Thanks.
Done.

[Søren Thygesen Gjesse, 2010-03-16 07:42:02]

http://codereview.chromium.org/914003/diff/12001/13006
File src/liveedit.cc (right):

http://codereview.chromium.org/914003/diff/12001/13006#newcode455
src/liveedit.cc:455: // Clear the buffer in debug mode. Use 'int3' instructions
to make
On 2010/03/15 20:56:10, Peter Rybin wrote:
> On 2010/03/15 08:41:16, Søren Gjesse wrote:
> > in3 and opcode 0xCC is not very platform independent.
> 
> I simply removed it.
> Do we anything better, e.g. any API for platform-independent memory filler?

No, there is not. The ia32 and x64 assemblers fills 0xCC in code buffers they
allocate themselves in debug mode. The ARM assembler does no such thing.

http://codereview.chromium.org/914003/diff/12001/13006#newcode515
src/liveedit.cc:515: } else {
On 2010/03/15 20:56:10, Peter Rybin wrote:
> On 2010/03/15 08:41:16, Søren Gjesse wrote:
> > Please rephrase this comment.
> > 
> > Regarding the TODO, then shorting the code object in-place probably will not
> > work, as I don't think it is possible to have non-code fillers in the code
> > space, but I am not sure. Shorting objects in-place can be done by adjusting
> > their size and writing fillers (one pointer filler map, two pointer filler
map
> > or a byte array of the right size) in the freed-up space after the shorter
> > object. You can try it out, but otherwise I suggest that you add some
fillers
> to
> > the reloc info instead.
> 
> Done
> Speaking about TODO I think I can safely drop it, because it would make the
code
> too complicated for now real benefit.

Agree.

http://codereview.chromium.org/914003/diff/12001/13006#newcode580
src/liveedit.cc:580: 
On 2010/03/15 20:56:10, Peter Rybin wrote:
> On 2010/03/15 08:41:16, Søren Gjesse wrote:
> > Please add some comments to what the different iterators takes care of.
Isn't
> it
> > possible that the same object will be visited twice?
> 
> Done.
> 
> I'm not sure. However, I need to iterate over all pointers. I thought that
there
> are roots and there are pointers inside objects and that is roughly all
pointers
> in the system and these 2 sets do not overlap.
> Do you think it is also worth making into a comment?

As all objects are in the heap the heap iterator will visit all of them. The
strong roots are the set of objects which defines the initial set of live
objects when performing a GC. They include all "basic" objects, objects
reachable from the execution stack, objects reachable from the various C++
structures used internally, etc. In this case I think you could just use the
HeapIterator. We have been talking about making a HeapIterator which only visits
live objects using the same marking scheme as used by the GC, but have never
taken the time. The HeapIterator just visits everything.