Defer the writing of the source position data to the relocation information...

Defer the writing of the source position data to the relocation information
until a possible debug break location is reached. Currently this is call sites
with calls to code objects and JS return. Source position information in the
code therefore no longer refers to the "first" instruction generated for a
given source position (which was not the case defered code anyway) but to the
first break location after that source position was passed (again defered code
always start with source position information). This doesn't make a difference
for the debugger as it will always be stopped only at debug break locations.
However, this makes the life of the peep-hole optimizer much easier as many
oportunities for posh/pop eliminations where previosly blocked by relocation
information already written to the code object.

Two types of source positions are still collected. Statement positions indicate
the position of the start of the statement leading to this code and (plain)
positions indicate other places typically call sites to help indicate current
position in backtraces. The two different types of positions are also used to
distinguish between step next and step in.

Runs all the tests (including debugger tests) as before.

Moved the checking for the FLAG_debug_info to one place.

I will do the same changes to the ARM codegenerator in a seperate changelist.


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

[iposva, 2008-09-18 08:02:24]

A couple of nits I found while looking at this. Obviously I like the direction
of this change.

-Ivan

http://codereview.chromium.org/2957/diff/1/8
File src/assembler-ia32.cc (right):

http://codereview.chromium.org/2957/diff/1/8#newcode1865
Line 1865: if (pos == last_position_) return;
Isn't this check redundant?

http://codereview.chromium.org/2957/diff/1/8#newcode1873
Line 1873: if (pos == last_statement_position_) return;
Another redundant check...

[bak, 2008-09-18 08:10:04]

LGTM,
  Lars

http://codereview.chromium.org/2957/diff/1/7
File src/assembler-arm.h (right):

http://codereview.chromium.org/2957/diff/1/7#newcode647
Line 647: int last_position() const { return last_position_; }
It might be a good idea to introduce a class capturing
(position and is_statement).

http://codereview.chromium.org/2957/diff/1/8
File src/assembler-ia32.cc (right):

http://codereview.chromium.org/2957/diff/1/8#newcode1883
Line 1883: if (last_position_ != kNoPosition &&
Add () in expression.

http://codereview.chromium.org/2957/diff/1/6
File src/debug.cc (right):

http://codereview.chromium.org/2957/diff/1/6#newcode98
Line 98: // passed.
Please update the comment to reflect when position_ is updated.

http://codereview.chromium.org/2957/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/2957/diff/1/3#newcode4150
Line 4150: // Update the position if
Please update and reformat the comment.

http://codereview.chromium.org/2957/diff/1/3#newcode4157
Line 4157: (pc - it.rinfo()->pc()) < distance ||
Add parenthesis to reflect the grouping in boolean expression.

[Søren Thygesen Gjesse, 2008-09-18 08:49:12]

http://codereview.chromium.org/2957/diff/1/7
File src/assembler-arm.h (right):

http://codereview.chromium.org/2957/diff/1/7#newcode647
Line 647: int last_position() const { return last_position_; }
On 2008/09/18 08:10:04, bak wrote:
> It might be a good idea to introduce a class capturing
> (position and is_statement).

I will look at this in another CL.

http://codereview.chromium.org/2957/diff/1/8
File src/assembler-ia32.cc (right):

http://codereview.chromium.org/2957/diff/1/8#newcode1865
Line 1865: if (pos == last_position_) return;
On 2008/09/18 08:02:24, iposva wrote:
> Isn't this check redundant?

Removed - left over from before.

http://codereview.chromium.org/2957/diff/1/8#newcode1873
Line 1873: if (pos == last_statement_position_) return;
On 2008/09/18 08:02:24, iposva wrote:
> Another redundant check...

Removed - another left over from before.

http://codereview.chromium.org/2957/diff/1/8#newcode1883
Line 1883: if (last_position_ != kNoPosition &&
On 2008/09/18 08:10:04, bak wrote:
> Add () in expression.

Done.

http://codereview.chromium.org/2957/diff/1/6
File src/debug.cc (right):

http://codereview.chromium.org/2957/diff/1/6#newcode98
Line 98: // passed.
On 2008/09/18 08:10:04, bak wrote:
> Please update the comment to reflect when position_ is updated.

Updated and changed code to always update position_ as we don't want position_
to be before statement_position_.

http://codereview.chromium.org/2957/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/2957/diff/1/3#newcode4150
Line 4150: // Update the position if
On 2008/09/18 08:10:04, bak wrote:
> Please update and reformat the comment.
> 

Updated the comment with new code below.

http://codereview.chromium.org/2957/diff/1/3#newcode4157
Line 4157: (pc - it.rinfo()->pc()) < distance ||
On 2008/09/18 08:10:04, bak wrote:
> Add parenthesis to reflect the grouping in boolean expression.

Used two if's and some local variables.

    // Only look at positions after the current pc.
    if (it.rinfo()->pc() < pc) {
      // Get position and distance.
      int dist = pc - it.rinfo()->pc();
      int pos = it.rinfo()->data();
      // If this position is closer than the current candidate or if it has the
      // same distance as the current candidate and the position is higher then
      // this position is the new candidate.
      if ((dist < distance) ||
          (dist == distance && pos > position)) {
        position = pos;
        distance = dist;
      }
    }