Setup the pool pointer when entering Dart from C++ so the intrinsic functions may rely on it being …

Setup the pool pointer when entering Dart from C++ so the intrinsic functions may rely on it being valid.

R=asiva@google.com, zra@google.com

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

[rmacnak, 2013-09-18 21:04:24]

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/dart_api_impl_t...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:236: EXPECT_VALID(Dart_ObjectEquals(nan, nan,
&equal));
This will hit CompareDoubles.

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/intrinsifier_x6...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/intrinsifier_x6...
runtime/vm/intrinsifier_x64.cc:918: const intptr_t kReceiverOffset = 2;
Perhaps constants like these deserve to be defined once and used across the
intrinsics? Naming them might be an issue because the offsets of arguments
varies depending on whether the function is a method or static function.

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

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.c...
runtime/vm/stub_code_x64.cc:784: // R15 is already saved above by
EnterStubFrameWithPP.
Actually, we MUST not save R15 again, because it changes the offsets of the
Context pointer and exit frame info pushed below.

[siva, 2013-09-19 00:45:37]

LGTM with some comments on comments.

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/intrinsifier_x6...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/intrinsifier_x6...
runtime/vm/intrinsifier_x64.cc:918: const intptr_t kReceiverOffset = 2;
I prefer leaving the constants in each intrinsic, it is local and matches with
the code below and is easier to verify.

On 2013/09/18 21:04:24, Ryan Macnak wrote:
> Perhaps constants like these deserve to be defined once and used across the
> intrinsics? Naming them might be an issue because the offsets of arguments
> varies depending on whether the function is a method or static function.

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/intrinsifier_x6...
runtime/vm/intrinsifier_x64.cc:920: 
I think we should also add a comment that these intrinsics should only use
Bool::True, Bool::False and Object::null as the PP that we have setup in the
invocation stub will only be able to access these objects.

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

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.c...
runtime/vm/stub_code_x64.cc:771: const intptr_t kInitiallyPushedSlots = 3;
Not sure if kInitiallyPushedSlots is a good name, maybe
const int kInitialOffset = 3?

And the comment could include this stack layout structure

              | saved PP           |
              | callee's PC marker |
              | saved RBP          |    (RBP of current frame)
              | saved PC           |    (PC of current frame)
                                        <- RSP of current frame

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.c...
runtime/vm/stub_code_x64.cc:784: // R15 is already saved above by
EnterStubFrameWithPP.
True, the comment is to make sure we document that all the 
callee-saved registers per the C++ ABI are indeed saved.

(if you want you can add a comment that the offsets below of context pointer,
exit frame etc, need to be changed if additional values are pushed on the
stack).

On 2013/09/18 21:04:24, Ryan Macnak wrote:
> Actually, we MUST not save R15 again, because it changes the offsets of the
> Context pointer and exit frame info pushed below.

[zra, 2013-09-19 00:55:22]

lgtm

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

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.c...
runtime/vm/stub_code_x64.cc:771: const intptr_t kInitiallyPushedSlots = 3;
On 2013/09/19 00:45:37, siva wrote:
> Not sure if kInitiallyPushedSlots is a good name, maybe
> const int kInitialOffset = 3?
> 
> And the comment could include this stack layout structure
> 
>               | saved PP           |
>               | callee's PC marker |
>               | saved RBP          |    (RBP of current frame)
>               | saved PC           |    (PC of current frame)
>                                         <- RSP of current frame

+1

[rmacnak, 2013-09-19 16:57:36]

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

https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.c...
runtime/vm/stub_code_x64.cc:771: const intptr_t kInitiallyPushedSlots = 3;
On 2013/09/19 00:45:37, siva wrote:
> Not sure if kInitiallyPushedSlots is a good name, maybe
> const int kInitialOffset = 3?
> 
> And the comment could include this stack layout structure
> 
>               | saved PP           |
>               | callee's PC marker |
>               | saved RBP          |    (RBP of current frame)
>               | saved PC           |    (PC of current frame)
>                                         <- RSP of current frame

This stack looks upside down to me.

  __ EnterStubFrameWithPP();

  // At this point, the stack looks like:
  // | saved PC (return to DartEntry::InvokeFunction) |  
  // | saved RBP                                      | <-- RBP
  // | "saved PC area" = 0                            |
  // | saved RC15/PP                                  | <-- RSP

What is the middle value, what you labelled as "callee's PC marker" and what the
code calls "the saved PC area"? It looks like it is being used to find the
RawCode associated with a frame, so for the stub frame it will remain null?

[rmacnak, 2013-09-20 00:28:59]

On 2013/09/19 16:57:36, Ryan Macnak wrote:
> https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.cc
> File runtime/vm/stub_code_x64.cc (right):
> 
>
https://codereview.chromium.org/23480098/diff/5001/runtime/vm/stub_code_x64.c...
> runtime/vm/stub_code_x64.cc:771: const intptr_t kInitiallyPushedSlots = 3;
> On 2013/09/19 00:45:37, siva wrote:
> > Not sure if kInitiallyPushedSlots is a good name, maybe
> > const int kInitialOffset = 3?
> > 
> > And the comment could include this stack layout structure
> > 
> >               | saved PP           |
> >               | callee's PC marker |
> >               | saved RBP          |    (RBP of current frame)
> >               | saved PC           |    (PC of current frame)
> >                                         <- RSP of current frame
> 
> This stack looks upside down to me.
> 
>   __ EnterStubFrameWithPP();
> 
>   // At this point, the stack looks like:
>   // | saved PC (return to DartEntry::InvokeFunction) |  
>   // | saved RBP                                      | <-- RBP
>   // | "saved PC area" = 0                            |
>   // | saved RC15/PP                                  | <-- RSP
> 
> What is the middle value, what you labelled as "callee's PC marker" and what
the
> code calls "the saved PC area"? It looks like it is being used to find the
> RawCode associated with a frame, so for the stub frame it will remain null?

Using low addresses on top as discussed.

[rmacnak, 2013-09-20 00:33:30]

Committed patchset #4 manually as r27683 (presubmit successful).