Fix PcDescriptor iteratot to never return a pointer to a memory location since the data can move wi…

Fix PcDescriptor iterator to never return a pointer to a memory location since the PcDescriptor can move with the GC.

R=hausner@google.com

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

[hausner, 2014-07-16 22:29:38]

There is still a bug, see comment in ContextLevel().

I have to say I really don't like the overall change from iterating over PC
descriptors using an index to using an iterator. Not only is the code more
obscure now, but also rather inefficient. With this change you copy each
descriptor record on each iteration, just to check one field. That's  lot of
unnecessary copying.

At the very least, I hope you can turn this into an iterator that just remembers
the pc descriptor index, and the individual descriptor values then get accessed
through iterator methods. But my preference is to revert this change back to
simply using indices.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:360: if (token_pos_ < 0) {
This should now be if (!has_desc_rec_)

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:366: iter.NextRec(&rec);
Compared to the original version that accessed the pc descriptors directly with
an index, you now copy a lot more data, using individual assignments, just to
see whether the pc matches.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:370: has_desc_rec_ = true;
has_desc_rec_ should be set to true even if no descriptor was found. This way,
you avoid multiple attempts to find a matching pc descriptor if none exists.

In fact, has_desc_rec_ should probably be called pc_desc_initialized_ or
something similar, to reflect whether an attempt was made to find a pc
descriptor for this frame.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:432: if (!has_desc_rec_) {
I think this is still a bug, similar to the one we fixed yesterday. The original
code (prior to https://codereview.chromium.org/356923006) had a call to
PcDescIndex() here, which as a side effect searched the pc descriptor table.
This code no longer has the side effect. You need to call TokenPos() before this
check.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1555: iter.NextRec(&rec);
Again, you need to copy the entire pc descriptor (in individual assignments)
just to check the token pos. Not very efficient.

[srdjan, 2014-07-16 23:06:13]

Will address the performance issue in another CL.
PTAL.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:360: if (token_pos_ < 0) {
On 2014/07/16 22:29:37, hausner wrote:
> This should now be if (!has_desc_rec_)

Done.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:366: iter.NextRec(&rec);
On 2014/07/16 22:29:38, hausner wrote:
> Compared to the original version that accessed the pc descriptors directly
with
> an index, you now copy a lot more data, using individual assignments, just to
> see whether the pc matches.

Follow up CL to solve more efficient iteration.

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:370: has_desc_rec_ = true;
On 2014/07/16 22:29:38, hausner wrote:
> has_desc_rec_ should be set to true even if no descriptor was found. This way,
> you avoid multiple attempts to find a matching pc descriptor if none exists.
> 
> In fact, has_desc_rec_ should probably be called pc_desc_initialized_ or
> something similar, to reflect whether an attempt was made to find a pc
> descriptor for this frame.

Renamed to 'token_pos_initialized_'

https://codereview.chromium.org/396213005/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:432: if (!has_desc_rec_) {
On 2014/07/16 22:29:38, hausner wrote:
> I think this is still a bug, similar to the one we fixed yesterday. The
original
> code (prior to https://codereview.chromium.org/356923006) had a call to
> PcDescIndex() here, which as a side effect searched the pc descriptor table.
> This code no longer has the side effect. You need to call TokenPos() before
this
> check.

Done.

[hausner, 2014-07-16 23:39:57]

LGTM with a couple of nits.

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

https://codereview.chromium.org/396213005/diff/80001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:358: // Compute token_pos_ and pc_desc_index_.
Update comment, pc_desc_index_ does not exist anymore.

https://codereview.chromium.org/396213005/diff/80001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:381: TokenPos();  // Side effect: compute has_desc_rec_,
try_index_ etc. lazily.
Update comment: has_desc_rec_ doesn't exist anymore.

[srdjan, 2014-07-17 18:22:17]

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

https://codereview.chromium.org/396213005/diff/80001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:358: // Compute token_pos_ and pc_desc_index_.
On 2014/07/16 23:39:57, hausner wrote:
> Update comment, pc_desc_index_ does not exist anymore.

Done.

https://codereview.chromium.org/396213005/diff/80001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:381: TokenPos();  // Side effect: compute has_desc_rec_,
try_index_ etc. lazily.
On 2014/07/16 23:39:57, hausner wrote:
> Update comment: has_desc_rec_ doesn't exist anymore.

Done.

[srdjan, 2014-07-17 18:30:44]

Committed patchset #6 manually as r38338 (presubmit successful).