This change contains changes that were made on a separate copy of this code,...

src/trusted/gdb_rsp/target.cc

 /*
  * Copyright 2010 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can
  * be found in the LICENSE file.
  */
 
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
 
@@ skipping 16 matching lines @@
 using port::IMutex;
 using port::IPlatform;
 using port::IThread;
 using port::MutexLock;
 
 namespace gdb_rsp {
 
 
 Target::Target(const Abi* abi)
   : abi_(abi),
-    mutex_(NULL),
+    stateMutex_(NULL),
+    publicMutex_(NULL),
     sig_start_(NULL),
     sig_done_(NULL),
-    send_done_(false),

[noelallen_use_chromium, 2010/12/13 19:28:10]

Why did you remove this?  You have an uninitialized member.  Did you mean to
remove this from the structure as well?

[mlinck, 2010/12/14 17:23:24]

I didn't remove this line.  It had been removed in the version of code I'm
trying to consolidate, but the change was left unfinished.  I removed it from
the struct since it's not used.

     ctx_(NULL),
     cur_signal_(-1),
     sig_thread_(0),
-    run_thread_(-1),
-    reg_thread_(-1) {
-  if (NULL == abi_) abi_ = Abi::Get();
+    run_req_(-1),
+    run_last_(0),
+    reg_req_(-1),
+    reg_last_(0) {
+      if (NULL == abi_) {
+          abi_ = Abi::Get();
+      }
 }
 
 Target::~Target() {
   Destroy();
 }
 
 bool Target::Init() {
+  stateMutex_ = IMutex::Allocate();
+  publicMutex_ = IMutex::Allocate();
+  sig_start_ = IEvent::Allocate();
+  sig_done_ = IEvent::Allocate();
+  ctx_ = new uint8_t[abi_->GetContextSize()];
+
+  if ((NULL == stateMutex_) || (NULL == publicMutex_) || (NULL == sig_start_)
+      || (NULL == sig_done_) || (NULL == ctx_)) {
+    Destroy();
+    return false;
+  }
+
   string targ_xml = "l<target><architecture>";
-
   targ_xml += abi_->GetName();
   targ_xml += "</architecture></target>";
 
   // Set a more specific result which won't change.
-  properties_["target.xml"] = targ_xml;
-  properties_["Supported"] =
-    "PacketSize=7cf;qXfer:libraries:read+;qXfer:features:read+";
-
-  mutex_ = IMutex::Allocate();
-  sig_start_ = IEvent::Allocate();
-  sig_done_ = IEvent::Allocate();
-  ctx_ = new uint8_t[abi_->GetContextSize()];
-
-  if ((NULL == mutex_) || (NULL == sig_start_) || (NULL == sig_done_)
-      || (NULL == ctx_)) {
-    Destroy();
-    return false;
-  }
+  SetProperty("Xfer:features:read:target.xml", targ_xml);
+  SetProperty("Supported",
+    "PacketSize=7cf;qXfer:libraries:read+;qXfer:features:read+");
 
   // Allow one exception to happen
   sig_start_->Signal();
   return true;
 }
 
+bool Target::SetProperty(const std::string& name, const std::string& val) {
+  MutexLock lock(publicMutex_);
+  properties_[name] = val;
+  return true;
+}
+
+bool Target::GetProperty(const std::string& name, std::string* val) const {
+  MutexLock lock(publicMutex_);
+  PropertyMap_t::const_iterator itr = properties_.find(name);
+  if (itr == properties_.end()) return false;
+
+  *val = itr->second;
+  return true;
+}
+
+// MatchQuery looks for the longest prefix match, and returns that value.
+// A match key is in one of two forms:
+//  1 - <keyA>[:<keyB>[:<keyC>[...]]]
+//  2 - <Cmd>:<Type>:read/write:<Object>:<start>,<end>
+// We look for the second case, and assume the first if the format does not
+// match.  The second case is special in that it encodes a substring request.
+// This allows transfers of very large values on a transport with a much
+// smaller max packet size.
+size_t Target::MatchQuery(const std::string& match, std::string* val) const {
+  // Starting at longest match...
+  stringvec parts = StringSplit(match, ":");
+  for (size_t cnt = parts.size(); cnt ; cnt--) {
+    // Append the sub-parts and look for a match.
+    std::string cmp;
+    for (size_t i = 0; i < cnt; i++) {
+      if (i) cmp += ":";
+      cmp += parts[i];
+    }
+
+    // If we find this cached value, return it and the match length
+    if (GetProperty(cmp, val)) {
+      // Check for a "<CMD>:<TYPE>:read:<OBJ>:<start>,<len>
+      if ((cnt == 4) && (parts[3] == "read") && (parts.size() == 5)) {
+        // Split the start,length field
+        stringvec args = StringSplit(parts[4], ",");
+        if (args.size() != 2) return cnt;
+
+        std::string tmp = *val;
+        size_t offs = strtoul(args[0].data(), NULL, 16);
+        size_t max  = strtoul(args[1].data(), NULL, 16) + offs;
+        size_t len = tmp.size();
+
+        // Clear val and only send the requested portion
+        *val = "";
+        if (max >= len) max = len;
+        while (offs < max) {
+          *val += tmp[offs];
+          offs++;
+        }
+      }
+      return cnt;
+    }
+  }
+  return 0;
+}
+
 void Target::Destroy() {
-  if (mutex_) IMutex::Free(mutex_);
+  if (stateMutex_) IMutex::Free(stateMutex_);
+  if (publicMutex_) IMutex::Free(publicMutex_);
   if (sig_start_) IEvent::Free(sig_start_);
   if (sig_done_) IEvent::Free(sig_done_);
 
   delete[] ctx_;
 }
 
 bool Target::AddTemporaryBreakpoint(uint64_t address) {
+  MutexLock lock(publicMutex_);
   const Abi::BPDef *bp = abi_->GetBreakpointDef();
 
   // If this ABI does not support breakpoints then fail
   if (NULL == bp) return false;
 
-  // If we alreay have a breakpoint here then don't add it
+  // If we already have a breakpoint here then don't add it
   BreakMap_t::iterator itr = breakMap_.find(address);
   if (itr != breakMap_.end()) return false;
 
   uint8_t *data = new uint8_t[bp->size_];
   if (NULL == data) return false;
 
   // Copy the old code from here
   if (IPlatform::GetMemory(address, bp->size_, data) == false) {
     delete[] data;
     return false;
   }
   if (IPlatform::SetMemory(address, bp->size_, bp->code_) == false) {
     delete[] data;
     return false;
   }
 
   breakMap_[address] = data;
   return true;
 }
 
 bool Target::RemoveTemporaryBreakpoints() {
+  MutexLock lock(publicMutex_);
   const Abi::BPDef *bp = abi_->GetBreakpointDef();
 
   // Iterate through the map, removing breakpoints
   while (!breakMap_.empty()) {
     // Copy the key/value locally
     BreakMap_t::iterator cur = breakMap_.begin();
     uint64_t addr = cur->first;
     uint8_t *data = cur->second;
-  
+
     // Then remove it from the map
     breakMap_.erase(cur);
 
     // Copy back the old code, and free the data
     IPlatform::SetMemory(addr, bp->size_, data);
     delete[] data;
   }
 
   return true;
 }
 
 
-
 void Target::Signal(uint32_t id, int8_t sig, bool wait) {
   // Wait for this signal's turn in the signal Q.
   sig_start_->Wait();
   {
-    // Now lock the target, sleeping all active threads
-    MutexLock lock(mutex_);
+    // Lock the signal state information to revent a race

[noelallen_use_chromium, 2010/12/13 19:28:10]

prevent

[mlinck, 2010/12/14 17:23:24]

Done.

+    MutexLock lock(stateMutex_);
 

[noelallen_use_chromium, 2010/12/13 19:28:10]

This comment and surrounding code is a bit confusing and probably could use a
better comment.  I believe what is happening here is as follows:

We "wait" for a signal to start ensuring that we are the only debugable thread
processing a signal.  We start with the signal "primed" so that the first signal
can take place without waiting.  We then lock the state to ensure the stub
thread is not currently in it's run look and thus using state data.   Finally we
update the state information, unlock, allowing the stub thread to make progress.

Finally we wait for 'done' which gives us permission to return from the
exception state.

[mlinck, 2010/12/14 17:23:24]

Thanks for bringing this to my attention.  There are two outstanding bugs
against this code that are not in the scope of this CL.  We'll make sure we
comment this code properly once we have something that actually works.

-    // Suspend all threads except this one
-    uint32_t curId;
-    bool more = GetFirstThreadId(&curId);
-    while (more) {
-      if (curId != id) {
-        IThread *thread = threads_[curId];
-        thread->Suspend();
-      }
-      more = GetNextThreadId(&curId);
-    }
-
-    // Signal the stub (Run thread) that we are ready to process
-    // a trap, by updating the signal information and releasing
-    // the lock.
-    reg_thread_ = id;
-    run_thread_ = id;
+    // Signal the stub that we are ready to process a trap
+    // by locking the signal information, and updating it.
+    sig_thread_ = id;
     cur_signal_ = sig;
   }
 
   // Wait for permission to continue
   if (wait) sig_done_->Wait();
 }
 
+void Target::Update() {
+  // Next update the current thread info
+  char tmp[16];
+  snprintf(tmp, sizeof(tmp), "QC%x", sig_thread_);
+  SetProperty("C", tmp);
+}
+
+void Target::Pause() {
+  // put all active threads to sleep.
+  uint32_t curId;
+  bool more = GetFirstThreadId(&curId);
+  while (more) {
+    IThread *thread = GetThread(curId);
+    if (thread->GetState() == IThread::RUNNING) thread->Suspend();
+    more = GetNextThreadId(&curId);
+  }
+}
+
+void Target::Resume() {
+  // Now that we are done, we want to continue in the "correct order".
+  // This means letting the active thread go first, in case we are single
+  // stepping and want to catch it again.  This is a desired behavior but
+  // it is not guaranteed since another thread may already be in an
+  // exception state and next in line to notify the target.
+
+  // Wake up the run thread if it needs it, so it can go first
+  IThread* thread = GetThread(GetRunThreadId());
+  if (thread->GetState() == IThread::SUSPENDED) thread->Resume();
+
+  // If we took an exception, let the handler resume and allow
+  // the next exception to trigger (although it will still
+  // block until we release the mutex).
+  if (cur_signal_) {
+    sig_done_->Signal();
+    sig_start_->Signal();
+  }
+
+  // Now wake up everyone else that needs it
+  uint32_t curId;
+  bool more = GetFirstThreadId(&curId);
+  while (more) {
+    IThread* thread = GetThread(curId);
+
+    // Definitely don't mess with breakpoint state here and let debugger decide
+    // when it's time to turn off breakpoints.
+    if (thread->GetState() == IThread::SUSPENDED) thread->Resume();
+    more = GetNextThreadId(&curId);
+  }
+}
+
 void Target::Run(Session *ses) {
   bool first = true;
+
   do {
     // Give everyone else a chance to use the lock
     IPlatform::Relinquish(100);
 
     // Lock to prevent anyone else from modifying threads
     // or updating the signal information.
-    MutexLock lock(mutex_);
+    MutexLock lock(stateMutex_);
     Packet recv, reply;
 
-    uint32_t id = 0;
-
     // If no signal is waiting for this iteration...
     if (-1 == cur_signal_) {
-      // but the debugger is talking to us then force a break
+      // but the debugger is talking to use

[noelallen_use_chromium, 2010/12/13 19:28:10]

us

[mlinck, 2010/12/14 17:23:24]

Done.

       if (ses->DataAvailable()) {
-        // set signal to 0 to signify paused
+        // Then simulate a signal, by setting the state data
+        // Set signal to 0 to signify paused
         cur_signal_ = 0;
 
-        // put all the threads to sleep.
-        uint32_t curId;
-        bool more = GetFirstThreadId(&curId);
-        while (more) {
-          if (curId != id) {
-            IThread *thread = threads_[curId];
-            thread->Suspend();
-          }
-          more = GetNextThreadId(&curId);
-        }
+        // Set the "signaling" thread to the preferred thread
+        sig_thread_ = GetRunThreadId();
       } else {
-        // otherwise, nothing to do so try again.
+        // otherwise, we are not signaled, and we are not chatting
+        // release the lock, and try again.
         continue;
       }
-    } else {
-      // otherwise there really is an exception so get the id of the thread
-      id = GetRegThreadId();
     }
 
-    // If we got this far, then there is some kind of signal.
-    // So first, remove the breakpoints
-    RemoveTemporaryBreakpoints();
+    // We have a signal, so pause everyone
+    Pause();
 
-    // Next update the current thread info
-    char tmp[16];
-    snprintf(tmp, sizeof(tmp), "QC%x", id);
-    properties_["C"] = tmp;
+    // Update our local state information
+    Update();
 
     if (first) {
-      // First time on a connection, we don't sent the signal
+      // First time on a connection, we don't send the signal
       first = false;
     } else {
       // All other times, send the signal that triggered us
       Packet pktOut;
       pktOut.AddRawChar('S');
       pktOut.AddWord8(cur_signal_);
       ses->SendPacketOnly(&pktOut);
     }
 
     // Now we are ready to process commands
     // Loop through packets until we process a continue
     // packet.
     do {
       if (ses->GetPacket(&recv)) {
         reply.Clear();
         if (ProcessPacket(&recv, &reply)) {
           // If this is a continue command, break out of this loop
           break;
         } else {
-          // Othwerise send the reponse
+          // Otherwise send the reponse
           ses->SendPacket(&reply);
         }
       }
     } while (ses->Connected());
 
+    // Start everyone again.
+    Resume();
 
-    // Now that we are done, we want to continue in the "correct order".
-    // This means letting the active thread go first, in case we are single
-    // stepping and want to catch it again.  This is a desired behavior but
-    // it is not guaranteed since another thread may already be in an
-    // exception state and next in line to notify the target.
+    // Reset our state before we return, clearing the signal.  However
+    // we do not reset run_thread or reg_thread to preserve the last context.
+    cur_signal_ = -1;
+    sig_thread_ = 0;
 
-    // If the run thread is not the exception thread, wake it up now.
-    uint32_t run_thread = GetRunThreadId();
-    if (run_thread != id
-        && run_thread != static_cast<uint32_t>(-1)) {
-      IThread* thread = threads_[run_thread];
-      thread->Resume();
-    }
-
-    // Next, wake up the exception thread, if there is one and it needs
-    // to wake up.
-    if (id && send_done_) sig_done_->Signal();
-
-    // Now wake up everyone else
-    uint32_t curId;
-    bool more = GetFirstThreadId(&curId);
-    while (more) {
-      if ((curId != id) && (curId != GetRunThreadId())) {
-        IThread *thread = threads_[curId];
-        thread->Resume();
-      }
-      more = GetNextThreadId(&curId);
-    }
-
-    // Reset the signal value
-    cur_signal_ = -1;
-
-    // If we took an exception, let the handler resume and allow
-    // the next exception to come in.
-    if (cur_signal_) {
-      sig_done_->Signal();
-      sig_start_->Signal();
-    }
-
+    // At this point, we destroy the MutexLock, allowing anyone else to
+    // and/remove threads or register a new signal.
     // Continue running until the connection is lost.
   } while (ses->Connected());
 }
 
 
-
-
 bool Target::GetFirstThreadId(uint32_t *id) {
   threadItr_ = threads_.begin();
   return GetNextThreadId(id);
 }
 
 bool Target::GetNextThreadId(uint32_t *id) {
   if (threadItr_ == threads_.end()) return false;
 
   *id = (*threadItr_).first;
   threadItr_++;
 
   return true;
 }
 
 
-
 bool Target::ProcessPacket(Packet* pktIn, Packet* pktOut) {
   char cmd;
   int32_t seq = -1;
   ErrDef  err = NONE;
 
   // Clear the outbound message
   pktOut->Clear();
 
   // Pull out the sequence.
   pktIn->GetSequence(&seq);
   if (seq != -1) pktOut->SetSequence(seq);
 
   // Find the command
   pktIn->GetRawChar(&cmd);
 
   switch (cmd) {
+    // In the case of a force break, drop through
+    // and print the signal.  We have already suspended
+    // the threads at this point.
+    case 0x03:
+
     // IN : $?
     // OUT: $Sxx
     case '?':
+      // This case asks why the target has halted.
       pktOut->AddRawChar('S');
       pktOut->AddWord8(cur_signal_);
       break;
 
     // IN : $d
     // OUT: -NONE-
     case 'd':
+      // This packet could be asking the debugger to detach but
+      // we should never receive it.
       Detach();
-      break;
+      return true;
 
     // IN : $g
     // OUT: $xx...xx
     case 'g': {
+      // This packet is requesting the value of a general register.
       uint32_t id = GetRegThreadId();
       if (0 == id) {
         err = BAD_ARGS;
         break;
       }
 
       IThread *thread = GetThread(id);
       if (NULL == thread) {
         err = BAD_ARGS;
         break;
       }
 
       // Copy OS preserved registers to GDB payload
       for (uint32_t a = 0; a < abi_->GetRegisterCount(); a++) {
         const Abi::RegDef *def = abi_->GetRegisterDef(a);
         thread->GetRegister(a, &ctx_[def->offset_], def->bytes_);
       }
 
       pktOut->AddBlock(ctx_, abi_->GetContextSize());
       break;
     }
 
     // IN : $Gxx..xx
     // OUT: $OK
     case 'G': {
+      // This packet is requesting that the value of a general register be
+      // changed to something else.
       uint32_t id = GetRegThreadId();
       if (0 == id) {
         err = BAD_ARGS;
         break;
       }
 
       IThread *thread = threads_[id];
       if (NULL == thread) {
         err = BAD_ARGS;
         break;
       }
 
       // GDB payload to OS registers
       for (uint32_t a = 0; a < abi_->GetRegisterCount(); a++) {
         const Abi::RegDef *def = abi_->GetRegisterDef(a);
         thread->SetRegister(a, &ctx_[def->offset_], def->bytes_);
       }
       pktOut->AddBlock(ctx_, abi_->GetContextSize());
       break;
     }
 
     // IN : $H(c/g)(-1,0,xxxx)
     // OUT: $OK
     case 'H': {
-        char type;
-        uint64_t id;
+      // This packet is requesting that the thread be set for subsequent
+      // operations.
+      char type;
+      uint64_t id;
 
-        if (!pktIn->GetRawChar(&type)) {
-          err = BAD_FORMAT;
+      if (!pktIn->GetRawChar(&type)) {
+        err = BAD_FORMAT;
+        break;
+      }
+      if (!pktIn->GetNumberSep(&id, 0)) {
+        err = BAD_FORMAT;
+        break;
+      }
+
+      if (threads_.begin() == threads_.end()) {
+        err = BAD_ARGS;
+        break;
+      }
+
+      // If we are using "any" get the first thread
+      if (id == static_cast<uint64_t>(-1)) id = threads_.begin()->first;
+
+      // Verify that we have the thread
+      if (threads_.find(static_cast<uint32_t>(id)) == threads_.end()) {
+        err = BAD_ARGS;
+        break;
+      }
+
+      pktOut->AddString("OK");
+      switch (type) {
+        case 'g':
+          reg_req_ = static_cast<uint32_t>(id);
           break;
-        }
-        if (!pktIn->GetNumberSep(&id, 0)) {
-          err = BAD_FORMAT;
+
+        case 'c':
+          run_req_ = static_cast<uint32_t>(id);
           break;
-        }
 
-        if (threads_.begin() == threads_.end()) {
-            err = BAD_ARGS;
-            break;
-        }
-
-        // If we are using "any" get the first thread
-        if (id == static_cast<uint64_t>(-1)) id = threads_.begin()->first;
-
-        // Verify that we have the thread
-        if (threads_.find(static_cast<uint32_t>(id)) == threads_.end()) {
+        default:
           err = BAD_ARGS;
           break;
         }
+      break;
+    }
 
-        pktOut->AddString("OK");
-        switch (type) {
-          case 'g':
-            reg_thread_ = static_cast<uint32_t>(id);
-            break;
-
-          case 'c':
-            run_thread_ = static_cast<uint32_t>(id);
-            break;
-
-          default:
-            err = BAD_ARGS;
-            break;
-        }
-        break;
-      }
+    case 'k':
+      port::IPlatform::LogError("Process killed by debugger.");
+      break;
 
     // IN : $maaaa,llll
     // OUT: $xx..xx
     case 'm': {
-        uint64_t addr;
-        uint64_t wlen;
-        uint32_t len;
-        if (!pktIn->GetNumberSep(&addr, 0)) {
-          err = BAD_FORMAT;
-          break;
-        }
-
-        if (!pktIn->GetNumberSep(&wlen, 0)) {
-          err = BAD_FORMAT;
-          break;
-        }
-
-        len = static_cast<uint32_t>(wlen);
-        uint8_t *block = new uint8_t[len];
-        if (!port::IPlatform::GetMemory(addr, len, block)) err = FAILED;
-
-        pktOut->AddBlock(block, len);
+      // This packet is requesting a read operation for more than one byte.

[noelallen_use_chromium, 2010/12/13 19:28:10]

Why more than one byte?  It should be legal for 'llll' to be 1.

[mlinck, 2010/12/14 17:23:24]

Done.

+      uint64_t addr;
+      uint64_t wlen;
+      uint32_t len;
+      if (!pktIn->GetNumberSep(&addr, 0)) {
+        err = BAD_FORMAT;
         break;
       }
 
+      if (!pktIn->GetNumberSep(&wlen, 0)) {
+        err = BAD_FORMAT;
+        break;
+      }
+
+      len = static_cast<uint32_t>(wlen);
+      uint8_t *block = new uint8_t[len];
+      if (!port::IPlatform::GetMemory(addr, len, block)) err = FAILED;
+
+      pktOut->AddBlock(block, len);
+      break;
+    }
+
     // IN : $Maaaa,llll:xx..xx
     // OUT: $OK
     case 'M':  {
-        uint64_t addr;
-        uint64_t wlen;
-        uint32_t len;
+      // This packet is requesting that multiple bytes be overwritten,
+      // starting at a specific address.

[noelallen_use_chromium, 2010/12/13 19:28:10]

one or more

[mlinck, 2010/12/14 17:23:24]

Done.

+      uint64_t addr;
+      uint64_t wlen;
+      uint32_t len;
 
-        if (!pktIn->GetNumberSep(&addr, 0)) {
-          err = BAD_FORMAT;
-          break;
-        }
-        if (!pktIn->GetNumberSep(&wlen, 0)) {
-          err = BAD_FORMAT;
-          break;
-        }
-
-        len = static_cast<uint32_t>(wlen);
-        uint8_t *block = new uint8_t[len];
-        pktIn->GetBlock(block, len);
-
-        if (!port::IPlatform::SetMemory(addr, len, block)) err = FAILED;
-
-        pktOut->AddString("OK");
+      if (!pktIn->GetNumberSep(&addr, 0)) {
+        err = BAD_FORMAT;
+        break;
+      }
+      if (!pktIn->GetNumberSep(&wlen, 0)) {
+        err = BAD_FORMAT;
         break;
       }
 
+      len = static_cast<uint32_t>(wlen);
+      uint8_t *block = new uint8_t[len];
+      pktIn->GetBlock(block, len);

[noelallen_use_chromium, 2010/12/13 19:28:10]

Where is the delete[]?  Is this a leak?

[mlinck, 2010/12/14 17:23:24]

Done.

+
+      if (!port::IPlatform::SetMemory(addr, len, block)) err = FAILED;
+
+      pktOut->AddString("OK");
+      break;
+    }
+
     case 'q': {
+      // This packet is carrying out a general query, possibly for supported
+      // commands and the like.
       string tmp;
       const char *str = &pktIn->GetPayload()[1];
-      stringvec toks = StringSplit(str, ":;");
-      PropertyMap_t::const_iterator itr = properties_.find(toks[0]);
+      stringvec toks = StringSplit(str, ";");
+      std::string strout;
+
+      // Check if we can find this query in the local cache
+      if (MatchQuery(toks[0], &strout)) {
+        pktOut->AddString(strout.data());
+        break;
+      }
 
       // If this is a thread query
       if (!strcmp(str, "fThreadInfo") || !strcmp(str, "sThreadInfo")) {
         uint32_t curr;
         bool more = false;
         if (str[0] == 'f') {
           more = GetFirstThreadId(&curr);
         } else {
           more = GetNextThreadId(&curr);
         }
 
         if (!more) {
           pktOut->AddString("l");
         } else {
           pktOut->AddString("m");
           pktOut->AddNumberSep(curr, 0);
         }
         break;
       }
 
-      // Check for architecture query
-      tmp = "Xfer:features:read:target.xml";
-      if (!strncmp(str, tmp.data(), tmp.length())) {
-        stringvec args = StringSplit(&str[tmp.length()+1], ",");
-        if (args.size() != 2) break;
-
-        const char *out = properties_["target.xml"].data();
-        int offs = strtol(args[0].data(), NULL, 16);
-        int max  = strtol(args[1].data(), NULL, 16) + offs;
-        int len  = static_cast<int>(strlen(out));
-
-        if (max >= len) max = len;
-
-        while (offs < max) {
-          pktOut->AddRawChar(out[offs]);
-          offs++;
-        }
-        break;
-      }
-
-      // Check the property cache
-      if (itr != properties_.end()) {
-        pktOut->AddString(itr->second.data());
-      }
+      // Didn't find anything
       break;
     }
 
     case 'T': {
+      // This packet constitutes a check, whether a given thread is alive or
+      // dead.
       uint64_t id;
       if (!pktIn->GetNumberSep(&id, 0)) {
         err = BAD_FORMAT;
         break;
       }
 
       if (GetThread(static_cast<uint32_t>(id)) == NULL) {
         err = BAD_ARGS;
         break;
       }
 
       pktOut->AddString("OK");
       break;
     }
 
     case 's':  {
+      // This packet is asking for a thread to step a single instruction
       IThread *thread = GetThread(GetRunThreadId());
       if (thread) thread->SetStep(true);
       return true;

[noelallen_use_chromium, 2010/12/13 19:28:10]

Where is SetStep(false)?  Make sure we turn off single step somewhere.  Probably
should always turn in off in the signal handler.

[mlinck, 2010/12/14 17:23:24]

Well, there was a massive bug associated SetStep(false) because it caused the
application to no longer stop at breakpoints.  Stepping needs to be revisited
and implemented in a way that doesn't break breakpoint behavior.  There are
outstanding issues that will need to resolve step behavior.

[noelallen_use_chromium, 2010/12/14 20:18:41]

That seems incorrect.  This function should be turning on or off the STEP flag
on the CPU.  If on, this thread will report a sig 5 every instruction.  Turning
HW step on and off does not affect breakpoints.


On 2010/12/14 17:23:24, mlinck wrote:

[mlinck, 2010/12/16 20:03:01]

That is false.  I haven't looked closely at how step was implemented because it
is beyond the scope of this CL (seeing how there are two outstanding tickets to
address it separetely) but I know that the "resume" function used to result in a
call to setStep(false) and I verified by testing that that line was responsible
for the debug_stub ignoring breakpoints after resume and never breaking again. 
I had to remove the line in order to fix breakpoint behavior.  What I want here
as a baseline is code that I know does breakpoints, and tickets to fix all the
other stuff that never worked properly in an organized fashion.

Here's the version of the function that was clearing breakpoints.  Resume() line
268 was the problem.
http://code.google.com/p/nativeclient-vsx/source/browse/third_party/nacl_sub/...

     }
 
-    case 'c':
-      return true;
-
-    default: {
-      // If the command is not recognzied, ignore it by sending an
-      // empty reply.
-      string str;
-      pktIn->GetString(&str);
-      port::IPlatform::LogError("Unknown command: %s", str.data());
-      return false;
-    }
+    default:
+      // If the command is not recognized, ignore it by sending an empty reply.
+      // Don't use LogError here or the debug stub will crash instead of
+      // negotiating a protocol with the debugger.
+      port::IPlatform::LogInfo("Unknown command: %s", pktIn->GetPayload());
+      break;
   }
 
   // If there is an error, return the error code instead of a payload
   if (err) {
     pktOut->Clear();
     pktOut->AddRawChar('E');
     pktOut->AddWord8(err);
   }
   return false;
 }
 
 
 void Target::TrackThread(IThread* thread) {
+  MutexLock lock(stateMutex_);
+
   uint32_t id = thread->GetId();
-  mutex_->Lock();
   threads_[id] = thread;
-  mutex_->Unlock();
 }
 
 void Target::IgnoreThread(IThread* thread) {
+  MutexLock lock(stateMutex_);
+
   uint32_t id = thread->GetId();
-  mutex_->Lock();
   ThreadMap_t::iterator itr = threads_.find(id);
-
   if (itr != threads_.end()) threads_.erase(itr);
-  mutex_->Lock();
 }
 
 
 void Target::Detach() {
   port::IPlatform::LogInfo("Requested Detach.\n");
 }
 
 
 uint32_t Target::GetRegThreadId() const {
-  ThreadMap_t::const_iterator itr;
+  IThread* thread;
+  uint32_t id = 0;
 
-  switch (reg_thread_) {
-    // If we wany "any" then try the signal'd thread first
+  switch (reg_req_) {
     case 0:
     case 0xFFFFFFFF:
-      itr = threads_.begin();
+      // If we wany "any" then try the last result first
+      thread = GetThread(reg_last_);
+
+      // If not there, try the sig_thread next
+      if (NULL == thread) thread = GetThread(sig_thread_);
+
+      // If stil not there, get the first thread
+      if (NULL == thread) thread = threads_.begin()->second;
       break;
 
     default:
-      itr = threads_.find(reg_thread_);
+      // Otherwise, get the specificly requested one
+      thread = GetThread(reg_req_);
       break;
   }
 
-  if (itr == threads_.end()) return 0;
+  // Update last result.
+  if (NULL == thread) {
+    reg_last_ = 0;
+  } else {
+    reg_last_ = thread->GetId();
+  }
 
-  return itr->first;
+  // Return the result
+  return reg_last_;
 }
 
 uint32_t Target::GetRunThreadId() const {
-  return run_thread_;
+  IThread* thread;
+  uint32_t id = 0;
+
+  switch (run_req_) {
+    case 0:
+    case 0xFFFFFFFF:
+      // If we wany "any" then try the last result first
+      thread = GetThread(run_last_);
+
+      // If not there, try the sig_thread next
+      if (NULL == thread) thread = GetThread(sig_thread_);
+
+      // If stil not there, get the first thread
+      if (NULL == thread) thread = threads_.begin()->second;
+      break;
+
+    default:
+      // Otherwise, get the specificly requested one
+      thread = GetThread(run_req_);
+      break;
+  }
+
+  // Update last result.
+  if (NULL == thread) {
+    run_last_ = 0;
+  } else {
+    run_last_ = thread->GetId();
+  }
+
+  // Return the result
+  return run_last_;
 }
 
-IThread* Target::GetThread(uint32_t id) {
+IThread* Target::GetThread(uint32_t id) const {
   ThreadMap_t::const_iterator itr;
   itr = threads_.find(id);
   if (itr != threads_.end()) return itr->second;
 
   return NULL;
 }
 
 
 }  // namespace gdb_rsp