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

src/ia32/deoptimizer-ia32.cc

Index: src/ia32/deoptimizer-ia32.cc
===================================================================
--- src/ia32/deoptimizer-ia32.cc	(revision 6550)
+++ src/ia32/deoptimizer-ia32.cc	(working copy)
@@ -33,6 +33,7 @@
 #include "deoptimizer.h"
 #include "full-codegen.h"
 #include "safepoint-table.h"
+#include "utils.h"
 
 namespace v8 {
 namespace internal {
@@ -48,14 +49,16 @@
   // Get the optimized code.
   Code* code = function->code();
 
-  // Invalidate the relocation information, as it will become invalid by the
-  // code patching below, and is not needed any more.
-  code->InvalidateRelocation();
-
   // For each return after a safepoint insert a absolute call to the
   // corresponding deoptimization entry.
   unsigned last_pc_offset = 0;
   SafepointTable table(function->code());
+
+  Address original_reloc_payload = code->relocation_start();

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

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());

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

Done.

+  Address reloc_end =
+      RoundUp(original_reloc_payload + code->relocation_size(), kPointerSize);;
+  RelocInfoWriter reloc_info_writer(reloc_end, code->instruction_start());
+
   for (unsigned i = 0; i < table.length(); i++) {
     unsigned pc_offset = table.GetPcOffset(i);
     SafepointEntry safepoint_entry = table.GetEntry(i);
@@ -72,12 +75,15 @@
 #endif
     last_pc_offset = pc_offset;
     if (deoptimization_index != Safepoint::kNoDeoptimizationIndex) {
-      CodePatcher patcher(
-          code->instruction_start() + pc_offset + gap_code_size,
-          Assembler::kCallInstructionLength);
-      patcher.masm()->call(GetDeoptimizationEntry(deoptimization_index, LAZY),
-                           RelocInfo::NONE);
+      Address call_pc = code->instruction_start() + pc_offset + gap_code_size;
+      CodePatcher patcher(call_pc, Assembler::kCallInstructionLength);
+      Address entry_address =

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

Does this fit on one line?  What if you just call it "entry"?

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

Done.

+          GetDeoptimizationEntry(deoptimization_index, LAZY);
+      patcher.masm()->call(entry_address, RelocInfo::NONE);
       last_pc_offset += gap_code_size + Assembler::kCallInstructionLength;
+      RelocInfo rinfo(call_pc + 1, RelocInfo::RUNTIME_ENTRY,
+                      reinterpret_cast<intptr_t>(entry_address));
+      reloc_info_writer.Write(&rinfo);
     }
   }
 #ifdef DEBUG
@@ -90,6 +96,40 @@
   }
 #endif
 
+  int reloc_size = reloc_end - reloc_info_writer.pos();

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

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.

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

Done.

+  memmove(original_reloc_payload, reloc_info_writer.pos(), reloc_size);
+
+  // The relocation info is in place, update the size.
+  code->relocation_info()->set_length(reloc_size);

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

Simply:

reloc_info->set_length(reloc_size);

if you name the reloc info as suggested above.

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

Done.

+
+  Address new_reloc_end =

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

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();

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

Done.

+      RoundUp(code->relocation_start() + reloc_size, kPointerSize);
+  CHECK(new_reloc_end <= reloc_end);

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

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.

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

Done.

+
+  // Handle the junk part after the new relocation info.

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

Can we use Heap::CreateFillerObjectAt() here?

+  if (reloc_end - new_reloc_end <= ByteArray::kHeaderSize) {
+    // We get in here if there is not enough space for a ByteArray.
+
+    // Both addresses are kPointerSize alligned.
+    CHECK((reloc_end -new_reloc_end) % 4 == 0);
+    while(reloc_end > new_reloc_end) {

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

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.

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

Well, it explicitly say one_POINTER_filler, not one_word_filler, so I guess
there could never be any confusion. 

+      Address filler = reloc_end - kPointerSize;
+      Memory::Object_at(filler) = Heap::one_pointer_filler_map();
+      reloc_end -= kPointerSize;
+    }
+  } else {
+    Address junk_data_start = new_reloc_end + ByteArray::kHeaderSize;

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

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]

Done.

+    int junk_size = reloc_end - junk_data_start;
+
+    // Since the reloc_end address and junk_data_start are both alligned,
+    // we shouild never have junk which is not a multipla of kPointerSize.
+    CHECK(junk_size % kPointerSize == 0);
+    CHECK(junk_size > 0);
+    ByteArray* junk_array = ByteArray::FromDataStartAddress(junk_data_start);
+    junk_array->set_map(Heap::byte_array_map());
+    junk_array->set_length(junk_size);
+  }
+
   // Add the deoptimizing code to the list.
   DeoptimizingCodeListNode* node = new DeoptimizingCodeListNode(code);
   node->set_next(deoptimizing_code_list_);