Fix for issue 14790 - Crash when using dartium devtools

Fix for issue 14790 - Crash when using dartium devtools

The stack frame iteration during GC was missing one slot in stub frames that
did not save the pool pointer.

- This change makes all stub frames uniform, i.e they always save/restore the pool pointer. This ensures that we will traverse all slots on the stack.

- A new constant called kFirstObjectSlotFromFp has been added which is used as the slot to start stack traversal.

(The ARM and MIPs changes will be in a different CL)

R=iposva@google.com, regis@google.com, zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30088

[zra, 2013-11-07 19:28:17]

lgtm

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2876: void Assembler::LeaveStubFrame() {
This looks the same as LeaveDartFrame. I can see the benefit of using names
Enter/LeaveStubFrame and Enter/LeaveDartFrame, but would it make sense to have
LeaveStubFrame and LeaveDartFrame call the same function?

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame.c...
runtime/vm/stack_frame.cc:71: // Optimized frames have a stack map we need to
visit the frame based
map. We

[regis, 2013-11-07 19:53:11]

LGTM

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_a...
File runtime/vm/stack_frame_arm.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_a...
runtime/vm/stack_frame_arm.h:29: * against a slot indicates it needs to be
traversed during GC.
For clarity, I would place * in all frames, not just the current one, because
this drawing of the current frame does not explicitly show the callee's
arguments, and incoming parameters are in the caller's frame.

Also, when I first saw *, I thought "pointer dereference".
How about a T (for "traversed") on the right edge where you have more space.
People are use to see the tax marker on receipts :-) 
                | first local       T|

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_i...
File runtime/vm/stack_frame_ia32.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_i...
runtime/vm/stack_frame_ia32.h:26: * against a slot indicates it needs to be
traversed during GC.
ditto

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_m...
File runtime/vm/stack_frame_mips.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_m...
runtime/vm/stack_frame_mips.h:27: * against a slot indicates it needs to be
traversed during GC.
ditto

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_x...
File runtime/vm/stack_frame_x64.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_x...
runtime/vm/stack_frame_x64.h:29: * against a slot indicates it needs to be
traversed during GC.
ditto

[Ivan Posva, 2013-11-07 22:11:57]

LGTM

-Ivan

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.h:778: //   ret PC
In all other ASCII graphics we have low addresses at the top.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.h:806: //   ret PC
ditto

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64_test.cc:1628: __ EnterDartFrame(0);
I don't think we need to use EnterDartFrame at all here and below?

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64_test.cc:1787: __ EnterDartFrame(0);
All we need here is saving and restoring of PP.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/instructions_...
File runtime/vm/instructions_x64_test.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/instructions_...
runtime/vm/instructions_x64_test.cc:31: __ EnterDartFrame(0);  // 20 bytes
ditto

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/object_x64_te...
File runtime/vm/object_x64_test.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/object_x64_te...
runtime/vm/object_x64_test.cc:37: __ EnterDartFrame(0);
ditto

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stub_code_x64...
runtime/vm/stub_code_x64.cc:649: __ EnterStubFrame(true);  // Uses PP to access
null object.
Where PP is only used to access null we do not need to load it specifically and
can reuse the one from the caller.

Here and some other places.

