Fix potential overwriting of debug jumps of following code.

src/x64/deoptimizer-x64.cc

Index: src/x64/deoptimizer-x64.cc
diff --git a/src/x64/deoptimizer-x64.cc b/src/x64/deoptimizer-x64.cc
index 1f7c1ef60627a7cf064a671ba006340cd05d48eb..e058c2e49c19aa7d252e5c7821f137a45ed2250d 100644
--- a/src/x64/deoptimizer-x64.cc
+++ b/src/x64/deoptimizer-x64.cc
@@ -53,7 +53,9 @@ void Deoptimizer::DeoptimizeFunction(JSFunction* function) {
   code->InvalidateRelocation();
 
   // For each return after a safepoint insert a absolute call to the
-  // corresponding deoptimization entry.
+  // corresponding deoptimization entry, or a short call to an absolute
+  // jump if space is short.
+  unsigned jump_table = function->code()->safepoint_table_start();
   unsigned last_pc_offset = 0;
   SafepointTable table(function->code());
   for (unsigned i = 0; i < table.length(); i++) {
@@ -63,27 +65,60 @@ void Deoptimizer::DeoptimizeFunction(JSFunction* function) {
     int gap_code_size = safepoint_entry.gap_code_size();
 #ifdef DEBUG
     // Destroy the code which is not supposed to run again.
+    CHECK(pc_offset >= last_pc_offset);
     unsigned instructions = pc_offset - last_pc_offset;
     CodePatcher destroyer(code->instruction_start() + last_pc_offset,
                           instructions);
-    for (unsigned i = 0; i < instructions; i++) {
+    while (instructions > 0) {

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

This whole code hunk in #ifdef DEBUG is repeated below.  It needs to be moved
into a separate function.

Isn't it an idiom to write "while (instructions-- > 0) ..."?  You could do the
same below, as well (if you don't factor into a separate function).

[Lasse Reichstein, 2011/02/03 14:14:12]

Moved int3-writing to function called ZapInstructions.

Decrementing in the condition probably is an idiom (or perhaps it's "while
(--instructions >= 0)" for the case where you use the index in the loop - and
for the microoptimizers who prefer prefix decrement to postfix). I usually try
to avoid conditions with side effects, idiomatic or not, but I guess it's easy
enough to read here.

[Kevin Millikin (Chromium), 2011/02/04 10:27:29]

You can (but I'm not suggesting you do) write it as:

while (instructions --> 0) { ... }

and pronounce the "-->" operator as "goes to".  :)

       destroyer.masm()->int3();
+      instructions--;
     }
 #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);
-      last_pc_offset += gap_code_size + Assembler::kCallInstructionLength;
+      int call_length = MacroAssembler::kCallInstructionLength;

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

I find all the code below confusing.

You only really care if call_length is >= kCallInstructionLength or not.  Please
use a boolean:

bool fits = true;

[Lasse Reichstein, 2011/02/03 14:14:12]

The code is generally confusing. I have refactored it completely, and moved the
iteration over deoptimization entries to a separate class.
This allows the loop to concentrate on just checking for room and writing one of
two code pieces.

+      // Check if the next entry (if any) is located so close that
+      // we can't write a long Call sequence.
+      int end_of_long_call =
+          pc_offset + gap_code_size + MacroAssembler::kCallInstructionLength;

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

All of the code in this if (deoptimization_index !=
Safepoint::kNoDeoptimizationIndex) is more understandable if you skip the gap as
soon as you know you will, rather than at the very end.

if (...) {
  last_pc_offset += gap_code_size;
  bool fits = true;
  int end_of_long_call = last_pc_offset +
MacroAssembler::kCallInstructionLength;

I also think something like target_offset is a better name for end_of_long_call.

+      for (unsigned j = i + 1; j < table.length(); j++) {
+        // Stop if the next entry ends later than the call would.
+        int next_pc = table.GetPcOffset(j);

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

Call this next_pc_offset of something so it's clear it's an offset and not an
address.

[Lasse Reichstein, 2011/02/03 14:14:12]

Done.

+        if (next_pc >= end_of_long_call) break;
+        // Only care if the entry is a deoptimization point.
+        if (table.GetEntry(j).deoptimization_index() !=
+            Safepoint::kNoDeoptimizationIndex) {
+          call_length = next_pc - (pc_offset + gap_code_size);

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

fits = false;
break;

+          break;
+        }
+      }
+      if (call_length >= MacroAssembler::kCallInstructionLength) {

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

if (fits) ...

+        CodePatcher patcher(

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

CodePatcher patcher(code->instruction_start() + last_pc_offset,
                    Assembler::kCallInstructionLength);

+            code->instruction_start() + pc_offset + gap_code_size,
+            Assembler::kCallInstructionLength);
+        patcher.masm()->Call(GetDeoptimizationEntry(deoptimization_index, LAZY),
+                             RelocInfo::NONE);
+        last_pc_offset += gap_code_size + Assembler::kCallInstructionLength;

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

last_pc_offset += Assembler::kCallInstructionLength;

+      } else {
+        jump_table -= MacroAssembler::kJumpInstructionLength;
+        CodePatcher jump_patcher(code->instruction_start() + jump_table,

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

It probably bears naming

Address jump_address = code->instruction_start() + jump_table;

Or the whole thing would be simpler if you used the address of the next jump
throughout rather than the offset.

+                                 MacroAssembler::kJumpInstructionLength);
+        jump_patcher.masm()->Jump(
+            GetDeoptimizationEntry(deoptimization_index, LAZY),
+            RelocInfo::NONE);
+
+        CodePatcher call_patcher(

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

CodePatcher call_patcher(code->instruction_start + last_pc_offset,
                         Assembler::kShortCallLength);

The code patcher requires the length to be exact, so you can't really pass the
variable call_length unless you ensure that it is always equal to the length of
this particular call (which is why I say you only actually care about whether
call_length >= kCallInstructionLength).

[Lasse Reichstein, 2011/02/03 14:14:12]

Fixed.

+            code->instruction_start() + pc_offset + gap_code_size,
+            call_length);
+        call_patcher.masm()->call(code->instruction_start() + jump_table);
+        last_pc_offset += gap_code_size + call_patcher.masm()->pc_offset();

[Kevin Millikin (Chromium), 2011/02/02 13:05:33]

last_pc_offset += Assembler::kShortCallLength;

+      }
     }
   }
 #ifdef DEBUG
   // Destroy the code which is not supposed to run again.
-  CHECK(code->safepoint_table_start() >= last_pc_offset);
-  unsigned instructions = code->safepoint_table_start() - last_pc_offset;
+  CHECK(jump_table >= last_pc_offset);
+  unsigned instructions = jump_table - last_pc_offset;
   CodePatcher destroyer(code->instruction_start() + last_pc_offset,
                         instructions);
   for (unsigned i = 0; i < instructions; i++) {
@@ -384,7 +419,7 @@ void Deoptimizer::EntryGenerator::Generate() {
     __ pop(Operand(rbx, offset));
   }
 
-    // Fill in the double input registers.
+  // Fill in the double input registers.
   int double_regs_offset = FrameDescription::double_registers_offset();
   for (int i = 0; i < XMMRegister::kNumAllocatableRegisters; i++) {
     int dst_offset = i * kDoubleSize + double_regs_offset;
@@ -398,7 +433,7 @@ void Deoptimizer::EntryGenerator::Generate() {
     __ addq(rsp, Immediate(2 * kPointerSize));
   }
 
-  // Compute a pointer to the unwinding limit in register ecx; that is
+  // Compute a pointer to the unwinding limit in register rcx; that is
   // the first stack slot not part of the input frame.
   __ movq(rcx, Operand(rbx, FrameDescription::frame_size_offset()));
   __ addq(rcx, rsp);