Move some isolate state setup from C++ code to JumpToExceptionHandler stub

Move some isolate state setup from C++ code to JumpToExceptionHandler stub

This change means that the isolate's top exit frame info and vm tag will only be updated after setting the frame pointer.

This is step one in a series of changes to allow the profiler to walk the stack when there is no frame pointer.

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

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

[regis, 2014-07-14 20:00:34]

LGTM

All my comments are regarding the use of CTX, which may be free, but it would be
clearer to use a different register.

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm.cc
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm.cc#...
runtime/vm/stub_code_arm.cc:1698: __ ldr(CTX, Address(SP, 4));  // Isolate.
Are you sure it is OK to trash the CTX register? I guess it is, but to be safe,
you could use R3.

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm64.cc
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm64.c...
runtime/vm/stub_code_arm64.cc:1808: __ mov(CTX, R5);  // Isolate.
Why not use R5 below?

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_ia32.cc...
runtime/vm/stub_code_ia32.cc:1761: __ movl(CTX, Address(ESP, 6 * kWordSize)); 
// Load target isolate.
You could use EAX instead of CTX.

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_mips.cc
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_mips.cc...
runtime/vm/stub_code_mips.cc:1905: __ lw(CTX, Address(SP, 5 * kWordSize));  //
Isolate.
A3?

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

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_x64.cc#...
runtime/vm/stub_code_x64.cc:1732: Register isolate_reg = CTX;
You could use RAX.

[siva, 2014-07-14 20:13:12]

LGTM with some comments

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:288: isolate->set_top_context(Context::null());
Why not move the resetting of the top context also into the
JumpToExceptionHandler code.

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm.cc
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1727: __ mov(SP, Operand(IP));  // Stack pointer.
The comment here is confusing, stack pointer is being set twice
according to the comments???

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1733: __ StoreToOffset(kWord, R2, CTX,
Isolate::top_exit_frame_info_offset());
Not sure if using CTX like this is a good idea it appears as though
setting CTX is part of the contract to jump to the exception
handler code whereas here you are using it as a temporary register
to access the isolate fields.

Why not use TMP (i.e IP)

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm64.cc:1837: __ StoreToOffset(ZR, CTX,
Isolate::top_exit_frame_info_offset(), kNoPP);
Ditto comment about CTX here and in other target files.

[Cutch, 2014-07-14 21:32:08]

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm.cc
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm.cc#...
runtime/vm/stub_code_arm.cc:1698: __ ldr(CTX, Address(SP, 4));  // Isolate.
On 2014/07/14 20:00:34, regis wrote:
> Are you sure it is OK to trash the CTX register? I guess it is, but to be
safe,
> you could use R3.

Done.

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm64.cc
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_arm64.c...
runtime/vm/stub_code_arm64.cc:1808: __ mov(CTX, R5);  // Isolate.
On 2014/07/14 20:00:34, regis wrote:
> Why not use R5 below?

Done.

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_ia32.cc...
runtime/vm/stub_code_ia32.cc:1761: __ movl(CTX, Address(ESP, 6 * kWordSize)); 
// Load target isolate.
On 2014/07/14 20:00:34, regis wrote:
> You could use EAX instead of CTX.

Can't use EAX because it is the same askExceptionObjectReg. Switched to EDI.

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_mips.cc
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_mips.cc...
runtime/vm/stub_code_mips.cc:1905: __ lw(CTX, Address(SP, 5 * kWordSize));  //
Isolate.
On 2014/07/14 20:00:34, regis wrote:
> A3?

Done.

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

https://codereview.chromium.org/396463003/diff/1/runtime/vm/stub_code_x64.cc#...
runtime/vm/stub_code_x64.cc:1732: Register isolate_reg = CTX;
On 2014/07/14 20:00:34, regis wrote:
> You could use RAX.

Can't use RAX because it's used by kExceptionObjectReg. Using RDI which is free.

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:288: isolate->set_top_context(Context::null());
On 2014/07/14 20:13:11, siva wrote:
> Why not move the resetting of the top context also into the
> JumpToExceptionHandler code.

I split it this way because we do not have a pool pointer in the jump to
exception stub and we always load the null object from the pool.

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm.cc
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1727: __ mov(SP, Operand(IP));  // Stack pointer.
On 2014/07/14 20:13:12, siva wrote:
> The comment here is confusing, stack pointer is being set twice
> according to the comments???

I've cleaned up the comments to make it more clear.

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1733: __ StoreToOffset(kWord, R2, CTX,
Isolate::top_exit_frame_info_offset());
On 2014/07/14 20:13:12, siva wrote:
> Not sure if using CTX like this is a good idea it appears as though
> setting CTX is part of the contract to jump to the exception
> handler code whereas here you are using it as a temporary register
> to access the isolate fields.
> 
> Why not use TMP (i.e IP)

AFAIK Setting CTX is not part of the contract of jumping to the exception
handler. It's a free register that can be used in this stub. I agree it reads
weird and have replaced all usage of it with another free register.

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/396463003/diff/20001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm64.cc:1837: __ StoreToOffset(ZR, CTX,
Isolate::top_exit_frame_info_offset(), kNoPP);
On 2014/07/14 20:13:12, siva wrote:
> Ditto comment about CTX here and in other target files.

Acknowledged.

[Cutch, 2014-07-14 22:00:05]

Committed patchset #3 manually as r38216 (presubmit successful).