Direct call accessor getter callbacks (arm implementation).

src/arm/stub-cache-arm.cc

Index: src/arm/stub-cache-arm.cc
===================================================================
--- src/arm/stub-cache-arm.cc	(revision 6680)
+++ src/arm/stub-cache-arm.cc	(working copy)
@@ -655,12 +655,10 @@
   // already generated). Do not allow the assembler to perform a
   // garbage collection but instead return the allocation failure
   // object.
-  MaybeObject* result = masm->TryCallApiFunctionAndReturn(
-      &fun, argc + kFastApiCallArguments + 1);
-  if (result->IsFailure()) {
-    return result;
-  }
-  return Heap::undefined_value();
+  const int kStackUnwindSpace = argc + kFastApiCallArguments + 1;
+  ExternalReference ref =
+      ExternalReference(&fun, ExternalReference::DIRECT_API_CALL);
+  return masm->TryCallApiFunctionAndReturn(ref, kStackUnwindSpace);
 }
 
 class CallInterceptorCompiler BASE_EMBEDDED {
@@ -1241,22 +1239,43 @@
   __ b(eq, miss);
 
   // Check that the maps haven't changed.
-  Register reg =
+  Register holder_reg =

[antonm, 2011/02/17 16:13:44]

if you change this, please, change other platforms files as well: we're trying
to keep the things in parallel as much as we can,

[Zaheer, 2011/02/21 10:25:35]

Do not wish to touch x86, reverted it back to reg.

       CheckPrototypes(object, receiver, holder, scratch1, scratch2, scratch3,
                       name, miss);
+  ASSERT(holder_reg.is(receiver) || holder_reg.is(scratch1));

[antonm, 2011/02/17 16:13:44]

do you waht to assert this or rather the fact that !holder_reg.is(scratch{2,3})?

[Zaheer, 2011/02/21 10:25:35]

The holder_reg.is(scratch{2,3}) check already happens in checkprototypes.

[antonm, 2011/02/21 17:45:12]

So why you bother asserting here?  What kind of information this assert conveys
to the reader?  Sorry, I am probably missing something obvious here.

[Zaheer, 2011/02/23 06:34:07]

I added this as a sanity check on the return value. Judging from other uses of
CheckPrototypes it may not be required. removed.

 
-  // Push the arguments on the JS stack of the caller.
-  __ push(receiver);  // Receiver.
-  __ mov(scratch3, Operand(Handle<AccessorInfo>(callback)));  // callback data
-  __ ldr(ip, FieldMemOperand(scratch3, AccessorInfo::kDataOffset));
-  __ Push(reg, ip, scratch3, name_reg);
+  // Push AccessorInfo arguments and property name below the
+  // exit frame and hence accessible to GC.
+  __ push(receiver);

[antonm, 2011/02/17 16:13:44]

may I ask you to reformat and comment you code slightly to better communicate
what you're doing here?

I'd suggest:

Comment at ln. 1247 becomes:

// Build AccessorGetter arguments on the stack below exit frame to make GC aware
of them and store pointers to them.
[Empty line]
// Build AccessorInfo on the stack.
__ push(receiver);
__ mov(scratch2, sp);  // scratch2 = &AccessorInfo.
__ push(holder_reg);
....
__ push(sratch3);
[Here comes blank line]
// Push name to the stack.
__ push(name_reg);
__ mov(r0, sp);

[Zaheer, 2011/02/21 10:25:35]

with the merge of pushes, its difficult to separate the blocks. i have modified
the comments.

+  __ mov(scratch2, sp);
+  __ push(holder_reg);
+  Handle<AccessorInfo> callback_handle(callback);
+  if (Heap::InNewSpace(callback_handle->data())) {
+    __ Move(scratch3, callback_handle);
+    __ ldr(scratch3, FieldMemOperand(scratch3, AccessorInfo::kDataOffset));
+  } else {
+    __ Move(scratch3, Handle<Object>(callback_handle->data()));
+  }
+  __ push(scratch3);
+  __ push(name_reg);

[SeRya, 2011/02/20 17:28:39]

__ Push(receiver, scratch3, name_reg) (or at least __ Push(scratch3, name_reg))
apparently should work and save one instruction.

[Zaheer, 2011/02/21 10:25:35]

Done. Merged the last three pushes. cant merge the receiver since it can be same
as holder reg.

+  __ mov(r0, sp);  // r0 = Handle<String>
 
-  // Do tail-call to the runtime system.
-  ExternalReference load_callback_property =
-      ExternalReference(IC_Utility(IC::kLoadCallbackProperty));
-  __ TailCallExternalReference(load_callback_property, 5, 1);
+  Address getter_address = v8::ToCData<Address>(callback->getter());
+  ApiFunction fun(getter_address);
 
-  return Heap::undefined_value();  // Success.
+  const int kApiStackSpace = 1;
+  __ EnterExitFrame(false, kApiStackSpace);
+  __ str(scratch2, MemOperand(sp, 1 * kPointerSize));

[antonm, 2011/02/17 16:13:44]

Please, explain briefly why we need additional wrapping here.

BTW, is this ABI universal across ARM compilers?

[SeRya, 2011/02/20 17:28:39]

Is is an instance of v8::AccessorInfo which consists of a single pointer
internal::Object** args_. Not too hard to conclude from the next comment but may
be worth to mention.

On 2011/02/17 16:13:44, antonm wrote:

[Zaheer, 2011/02/21 10:25:35]

This to create internal::Object** (AccessorInfo only member)
I cant see any ABI/compiler specific code here. May be i miss your point.

[antonm, 2011/02/21 17:45:12]

I am sorry, I thought that's some additional wrapping.

+  __ add(r1, sp, Operand(1 * kPointerSize));  // r1 = AccessorInfo&
+
+  // Emitting a stub call may try to allocate (if the code is not
+  // already generated).  Do not allow the assembler to perform a
+  // garbage collection but instead return the allocation failure
+  // object.
+  const int kStackUnwindSpace = 4;
+  ExternalReference ref =
+      ExternalReference(&fun, ExternalReference::DIRECT_LOAD_CALL);
+  return masm()->TryCallApiFunctionAndReturn(ref, kStackUnwindSpace);
 }