Debugger: use debug break slot to break on call.

Debugger: use debug break slot to break on call.

Break point at calls are currently set via IC. To change this, we
need to set debug break slots instead. We also need to distinguish
those debug break slots as calls to support step-in.

To implement this, we add a data field to debug break reloc info to
indicate non-call debug breaks or in case of call debug breaks, the
number of arguments. We can later use this to find the callee on the
evaluation stack in Debug::PrepareStep.

BUG=v8:4269
R=ulan@chromium.org
LOG=N

Committed: https://crrev.com/8965b683ce39bc3c24ed2466d189323d81a70852
Cr-Commit-Position: refs/heads/master@{#29561}

[Yang, 2015-07-06 14:19:05]

On 2015/07/06 13:55:14, Yang wrote:

this is only the ia32 port. Id like to get an opinion before porting.

[ulan, 2015-07-07 13:31:31]

looks good, few nits:

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc#newcode517
src/assembler.cc:517: if (RelocInfo::IsDebugBreakSlot(rmode)) {
Let's make it into a method with descriptive name e.g. WriteDebugBreakData()

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc#newcode719
src/assembler.cc:719: AdvanceReadInt();
The old name was more discriptive: AdvanceReadPoolData. Can we restore it?

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc#newcode727
src/assembler.cc:727: if (RelocInfo::IsDebugBreakSlot(rmode)) {
Let's extract this into a separate function "AdvanceReadDebugSlotData"

[Yang, 2015-07-07 14:39:00]

Addressed your comments.

I ended up simply implementing the whole thing for setting debug breaks at calls
using debug break slots instead of call ICs.

Also added ports.

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc#newcode517
src/assembler.cc:517: if (RelocInfo::IsDebugBreakSlot(rmode)) {
On 2015/07/07 13:31:31, ulan wrote:
> Let's make it into a method with descriptive name e.g. WriteDebugBreakData()

Done.

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc#newcode719
src/assembler.cc:719: AdvanceReadInt();
On 2015/07/07 13:31:31, ulan wrote:
> The old name was more discriptive: AdvanceReadPoolData. Can we restore it?

Done.

https://codereview.chromium.org/1222093007/diff/1/src/assembler.cc#newcode727
src/assembler.cc:727: if (RelocInfo::IsDebugBreakSlot(rmode)) {
On 2015/07/07 13:31:31, ulan wrote:
> Let's extract this into a separate function "AdvanceReadDebugSlotData"

Done.

[ulan, 2015-07-08 11:53:33]

looking good.

https://codereview.chromium.org/1222093007/diff/40001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1222093007/diff/40001/src/assembler.cc#newcod...
src/assembler.cc:737: if (SetMode(rmode)) return
AdvanceReadDebugBreakSlotData();
Did you mean

if (SetMode(rmode)) return;
AdvanceReadDebugBreakSlotData()

here?

https://codereview.chromium.org/1222093007/diff/40001/src/debug.h
File src/debug.h (right):

https://codereview.chromium.org/1222093007/diff/40001/src/debug.h#newcode88
src/debug.h:88: inline int CallArgc() const {
Please use CallArgCount() or CallArgumentsCount()

https://codereview.chromium.org/1222093007/diff/40001/src/debug.h#newcode128
src/debug.h:128: static const int kNonCallArgc = -2;
Please use the same constants here and in assembler.h

[Yang, 2015-07-09 09:38:48]

I foolishly rebased. In the third patch set I only touched src/debug.cc,
src/debug.h and src/assembler.cc

https://codereview.chromium.org/1222093007/diff/40001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1222093007/diff/40001/src/assembler.cc#newcod...
src/assembler.cc:737: if (SetMode(rmode)) return
AdvanceReadDebugBreakSlotData();
On 2015/07/08 11:53:33, ulan wrote:
> Did you mean
> 
> if (SetMode(rmode)) return;
> AdvanceReadDebugBreakSlotData()
> 
> here?

actually no. if SetMode returns true, we want to read the slot data, and then
return. I changed this to be less ambiguous.

https://codereview.chromium.org/1222093007/diff/40001/src/debug.h
File src/debug.h (right):

https://codereview.chromium.org/1222093007/diff/40001/src/debug.h#newcode88
src/debug.h:88: inline int CallArgc() const {
On 2015/07/08 11:53:33, ulan wrote:
> Please use CallArgCount() or CallArgumentsCount()

Done.

https://codereview.chromium.org/1222093007/diff/40001/src/debug.h#newcode128
src/debug.h:128: static const int kNonCallArgc = -2;
On 2015/07/08 11:53:33, ulan wrote:
> Please use the same constants here and in assembler.h

Removed. They are not actually necessary anymore.

[Yang, 2015-07-09 11:03:26]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 11:03:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222093007/60001

[commit-bot: I haz the power, 2015-07-09 11:06:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-09 11:06:48]

Dry run: Try jobs failed on following builders:
  v8_win64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/6510)

[ulan, 2015-07-10 08:30:58]

lgtm

[Yang, 2015-07-10 08:33:24]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-07-10 08:33:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1222093007/60001

[commit-bot: I haz the power, 2015-07-10 08:49:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-10 08:49:39]

Patchset 4 (id:??) landed as
https://crrev.com/8965b683ce39bc3c24ed2466d189323d81a70852
Cr-Commit-Position: refs/heads/master@{#29561}