Fix potential overwriting of debug jumps of following code.

src/x64/deoptimizer-x64.cc

 // Copyright 2010 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 35 matching lines @@
   if (!function->IsOptimized()) return;
 
   // 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.
+  // 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++) {
     unsigned pc_offset = table.GetPcOffset(i);
     SafepointEntry safepoint_entry = table.GetEntry(i);
     int deoptimization_index = safepoint_entry.deoptimization_index();
     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++) {
     destroyer.masm()->int3();
   }
 #endif
 
   // Add the deoptimizing code to the list.
   DeoptimizingCodeListNode* node = new DeoptimizingCodeListNode(code);
   node->set_next(deoptimizing_code_list_);
@@ skipping 280 matching lines @@
   // Preserve deoptimizer object in register rax and get the input
   // frame descriptor pointer.
   __ movq(rbx, Operand(rax, Deoptimizer::input_offset()));
 
   // Fill in the input registers.
   for (int i = kNumberOfRegisters -1; i >= 0; i--) {
     int offset = (i * kPointerSize) + FrameDescription::registers_offset();
     __ 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;
     __ pop(Operand(rbx, dst_offset));
   }
 
   // Remove the bailout id from the stack.
   if (type() == EAGER) {
     __ addq(rsp, Immediate(kPointerSize));
   } else {
     __ 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);
 
   // Unwind the stack down to - but not including - the unwinding
   // limit and copy the contents of the activation frame to the input
   // frame description.
   __ lea(rdx, Operand(rbx, FrameDescription::frame_content_offset()));
   Label pop_loop;
   __ bind(&pop_loop);
@@ skipping 88 matching lines @@
   }
   __ bind(&done);
 }
 
 #undef __
 
 
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_X64