Fix arguments object materialization during deopt.

src/deoptimizer.cc

Index: src/deoptimizer.cc
diff --git a/src/deoptimizer.cc b/src/deoptimizer.cc
index d1b00f8bdea718a71eca5d12d47900cd2f677883..255efca7148eb0d9a97b8df3d2e5fa954e4f1cf5 100644
--- a/src/deoptimizer.cc
+++ b/src/deoptimizer.cc
@@ -27,6 +27,7 @@
 
 #include "v8.h"
 
+#include "accessors.h"
 #include "codegen.h"
 #include "deoptimizer.h"
 #include "disasm.h"
@@ -368,6 +369,8 @@ Deoptimizer::Deoptimizer(Isolate* isolate,
       output_count_(0),
       jsframe_count_(0),
       output_(NULL),
+      deferred_arguments_objects_values_(0),
+      deferred_arguments_objects_(0),
       deferred_heap_numbers_(0) {
   if (FLAG_trace_deopt && type != OSR) {
     if (type == DEBUGGER) {
@@ -633,8 +636,47 @@ void Deoptimizer::DoComputeOutputFrames() {
 }
 
 
-void Deoptimizer::MaterializeHeapNumbers() {
+static void MaterializeArgumentsObjectUsingOutputFrame(
+    Isolate* isolate, JavaScriptFrame* frame) {
+  Handle<JSFunction> function(JSFunction::cast(frame->function()), isolate);
+  Handle<Object> arguments;
+  for (int i = frame->ComputeExpressionsCount() - 1; i >= 0; --i) {
+    if (frame->GetExpression(i) == isolate->heap()->arguments_marker()) {
+      if (arguments.is_null()) {
+        // FunctionGetArguments can't throw an exception, so cast away the
+        // doubt with an assert.
+        arguments = Handle<Object>(
+            Accessors::FunctionGetArguments(*function,
+                                            NULL)->ToObjectUnchecked());
+        ASSERT(*arguments != isolate->heap()->null_value());
+        ASSERT(*arguments != isolate->heap()->undefined_value());
+      }
+      frame->SetExpression(i, *arguments);
+      if (FLAG_trace_deopt) {
+        PrintF("Materializing arguments object for frame %p - %p: ",
+               reinterpret_cast<void*>(frame->sp()),
+               reinterpret_cast<void*>(frame->fp()));
+        arguments->ShortPrint();
+        PrintF("\n");
+      }
+    }
+  }
+}
+
+
+void Deoptimizer::MaterializeHeapObjects(JavaScriptFrameIterator* it) {
   ASSERT_NE(DEBUGGER, bailout_type_);
+
+  // Handlify all argument object values before triggering any allocation.
+  List<Handle<Object> > values(deferred_arguments_objects_values_.length());
+  for (int i = 0; i < deferred_arguments_objects_values_.length(); ++i) {
+    values.Add(Handle<Object>(deferred_arguments_objects_values_[i]));
+  }
+
+  // Play it safe and clear all unhandlified values before we continue.
+  deferred_arguments_objects_values_.Clear();
+
+  // Materialize all heap numbers before looking at arguments.

[Sven Panne, 2012/09/12 07:14:22]

Describe why this is necessary to do before arguments objects materialization.

[Michael Starzinger, 2012/09/12 10:07:28]

Done.

   for (int i = 0; i < deferred_heap_numbers_.length(); i++) {
     HeapNumberMaterializationDescriptor d = deferred_heap_numbers_[i];
     Handle<Object> num = isolate_->factory()->NewNumber(d.value());
@@ -644,9 +686,52 @@ void Deoptimizer::MaterializeHeapNumbers() {
              d.value(),
              d.slot_address());
     }
-
     Memory::Object_at(d.slot_address()) = *num;
   }
+
+  // Materialize arguments objects one frame at a time.
+  int values_index = 0;
+  int arguments_index = 0;
+  for (int frame_index = 0; frame_index < jsframe_count(); ++frame_index) {
+    if (frame_index != 0) it->Advance();
+    JavaScriptFrame* frame = it->frame();
+    Handle<JSFunction> function(JSFunction::cast(frame->function()), isolate_);
+    Handle<JSObject> arguments;
+    for (int i = frame->ComputeExpressionsCount() - 1; i >= 0; --i) {
+      if (frame->GetExpression(i) == isolate_->heap()->arguments_marker()) {
+        ArgumentsObjectMaterializationDescriptor descriptor =
+            deferred_arguments_objects_[arguments_index++];
+        const int length = descriptor.arguments_length();
+        if (arguments.is_null()) {
+          // Construct an arguments object and copy the parameters to a newly
+          // allocated arguments object backing store.
+          arguments = isolate_->factory()->NewArgumentsObject(function, length);
+          Handle<FixedArray> array = isolate_->factory()->NewFixedArray(length);
+          ASSERT(array->length() == length);
+          for (int i = 0; i < length; ++i) {
+            array->set(i, *values[values_index++]);
+          }
+          arguments->set_elements(*array);
+        } else {
+          values_index += length;
+        }
+        if (!frame->has_adapted_arguments()) {

[Sven Panne, 2012/09/12 07:14:22]

Can rearrange things a bit here so that we have a single 'if'?

   if (frame->has_adapted_arguments()) {
      ...
   } else {
      ...
   }

Seeing the different cases is a bit hard currently, but this might be due to the
inherent complexity of the problem. :-)

[Michael Starzinger, 2012/09/12 10:07:28]

Done. I removed the static helper method completely and merged it into this
method. It is now clearer when we use the output frame and when we use the
deoptimization info to materialize the arguments object.

+          frame->SetExpression(i, *arguments);
+          if (FLAG_trace_deopt) {
+            PrintF("Materializing arguments object for %p: ",
+                   reinterpret_cast<void*>(descriptor.slot_address()));
+            arguments->ShortPrint();
+            PrintF("\n");
+          }
+        }
+      }
+    }
+    if (frame->has_adapted_arguments()) {
+      MaterializeArgumentsObjectUsingOutputFrame(isolate_, frame);
+    }
+  }
+  ASSERT_EQ(values_index, values.length());
+  ASSERT_EQ(arguments_index, deferred_arguments_objects_.length());
 }
 
 
@@ -932,8 +1017,8 @@ void Deoptimizer::DoTranslateCommand(TranslationIterator* iterator,
     }
 
     case Translation::ARGUMENTS_OBJECT: {
-      // Use the arguments marker value as a sentinel and fill in the arguments
-      // object after the deoptimized frame is built.
+      int args_index = iterator->Next() + 1;  // Skip receiver.
+      int args_length = iterator->Next() - 1;  // Skip receiver.
       if (FLAG_trace_deopt) {
         PrintF("    0x%08" V8PRIxPTR ": [top + %d] <- ",
                output_[frame_index]->GetTop() + output_offset,
@@ -941,9 +1026,20 @@ void Deoptimizer::DoTranslateCommand(TranslationIterator* iterator,
         isolate_->heap()->arguments_marker()->ShortPrint();
         PrintF(" ; arguments object\n");
       }
+      // Use the arguments marker value as a sentinel and fill in the arguments
+      // object after the deoptimized frame is built.
       intptr_t value = reinterpret_cast<intptr_t>(
           isolate_->heap()->arguments_marker());
+      AddArgumentsObject(
+          output_[frame_index]->GetTop() + output_offset, args_length);
       output_[frame_index]->SetFrameSlot(output_offset, value);
+      // We save the tagged argument values on the side and materialize the
+      // actual arguments object after the deoptimized frame is built.
+      for (int i = 0; i < args_length; i++) {
+        unsigned input_offset = input_->GetOffsetFromSlotIndex(args_index + i);
+        intptr_t input_value = input_->GetFrameSlot(input_offset);
+        AddArgumentsObjectValue(input_value);
+      }
       return;
     }
   }
@@ -1285,8 +1381,19 @@ Object* Deoptimizer::ComputeLiteral(int index) const {
 }
 
 
-void Deoptimizer::AddDoubleValue(intptr_t slot_address,
-                                 double value) {
+void Deoptimizer::AddArgumentsObject(intptr_t slot_address, int argc) {
+  ArgumentsObjectMaterializationDescriptor object_desc(
+      reinterpret_cast<Address>(slot_address), argc);
+  deferred_arguments_objects_.Add(object_desc);
+}
+
+
+void Deoptimizer::AddArgumentsObjectValue(intptr_t value) {
+  deferred_arguments_objects_values_.Add(reinterpret_cast<Object*>(value));
+}
+
+
+void Deoptimizer::AddDoubleValue(intptr_t slot_address, double value) {
   HeapNumberMaterializationDescriptor value_desc(
       reinterpret_cast<Address>(slot_address), value);
   deferred_heap_numbers_.Add(value_desc);
@@ -1570,8 +1677,10 @@ void Translation::StoreLiteral(int literal_id) {
 }
 
 
-void Translation::StoreArgumentsObject() {
+void Translation::StoreArgumentsObject(int args_index, int args_length) {
   buffer_->Add(ARGUMENTS_OBJECT, zone());
+  buffer_->Add(args_index, zone());
+  buffer_->Add(args_length, zone());
 }
 
 
@@ -1582,7 +1691,6 @@ void Translation::MarkDuplicate() {
 
 int Translation::NumberOfOperandsFor(Opcode opcode) {
   switch (opcode) {
-    case ARGUMENTS_OBJECT:
     case DUPLICATE:
       return 0;
     case GETTER_STUB_FRAME:
@@ -1600,6 +1708,7 @@ int Translation::NumberOfOperandsFor(Opcode opcode) {
     case BEGIN:
     case ARGUMENTS_ADAPTOR_FRAME:
     case CONSTRUCT_STUB_FRAME:
+    case ARGUMENTS_OBJECT:
       return 2;
     case JS_FRAME:
       return 3;