Use a reserved stack local variable to store the Code object so that it can be looked up easily whe…

Change dart frame and stub frame layout to include the PC of the code executing on the frame. The stack frame layout is as follows:
  ret PC
  saved EBP                          <== EBP
  PC (used to locate RawInstruction) <== ESP
Committed: https://code.google.com/p/dart/source/detail?r=7675

[hausner, 2012-05-14 16:55:43]

Do you mind taking a quick look at vm/debugger_ia32.cc (and its friends from
other platforms)? That code makes some assumptions about stack layout. Thanks.

[regis, 2012-05-14 17:33:10]

LGTM

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
File vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
vm/code_generator_ia32.cc:459: int index = ParsedFunction::kFirstStackSlotIndex
+ 1;
const

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
vm/code_generator_ia32.cc:553: // We need to unwind the space we reserved for
locals/copied parms etc.
I add a comment here when reviewing your other overlapping cl.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/flow_graph_comp...
File vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/flow_graph_comp...
vm/flow_graph_compiler_x64.cc:1363: int index =
ParsedFunction::kFirstStackSlotIndex + 1;
const

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/flow_graph_comp...
vm/flow_graph_compiler_x64.cc:1458: // We need to unwind the space we reserved
for locals/copied parms etc.
ditto

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/parser.h
File vm/parser.h (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/parser.h#newcode33
vm/parser.h:33: static const int kFirstStackSlotIndex = -2;
The name is not clear, since parameters also occupy stack slots. How about
kFirstLocalSlotIndex?

In fact, you could also introduce a kLastParamSlotIndex (=1) and use it along
kFirstLocalSlotIndex in the offset calculations.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/stack_frame.cc
File vm/stack_frame.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/stack_frame.cc#...
vm/stack_frame.cc:82: RawObject** end = reinterpret_cast<RawObject**>(fp() - (2
* kWordSize));
Use defined constant instead of 2.

[srdjan, 2012-05-14 20:00:17]

LGTM with comments (in addition to Regis's comments)

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_ia32.cc
File vm/assembler_macros_ia32.cc (right):

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_ia32.c...
vm/assembler_macros_ia32.cc:63: intptr_t offset = assembler->CodeSize();
const

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_ia32.c...
vm/assembler_macros_ia32.cc:75: __ subl(ESP, frame_space);
This could fit well into one line, your choice.

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_ia32.h
File vm/assembler_macros_ia32.h (right):

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_ia32.h...
vm/assembler_macros_ia32.h:53: static void EnterDartFrame(Assembler* assembler,
intptr_t frame_space);
what is 'frame_space' ? frame_size? From EBP-1 to ESP?

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_ia32.h...
vm/assembler_macros_ia32.h:76: //   call L             (size is 5 bytes)
Can we have a Call near added to the assembler (3 instead of 5 bytes) .. maybe
in next CL.

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_x64.cc
File vm/assembler_macros_x64.cc (right):

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_x64.cc...
vm/assembler_macros_x64.cc:67: intptr_t offset = assembler->CodeSize();
ditto

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_x64.h
File vm/assembler_macros_x64.h (right):

http://codereview.chromium.org/10375059/diff/13001/vm/assembler_macros_x64.h#...
vm/assembler_macros_x64.h:76: //   call L             (size is 5 bytes)
ditto

http://codereview.chromium.org/10375059/diff/13001/vm/code_generator_x64.h
File vm/code_generator_x64.h (right):

http://codereview.chromium.org/10375059/diff/13001/vm/code_generator_x64.h#ne...
vm/code_generator_x64.h:13: #include "vm/assembler_macros.h"
Why are these imports needed?

http://codereview.chromium.org/10375059/diff/13001/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/10375059/diff/13001/vm/object.h#newcode2244
vm/object.h:2244: uword pc_;
const?

[siva, 2012-05-15 23:52:39]

Addressed review comments.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
File vm/assembler_macros_ia32.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
vm/assembler_macros_ia32.cc:63: intptr_t offset = assembler->CodeSize();
On 2012/05/14 20:00:17, srdjan wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
vm/assembler_macros_ia32.cc:75: __ subl(ESP, frame_space);
Made it one line.

On 2012/05/14 20:00:17, srdjan wrote:
> This could fit well into one line, your choice.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
File vm/assembler_macros_ia32.h (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
vm/assembler_macros_ia32.h:53: static void EnterDartFrame(Assembler* assembler,
intptr_t frame_space);
Sorry that should be frame_size not frame_space, i.e number
of slots on the frame.

On 2012/05/14 20:00:17, srdjan wrote:
> what is 'frame_space' ? frame_size? From EBP-1 to ESP?

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
vm/assembler_macros_ia32.h:76: //   call L             (size is 5 bytes)
I have added an issue (3069) for this as I did not want to clutter this CL too
much.

On 2012/05/14 20:00:17, srdjan wrote:
> Can we have a Call near added to the assembler (3 instead of 5 bytes) .. maybe
> in next CL.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
File vm/assembler_macros_x64.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
vm/assembler_macros_x64.cc:67: intptr_t offset = assembler->CodeSize();
On 2012/05/14 20:00:17, srdjan wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
File vm/assembler_macros_x64.h (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/assembler_macro...
vm/assembler_macros_x64.h:76: //   call L             (size is 5 bytes)
Created issue (3069).

On 2012/05/14 20:00:17, srdjan wrote:
> ditto

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
File vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
vm/code_generator_ia32.cc:459: int index = ParsedFunction::kFirstStackSlotIndex
+ 1;
On 2012/05/14 17:33:10, regis wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
vm/code_generator_ia32.cc:553: // We need to unwind the space we reserved for
locals/copied parms etc.
Yes, I incorporated your comment into the other CL, so this CL does not have
this change anymore.

On 2012/05/14 17:33:10, regis wrote:
> I add a comment here when reviewing your other overlapping cl.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
File vm/code_generator_x64.h (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/code_generator_...
vm/code_generator_x64.h:13: #include "vm/assembler_macros.h"
Removed these includes here and moved it to stub_generator_x64.cc

On 2012/05/14 20:00:17, srdjan wrote:
> Why are these imports needed?

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/flow_graph_comp...
File vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/flow_graph_comp...
vm/flow_graph_compiler_x64.cc:1363: int index =
ParsedFunction::kFirstStackSlotIndex + 1;
On 2012/05/14 17:33:10, regis wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/flow_graph_comp...
vm/flow_graph_compiler_x64.cc:1458: // We need to unwind the space we reserved
for locals/copied parms etc.
Yes, I incorporated your comment into the other CL that I submitted, these
changes are not in this CL anymore.
On 2012/05/14 17:33:10, regis wrote:
> ditto

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/object.h#newcod...
vm/object.h:2244: uword pc_;
On 2012/05/14 20:00:17, srdjan wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/parser.h
File vm/parser.h (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/parser.h#newcode33
vm/parser.h:33: static const int kFirstStackSlotIndex = -2;
Changed name as discussed offline.

The naming for +1 will be done in another CL.

On 2012/05/14 17:33:10, regis wrote:
> The name is not clear, since parameters also occupy stack slots. How about
> kFirstLocalSlotIndex?
> 
> In fact, you could also introduce a kLastParamSlotIndex (=1) and use it along
> kFirstLocalSlotIndex in the offset calculations.

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/stack_frame.cc
File vm/stack_frame.cc (right):

https://chromiumcodereview.appspot.com/10375059/diff/13001/vm/stack_frame.cc#...
vm/stack_frame.cc:82: RawObject** end = reinterpret_cast<RawObject**>(fp() - (2
* kWordSize));
On 2012/05/14 17:33:10, regis wrote:
> Use defined constant instead of 2.

Done.

[regis, 2012-05-16 00:27:46]

Still LGTM!