[IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames.

src/ic/ic.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/ic/ic.h"
 
 #include <iostream>
 
 #include "src/accessors.h"
 #include "src/api-arguments-inl.h"
@@ skipping 174 matching lines @@
   StackFrameIterator it(isolate);
   for (int i = 0; i < depth + 1; i++) it.Advance();
   StackFrame* frame = it.frame();
   DCHECK(fp == frame->fp() && pc_address == frame->pc_address());
 #endif
   // For interpreted functions, some bytecode handlers construct a
   // frame. We have to skip the constructed frame to find the interpreted
   // function's frame. Check if the there is an additional frame, and if there
   // is skip this frame. However, the pc should not be updated. The call to
   // ICs happen from bytecode handlers.
+  // TODO(rmcilroy): Remove this once bytecode handlers don't need a frame.

[mythria, 2016/10/31 15:47:37]

Do we need this TODO? since we need to call into runtime for this handler, is it
possible to avoid a frame?

[rmcilroy, 2016/11/01 13:01:02]

Yeah I had plans to avoid needing to build the frame, but it's probably not
worth it. Could you just remove this TODO please.

[mythria, 2016/11/01 15:55:53]

Done.

   Object* frame_type =
       Memory::Object_at(fp + TypedFrameConstants::kFrameTypeOffset);
   if (frame_type == Smi::FromInt(StackFrame::STUB)) {
     fp = Memory::Address_at(fp + TypedFrameConstants::kCallerFPOffset);
   }
+
   fp_ = fp;
+

[rmcilroy, 2016/11/01 13:01:02]

Intended changes?

[mythria, 2016/11/01 15:55:53]

No. Removed them. Done.

   if (FLAG_enable_embedded_constant_pool) {
     constant_pool_address_ = constant_pool;
   }
   pc_address_ = StackFrame::ResolveReturnAddressLocation(pc_address);
   Code* target = this->target();
   kind_ = target->kind();
   state_ = UseVector() ? nexus->StateFromFeedback() : StateFromCode(target);
   old_state_ = state_;
   extra_ic_state_ = target->extra_ic_state();
 }
@@ skipping 32 matching lines @@
       return UNINITIALIZED;
   }
 }
 
 SharedFunctionInfo* IC::GetSharedFunctionInfo() const {
   // Compute the JavaScript frame for the frame pointer of this IC
   // structure. We need this to be able to find the function
   // corresponding to the frame.
   StackFrameIterator it(isolate());
   while (it.frame()->fp() != this->fp()) it.Advance();
-  if (it.frame()->type() == StackFrame::STUB) {
-    // We might need to advance over bytecode handler frame for Ignition.
-    it.Advance();
-  }
+  // For ignition, bytecode handlers build a stub frame. This frame should be
+  // skipped in the constructor. DCHECK to be sure we skipped the frame.
+  DCHECK(it.frame()->type() != StackFrame::STUB);

[mythria, 2016/10/31 15:47:37]

I wanted to add a check to see if it is the correct type, but that has a lot of
frame types: Interpreted, optimized, javascript, builtin and may be
ArgumentsAdaptor.

[rmcilroy, 2016/11/01 13:01:02]

Hmm, we could probably just remove this - I'm not sure we need to check it isn't
a STUB frame.

[mythria, 2016/11/01 15:55:54]

Removed it. I wasn't very happy with that check either. 

+
   JavaScriptFrame* frame = JavaScriptFrame::cast(it.frame());
   // Find the function on the stack and both the active code for the
   // function and the original code.
   JSFunction* function = frame->function();
   return function->shared();
 }
 
 
 Code* IC::GetCode() const {
   HandleScope scope(isolate());
@@ skipping 2802 matching lines @@
     DCHECK_EQ(LookupIterator::INTERCEPTOR, it.state());
     it.Next();
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result,
                                        Object::GetProperty(&it));
   }
 
   return *result;
 }
 }  // namespace internal
 }  // namespace v8