Fix x64 patching of asllocation stub: the pool pointer must be set-up before patching jump is execu…

Fix x64 patching of asllocation stub: the pool pointer must be set-up before patching jump is executed.

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

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

[srdjan, 2014-09-16 20:19:39]

srdjan@google.com changed reviewers:
+ zra@google.com

[zra, 2014-09-16 20:55:59]

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

https://codereview.chromium.org/574843002/diff/1/runtime/vm/stub_code_x64.cc#...
runtime/vm/stub_code_x64.cc:1094: *entry_patch_offset = assembler->CodeSize();
Since this is a delicate bit of code, I'd recommend adding a function to the x64
assembler instead of duplicating from EmitFrameEntry. (It also appears in the
else-if case in EmitFrameEntry, but duplicating there is possibly not as bad
since they're right next to each other).

[srdjan, 2014-09-16 21:54:50]

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

https://codereview.chromium.org/574843002/diff/1/runtime/vm/stub_code_x64.cc#...
runtime/vm/stub_code_x64.cc:1094: *entry_patch_offset = assembler->CodeSize();
On 2014/09/16 20:55:58, zra wrote:
> Since this is a delicate bit of code, I'd recommend adding a function to the
x64
> assembler instead of duplicating from EmitFrameEntry. (It also appears in the
> else-if case in EmitFrameEntry, but duplicating there is possibly not as bad
> since they're right next to each other).

Good suggestion! Done.

[koda, 2014-09-16 22:01:29]

koda@google.com changed reviewers:
+ koda@google.com

[koda, 2014-09-16 22:01:30]

lgtm

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

https://codereview.chromium.org/574843002/diff/20001/runtime/vm/stub_code.cc#...
runtime/vm/stub_code.cc:121: uword patch_code_offset = 0;;
Stray semicolon.

[zra, 2014-09-16 22:18:02]

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

https://codereview.chromium.org/574843002/diff/40001/runtime/vm/stub_code_x64...
runtime/vm/stub_code_x64.cc:1077: __ SetupPP(new_pp, new_pc);
Looking at this more closely, I think this could actually just be:

__ LoadPoolPointer(new_pp)

The only reason EmitFrameEntry does something else is to avoid duplicating work
creating the PC marker. Sorry for not remembering this earlier.

[srdjan, 2014-09-16 22:42:43]

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

https://codereview.chromium.org/574843002/diff/40001/runtime/vm/stub_code_x64...
runtime/vm/stub_code_x64.cc:1077: __ SetupPP(new_pp, new_pc);
On 2014/09/16 22:18:01, zra wrote:
> Looking at this more closely, I think this could actually just be:
> 
> __ LoadPoolPointer(new_pp)
> 
> The only reason EmitFrameEntry does something else is to avoid duplicating
work
> creating the PC marker. Sorry for not remembering this earlier.

Reverting the change in assembler (SetupPP), using LoadPoolPointer.

[zra, 2014-09-16 22:50:59]

lgtm

[srdjan, 2014-09-16 23:14:59]

Committed patchset #5 (id:80001) manually as r40358 (presubmit successful).