[siva, 2013-11-07 23:08:19]

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2876: void Assembler::LeaveStubFrame() {
On 2013/11/07 19:28:17, zra wrote:
> This looks the same as LeaveDartFrame. I can see the benefit of using names
> Enter/LeaveStubFrame and Enter/LeaveDartFrame, but would it make sense to have
> LeaveStubFrame and LeaveDartFrame call the same function?

I have tried to be consistent with how it is done in ARM and MIPs,
maybe I could do another CL after this to factor out the code into a helper
method that could be called from here.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.h:778: //   ret PC
On 2013/11/07 22:11:58, Ivan Posva wrote:
> In all other ASCII graphics we have low addresses at the top.

Inverted the layout.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.h:806: //   ret PC
On 2013/11/07 22:11:58, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64_test.cc:1628: __ EnterDartFrame(0);
On 2013/11/07 22:11:58, Ivan Posva wrote:
> I don't think we need to use EnterDartFrame at all here and below?

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64_test.cc:1787: __ EnterDartFrame(0);
On 2013/11/07 22:11:58, Ivan Posva wrote:
> All we need here is saving and restoring of PP.

Changed to EnterFrame() ... LeaveFrame as it is needed to ensure all pushed
values are popped.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/instructions_...
File runtime/vm/instructions_x64_test.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/instructions_...
runtime/vm/instructions_x64_test.cc:31: __ EnterDartFrame(0);  // 20 bytes
On 2013/11/07 22:11:58, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/object_x64_te...
File runtime/vm/object_x64_test.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/object_x64_te...
runtime/vm/object_x64_test.cc:37: __ EnterDartFrame(0);
On 2013/11/07 22:11:58, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame.c...
runtime/vm/stack_frame.cc:71: // Optimized frames have a stack map we need to
visit the frame based
On 2013/11/07 19:28:17, zra wrote:
> map. We

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_a...
File runtime/vm/stack_frame_arm.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_a...
runtime/vm/stack_frame_arm.h:29: * against a slot indicates it needs to be
traversed during GC.
On 2013/11/07 19:53:11, regis wrote:
> For clarity, I would place * in all frames, not just the current one, because
> this drawing of the current frame does not explicitly show the callee's
> arguments, and incoming parameters are in the caller's frame.
> 

I am interested in what needs to be traversed in a frame and that is why I
limited it to just the current frame. The current frame is responsible for only
what is in it's frame. 



> Also, when I first saw *, I thought "pointer dereference".
> How about a T (for "traversed") on the right edge where you have more space.
> People are use to see the tax marker on receipts :-) 
>                 | first local       T|

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_i...
File runtime/vm/stack_frame_ia32.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_i...
runtime/vm/stack_frame_ia32.h:26: * against a slot indicates it needs to be
traversed during GC.
On 2013/11/07 19:53:11, regis wrote:
> ditto

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_m...
File runtime/vm/stack_frame_mips.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_m...
runtime/vm/stack_frame_mips.h:27: * against a slot indicates it needs to be
traversed during GC.
On 2013/11/07 19:53:11, regis wrote:
> ditto

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_x...
File runtime/vm/stack_frame_x64.h (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stack_frame_x...
runtime/vm/stack_frame_x64.h:29: * against a slot indicates it needs to be
traversed during GC.
On 2013/11/07 19:53:11, regis wrote:
> ditto

Done.

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/63093003/diff/380001/runtime/vm/stub_code_x64...
runtime/vm/stub_code_x64.cc:649: __ EnterStubFrame(true);  // Uses PP to access
null object.
On 2013/11/07 22:11:58, Ivan Posva wrote:
> Where PP is only used to access null we do not need to load it specifically
and
> can reuse the one from the caller.
> 
> Here and some other places.

Done.

[regis, 2013-11-07 23:17:52]

LGTM

https://codereview.chromium.org/63093003/diff/510001/runtime/vm/stack_frame_a...
File runtime/vm/stack_frame_arm.h (right):

https://codereview.chromium.org/63093003/diff/510001/runtime/vm/stack_frame_a...
runtime/vm/stack_frame_arm.h:29: R against a slot indicates it needs to be
traversed during GC.
R -> T

[siva, 2013-11-08 00:25:47]

https://codereview.chromium.org/63093003/diff/510001/runtime/vm/stack_frame_a...
File runtime/vm/stack_frame_arm.h (right):

https://codereview.chromium.org/63093003/diff/510001/runtime/vm/stack_frame_a...
runtime/vm/stack_frame_arm.h:29: R against a slot indicates it needs to be
traversed during GC.
On 2013/11/07 23:17:52, regis wrote:
> R -> T

Done.

[siva, 2013-11-08 00:25:53]

Committed patchset #6 manually as r30088 (presubmit successful).