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

runtime/vm/debugger.cc

Index: runtime/vm/debugger.cc
===================================================================
--- runtime/vm/debugger.cc	(revision 38310)
+++ runtime/vm/debugger.cc	(working copy)
@@ -180,7 +180,8 @@
       code_(Code::ZoneHandle(code.raw())),
       function_(Function::ZoneHandle(code.function())),
       token_pos_(-1),
-      desc_rec_(NULL),
+      try_index_(-1),
+      has_desc_rec_(false),
       line_number_(-1),
       column_number_(-1),
       context_level_(-1),
@@ -361,10 +362,12 @@
     GetPcDescriptors();
     PcDescriptors::Iterator iter(pc_desc_, RawPcDescriptors::kAnyKind);
     while (iter.HasNext()) {
-      const RawPcDescriptors::PcDescriptorRec& rec = iter.Next();
+      RawPcDescriptors::PcDescriptorRec rec;
+      iter.NextRec(&rec);

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

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.

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

Follow up CL to solve more efficient iteration.

       if (rec.pc() == pc_) {
-        desc_rec_ = &rec;
+        try_index_ = rec.try_index();
         token_pos_ = rec.token_pos();
+        has_desc_rec_ = true;

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

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.

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

Renamed to 'token_pos_initialized_'

         break;
       }
     }
@@ -374,14 +377,10 @@
 
 
 intptr_t ActivationFrame::TryIndex() {
-  if (desc_rec_ == NULL) {
-    TokenPos();  // Side effect: compute desc_rec_ lazily.
+  if (!has_desc_rec_) {
+    TokenPos();  // Side effect: compute has_desc_rec_, try_index_ etc. lazily.
   }
-  if (desc_rec_ == NULL) {
-    return -1;
-  } else {
-    return desc_rec_->try_index();
-  }
+  return try_index_;
 }
 
 
@@ -430,7 +429,7 @@
     // TODO(hausner): What to do if there is no descriptor entry
     // for the code position of the frame? For now say we are at context
     // level 0.
-    if (desc_rec_ == NULL) {
+    if (!has_desc_rec_) {

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

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.

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

Done.

       return context_level_;
     }
     ASSERT(!pc_desc_.IsNull());
@@ -1204,7 +1203,8 @@
   PcDescriptors& desc = PcDescriptors::Handle(isolate, code.pc_descriptors());
   PcDescriptors::Iterator iter(desc, kSafepointKind);
   while (iter.HasNext()) {
-    const RawPcDescriptors::PcDescriptorRec& rec = iter.Next();
+    RawPcDescriptors::PcDescriptorRec rec;
+    iter.NextRec(&rec);
     if (HasTokenPos(rec)) {
       CodeBreakpoint* bpt = GetCodeBreakpoint(rec.pc());
       if (bpt != NULL) {
@@ -1255,8 +1255,8 @@
       PcDescriptors::Handle(isolate, code.pc_descriptors());
   PcDescriptors::Iterator iter(pc_desc, RawPcDescriptors::kClosureCall);
   while (iter.HasNext()) {
-    const RawPcDescriptors::PcDescriptorRec& rec = iter.Next();
-    if (rec.pc() == pc) {
+    const uword rec_pc = iter.NextPc();
+    if (rec_pc == pc) {
       is_closure_call = true;
       break;
     }
@@ -1547,17 +1547,15 @@
   ASSERT(!code.IsNull());
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   intptr_t best_fit_pos = INT_MAX;
-  const RawPcDescriptors::PcDescriptorRec* best_fit_rec = NULL;
   uword lowest_pc = kUwordMax;
-  const RawPcDescriptors::PcDescriptorRec* lowest_pc_rec = NULL;
-
-  const RawPcDescriptors::PcDescriptorRec* rec = NULL;
+  intptr_t lowest_pc_token_pos = INT_MAX;
   PcDescriptors::Iterator iter(desc, kSafepointKind);
   while (iter.HasNext()) {
-    rec = &iter.Next();
-    intptr_t desc_token_pos = rec->token_pos();
+    RawPcDescriptors::PcDescriptorRec rec;
+    iter.NextRec(&rec);

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

Again, you need to copy the entire pc descriptor (in individual assignments)
just to check the token pos. Not very efficient.

+    intptr_t desc_token_pos = rec.token_pos();
     ASSERT(desc_token_pos >= 0);
-    if (HasTokenPos(*rec)) {
+    if (HasTokenPos(rec)) {
       if ((desc_token_pos < requested_token_pos) ||
           (desc_token_pos > last_token_pos)) {
         // This descriptor is outside the desired token range.
@@ -1567,24 +1565,23 @@
         // So far, this descriptor has the lowest token position after
         // the first acceptable token position.
         best_fit_pos = desc_token_pos;
-        best_fit_rec = rec;
       }
-      if (rec->pc() < lowest_pc) {
+      if (rec.pc() < lowest_pc) {
         // This descriptor so far has the lowest code address.
-        lowest_pc = rec->pc();
-        lowest_pc_rec = rec;
+        lowest_pc = rec.pc();
+        lowest_pc_token_pos = desc_token_pos;
       }
     }
   }
-  if (lowest_pc_rec != NULL) {
+  if (lowest_pc_token_pos != INT_MAX) {
     // We found the pc descriptor that has the lowest execution address.
     // This is the first possible breakpoint after the requested token
     // position. We use this instead of the nearest PC descriptor
     // measured in token index distance.
-    best_fit_rec = lowest_pc_rec;
+    return lowest_pc_token_pos;
   }
-  if (best_fit_rec != NULL) {
-    return best_fit_rec->token_pos();
+  if (best_fit_pos != INT_MAX) {
+    return best_fit_pos;
   }
   // We didn't find a safe point in the given token range. Try and find
   // a safe point in the remaining source code of the function.
@@ -1606,24 +1603,25 @@
   // Find the safe point with the lowest compiled code address
   // that maps to the token position of the source breakpoint.
   PcDescriptors::Iterator iter(desc, kSafepointKind);
-  const RawPcDescriptors::PcDescriptorRec* lowest_rec = NULL;
+  RawPcDescriptors::PcDescriptorRec lowest_rec;
   while (iter.HasNext()) {
-    const RawPcDescriptors::PcDescriptorRec& rec = iter.Next();
+    RawPcDescriptors::PcDescriptorRec rec;
+    iter.NextRec(&rec);
     intptr_t desc_token_pos = rec.token_pos();
     if ((desc_token_pos == bpt->token_pos_) && HasTokenPos(rec)) {
       if (rec.pc() < lowest_pc) {
         lowest_pc = rec.pc();
-        lowest_rec = &rec;
+        lowest_rec = rec;
       }
     }
   }
-  if (lowest_rec == NULL) {
+  if (lowest_pc == kUwordMax) {
     return;
   }
-  CodeBreakpoint* code_bpt = GetCodeBreakpoint(lowest_rec->pc());
+  CodeBreakpoint* code_bpt = GetCodeBreakpoint(lowest_rec.pc());
   if (code_bpt == NULL) {
     // No code breakpoint for this code exists; create one.
-    code_bpt = new CodeBreakpoint(code, *lowest_rec);
+    code_bpt = new CodeBreakpoint(code, lowest_rec);
     RegisterCodeBreakpoint(code_bpt);
   }
   code_bpt->set_src_bpt(bpt);