Fix very strange bug in FindBreakLocationFromAddress algorithm

Fix very strange bug in FindBreakLocationFromAddress algorithm

R=yangguo@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=15014

[Peter.Rybin, 2013-05-23 00:21:10]

FindBreakLocationFromAddress algorithm is identical to
FindBreakLocationFromPosition (10 lines below), except this very detail.

What's more, the following check
if (distance == 0)
is going to never success without the fix.

This changes semantics of the method. However, it looks like exact sharp pc that
is never actually passed to this method.

[Jakob Kummerow, 2013-05-23 08:05:08]

Drive-by comment:
Does this pass tests? I've stumbled over this oddity before, but had to conclude
that fixing it is a can of worms. See
https://code.google.com/p/v8/issues/detail?id=2047 for my findings.

[Yang, 2013-05-23 09:33:48]

On 2013/05/23 08:05:08, Jakob wrote:
> Drive-by comment:
> Does this pass tests? I've stumbled over this oddity before, but had to
conclude
> that fixing it is a can of worms. See
> https://code.google.com/p/v8/issues/detail?id=2047 for my findings.

It looks correct, but as Jakob said, there seem to be some tests that rely on
this. Maybe the tests are slightly wrong. Please check whether that is the case.

[Peter.Rybin, 2013-05-23 23:00:48]

On 2013/05/23 09:33:48, Yang wrote:
> On 2013/05/23 08:05:08, Jakob wrote:
> > Drive-by comment:
> > Does this pass tests? I've stumbled over this oddity before, but had to
> conclude
> > that fixing it is a can of worms. See
> > https://code.google.com/p/v8/issues/detail?id=2047 for my findings.
> 
> It looks correct, but as Jakob said, there seem to be some tests that rely on
> this. Maybe the tests are slightly wrong. Please check whether that is the
case.

The fix does break code. All usages were about finding current break position by
PC, but PC (as put by CPU on stack) always points to the next instruction, not
the current one. So this weird semantics actually was "find the closest break
location before PC, but not at PC". The PC may actually happen to point to
another break location, as it happens in tests.

I choose to fix the algorithm and fix usages ("pc" => "pc - 1"), as I have
uploaded.

Alternatively, I can keep all intact (with a strange semantics) and patch my own
usage that I plan to submit ("pc" => "pc + 1").

[Yang, 2013-06-07 12:14:51]

On 2013/06/07 11:34:01, peter.rybin wrote:

LGTM. Please add a comment on "pc - 1", something about the pc pointing to the
instruction after the break point.

Alternatively, rename FindBreakLocationFromAddress to something like
FindClosestBreakLocationBeforeAddress and leave the current implementation, but
I assume that you need it in some other places as well?

[Peter.Rybin, 2013-06-07 13:54:12]

On 2013/06/07 12:14:51, Yang wrote:
> On 2013/06/07 11:34:01, peter.rybin wrote:
> 
> LGTM. Please add a comment on "pc - 1", something about the pc pointing to the
> instruction after the break point.
> 
> Alternatively, rename FindBreakLocationFromAddress to something like
> FindClosestBreakLocationBeforeAddress and leave the current implementation,
but
> I assume that you need it in some other places as well?

I should be anyway a matter of taste: either +1 in some places, or -1 in other
places. I think BreakPointFromAddress is a more clear name, so I'm going only to
adjust semantics.

[Peter.Rybin, 2013-06-07 16:48:30]

Committed patchset #4 manually as r15014.