Specialize breakpoint stubs by set of live registers of the stubs they are intercepting.

Specialize breakpoint stubs by set of live registers of the stubs they are intercepting.

Trades space in the constant pool for space at call sites.

R=fschneider@google.com

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

[rmacnak, 2014-06-27 22:41:49]

Ignore the DART_UNUSED, I need this to run on my machine after the Trusty update
(gcc 4.8). (https://codereview.chromium.org/356233003/)

[Florian Schneider, 2014-06-30 09:00:40]

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:65: CodePatcher::SetPoolOffsetAt(pc_, stub_offset);
Why use the constant pool here?

So ARM and MIPS have a constant pool, but don't use it here, where as x64 and
arm64 do use it. I find this inconsistency suboptimal unless there are very good
reasons to use the constant pool here.

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

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1567: __ PushList((1 << R0) | (1 << R4) | (1 <<
R5));
Why do you need to preserve R4 and R5 here?

[rmacnak, 2014-07-01 01:08:51]

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:65: CodePatcher::SetPoolOffsetAt(pc_, stub_offset);
On 2014/06/30 09:00:40, Florian Schneider wrote:
> Why use the constant pool here?
> 
> So ARM and MIPS have a constant pool, but don't use it here, where as x64 and
> arm64 do use it. I find this inconsistency suboptimal unless there are very
good
> reasons to use the constant pool here.

Not sure. Was introduced in https://codereview.chromium.org/131853006/.

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

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1567: __ PushList((1 << R0) | (1 << R4) | (1 <<
R5));
On 2014/06/30 09:00:40, Florian Schneider wrote:
> Why do you need to preserve R4 and R5 here?

Don't I need to preserve them for the original stub?

save r4/5
load r4/5 with values for runtime call BreakpointRuntimeHandler
call
restore r4/5
jump to original stub (r0)

My test passes if they aren't though..

[Florian Schneider, 2014-07-03 19:56:03]

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

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:1567: __ PushList((1 << R0) | (1 << R4) | (1 <<
R5));
On 2014/07/01 01:08:51, rmacnak wrote:
> On 2014/06/30 09:00:40, Florian Schneider wrote:
> > Why do you need to preserve R4 and R5 here?
> 
> Don't I need to preserve them for the original stub?
> 
> save r4/5
> load r4/5 with values for runtime call BreakpointRuntimeHandler
> call
> restore r4/5
> jump to original stub (r0)
> 
> My test passes if they aren't though..

I don't think so.  When doing a code search for kRuntimeCall I only see one
place where it is used: the call to the IdenticalWithNumberCheck stub. R4 and R5
are not used for anything there.

[rmacnak, 2014-07-09 21:23:29]

On 2014/07/03 19:56:03, Florian Schneider wrote:
>
https://codereview.chromium.org/339183010/diff/80001/runtime/vm/stub_code_arm.cc
> File runtime/vm/stub_code_arm.cc (right):
> 
>
https://codereview.chromium.org/339183010/diff/80001/runtime/vm/stub_code_arm...
> runtime/vm/stub_code_arm.cc:1567: __ PushList((1 << R0) | (1 << R4) | (1 <<
> R5));
> On 2014/07/01 01:08:51, rmacnak wrote:
> > On 2014/06/30 09:00:40, Florian Schneider wrote:
> > > Why do you need to preserve R4 and R5 here?
> > 
> > Don't I need to preserve them for the original stub?
> > 
> > save r4/5
> > load r4/5 with values for runtime call BreakpointRuntimeHandler
> > call
> > restore r4/5
> > jump to original stub (r0)
> > 
> > My test passes if they aren't though..
> 
> I don't think so.  When doing a code search for kRuntimeCall I only see one
> place where it is used: the call to the IdenticalWithNumberCheck stub. R4 and
R5
> are not used for anything there.

Oh, I was thinking that CallToRuntime was also using kRuntimeCall, when it
actually uses kOther. (That naming should probably be reversed.) Removed saving
those registers.

[rmacnak, 2014-07-09 21:23:46]

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/339183010/diff/80001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:65: CodePatcher::SetPoolOffsetAt(pc_, stub_offset);
On 2014/07/01 01:08:51, rmacnak wrote:
> On 2014/06/30 09:00:40, Florian Schneider wrote:
> > Why use the constant pool here?
> > 
> > So ARM and MIPS have a constant pool, but don't use it here, where as x64
and
> > arm64 do use it. I find this inconsistency suboptimal unless there are very
> good
> > reasons to use the constant pool here.
> 
> Not sure. Was introduced in https://codereview.chromium.org/131853006/.

The difference is x64/arm64 reuse constant pool entries. A stub can be shared
between multiple call sites, and a breakpoint might be set for only one of them,
so we can't patch the pool entry.

[Florian Schneider, 2014-07-14 11:36:39]

Lgtm.

https://codereview.chromium.org/339183010/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm64.cc (left):

https://codereview.chromium.org/339183010/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm64.cc:1323: __ LoadImmediate(R5,
kInvalidObjectPointer, kNoPP);
Shouldn't that be R4 as well?

[rmacnak, 2014-07-14 19:34:05]

On 2014/07/14 11:36:39, Florian Schneider wrote:
> Lgtm.
> 
>
https://codereview.chromium.org/339183010/diff/100001/runtime/vm/flow_graph_c...
> File runtime/vm/flow_graph_compiler_arm64.cc (left):
> 
>
https://codereview.chromium.org/339183010/diff/100001/runtime/vm/flow_graph_c...
> runtime/vm/flow_graph_compiler_arm64.cc:1323: __ LoadImmediate(R5,
> kInvalidObjectPointer, kNoPP);
> Shouldn't that be R4 as well?

Yes. Added and for the other ISAs.

[rmacnak, 2014-07-14 19:38:28]

Committed patchset #7 manually as r38205 (presubmit successful).