This fixes the issue with the deoptimizer trashing the reloc info patching t...

This fixes the issue with the deoptimizer trashing the reloc info before patching the code. 

If we, immediately after the deoptimization, but before actually
running the patched code, get a compacting GC, the addresses from the
calls might no longer be valid.

I have validated that this works by patching the existing code to
always do a compacting gc after we finish deoptimizing. I will create
a real regression test for this, but this includes additional code for
allowing us to force a deopt/opt from javascript test code. I will
land this in a seperate change.



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

[Kevin Millikin (Chromium), 2011-02-02 08:33:31]

The approach LGTM, but it needs a couple of comments about what hte approach is.
 I also want to use higher level abstractions (e.g., HeapObject::Size and
HeapObject::set_map).

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:57: Address original_reloc_payload =
code->relocation_start();
This needs a comment to the effect of "We will overwrite the code's relocation
info in-place.  Relocation info is written backward.  The relocation info is the
payload of a byte array."

A simpler way to deal with the padding at the end, which I like better than
RoundUp is:

ByteArray* reloc_info = code->relocation_info();
Address end_address = reloc_info->address() + reloc_info->Size();
RelocInfoWriter writer(end_address, code->instruction_start());

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:80: Address entry_address =
Does this fit on one line?  What if you just call it "entry"?

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:99: int reloc_size = reloc_end -
reloc_info_writer.pos();
This needs a comment to the effect that we will now move the new relocation info
to the beginning of the original byte array and patch its size.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:103:
code->relocation_info()->set_length(reloc_size);
Simply:

reloc_info->set_length(reloc_size);

if you name the reloc info as suggested above.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:105: Address new_reloc_end =
This needs a comment to the effect that we'll put a non-live object in the extra
space at the end of the former reloc info.

More simply, without RoundUp:

Address junk_address = reloc_info->address() + reloc_info->Size();

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:107: CHECK(new_reloc_end <= reloc_end);
I'm a little uncomfortable with having this a CHECK instead of an ASSERT.  I
guess if it's false then the memmove has stomped over some other object.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:115: while(reloc_end > new_reloc_end) {
This is a little simpler if you decrement first.  I find it clearer if it counts
up, though.  I also like to leave explicit use of Memory:: functions for the GC
and use higher level abstractions here:

HeapObject* filler_map = Heap::one_pointer_filler_map();
while (junk_address < end_address) {
  HeapObject::FromAddress(junk_address)->set_map(filler_map);
  junk_address += kPointerSize;
}

It's annoying that the code implicitly depends on kPointerSize and the size of a
one word filler object being the same, but I guess that will never change.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:121: Address junk_data_start = new_reloc_end +
ByteArray::kHeaderSize;
I think this whole thing is

int size = end_address - junk_address;
HeapObject* junk_object = HeapObject::FromAddress(junk_address);
junk_object->set_map(Heap::byte_array_map());
ByteArray::cast(junk_object)->set_length(ByteArray::LengthFor(end - junk));

[Rico, 2011-02-02 09:19:08]

Comments addressed, please have another look

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:57: Address original_reloc_payload =
code->relocation_start();
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> This needs a comment to the effect of "We will overwrite the code's relocation
> info in-place.  Relocation info is written backward.  The relocation info is
the
> payload of a byte array."
> 
> A simpler way to deal with the padding at the end, which I like better than
> RoundUp is:
> 
> ByteArray* reloc_info = code->relocation_info();
> Address end_address = reloc_info->address() + reloc_info->Size();
> RelocInfoWriter writer(end_address, code->instruction_start());

Done.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:80: Address entry_address =
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> Does this fit on one line?  What if you just call it "entry"?

Done.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:99: int reloc_size = reloc_end -
reloc_info_writer.pos();
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> This needs a comment to the effect that we will now move the new relocation
info
> to the beginning of the original byte array and patch its size.

Done.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:103:
code->relocation_info()->set_length(reloc_size);
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> Simply:
> 
> reloc_info->set_length(reloc_size);
> 
> if you name the reloc info as suggested above.

Done.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:105: Address new_reloc_end =
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> This needs a comment to the effect that we'll put a non-live object in the
extra
> space at the end of the former reloc info.
> 
> More simply, without RoundUp:
> 
> Address junk_address = reloc_info->address() + reloc_info->Size();

Done.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:107: CHECK(new_reloc_end <= reloc_end);
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> I'm a little uncomfortable with having this a CHECK instead of an ASSERT.  I
> guess if it's false then the memmove has stomped over some other object.

Done.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:115: while(reloc_end > new_reloc_end) {
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> This is a little simpler if you decrement first.  I find it clearer if it
counts
> up, though.  I also like to leave explicit use of Memory:: functions for the
GC
> and use higher level abstractions here:
> 
> HeapObject* filler_map = Heap::one_pointer_filler_map();
> while (junk_address < end_address) {
>   HeapObject::FromAddress(junk_address)->set_map(filler_map);
>   junk_address += kPointerSize;
> }
> 
> It's annoying that the code implicitly depends on kPointerSize and the size of
a
> one word filler object being the same, but I guess that will never change.
Well, it explicitly say one_POINTER_filler, not one_word_filler, so I guess
there could never be any confusion.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:121: Address junk_data_start = new_reloc_end +
ByteArray::kHeaderSize;
On 2011/02/02 08:33:31, Kevin Millikin wrote:
> I think this whole thing is
> 
> int size = end_address - junk_address;
> HeapObject* junk_object = HeapObject::FromAddress(junk_address);
> junk_object->set_map(Heap::byte_array_map());
> ByteArray::cast(junk_object)->set_length(ByteArray::LengthFor(end - junk));

Done.

[Vitaly Repeshko, 2011-02-02 14:14:53]

One late question.

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#ne...
src/ia32/deoptimizer-ia32.cc:109: // Handle the junk part after the new
relocation info.
Can we use Heap::CreateFillerObjectAt() here?