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),
     ctx_(NULL),
     cur_signal_(-1),
     sig_thread_(0),
-    run_thread_(-1),
-    reg_thread_(-1) {
-  if (NULL == abi_) abi_ = Abi::Get();
+    run_req_(-1),

[Cliff L. Biffle, 2010/12/14 21:46:08]

You appear to be using two magic numbers, 0 and -1, here for thread IDs.  What's
the difference?

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

I have addressed this by creating a new constant, used here and in
"UpdateThreadId" below.  Said function is the result of quickly factoring two
almost identical functions because I got tired of making the same changes twice.

+    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--) {

[Cliff L. Biffle, 2010/12/14 21:46:08]

Excess space before semicolon.

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

Done.

+    // 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)) {

[Cliff L. Biffle, 2010/12/14 21:46:08]

Doing this inside the loop is confusing.  It seems like you could do this check
at the top-level.  But it's possible that I'm confused.

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

I've clarified this code a little bit but the only effect was to make it more
obvious that it never worked, and that the body of the next if-statement makes
no sense.  I wrote an issue that requires this function to be spec'd, isolated,
and tested.

+        // 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.
+  // Wait for an event that indicates that the Run Thread is ready to process
+  // the next signal.
   sig_start_->Wait();
   {
-    // Now lock the target, sleeping all active threads
-    MutexLock lock(mutex_);
+    // Lock the signal state information to prevent a race.
+    MutexLock lock(stateMutex_);
 
-    // 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
+  // Wait for the signal to be fully processed before exiting.
   if (wait) sig_done_->Wait();
 }
 
+void Target::Update() {
+  // Next update the current thread info
+  char tmp[16];

[Cliff L. Biffle, 2010/12/14 21:46:08]

Looks like this will be, at largest, 11 bytes.

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

Done.

+  snprintf(tmp, sizeof(tmp), "QC%x", sig_thread_);

[Cliff L. Biffle, 2010/12/14 21:46:08]

Consider using NACL_ARRAY_SIZE macro here -- it will defend you against the
future possibility of tmp becoming a char* instead of a char[], which will cause
sizeof to behave rather differently.

[Cliff L. Biffle, 2010/12/14 21:46:08]

Incidentally, isn't there a nice C++ way of doing this?

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

Since we have a known max-length and don't want to use magic numbers I've
declared a constant and used that here instead.

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

Implicit conversion between string and char * is being deprecated so if you
start with C-types you're probably best of sticking with them.  And I'm not sure
there's a "nice" way to do anything to char[] or char* since std::copy still
requires you to first allocate memory.  I could use a stringstream to do the
formatting but it certainly wouldn't be less code, it certainly wouldn't be
faster and, since we know the bounds of this buffer, not really safer.

+  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_) {

[Cliff L. Biffle, 2010/12/14 21:46:08]

I see cur_signal_ being set to 0, -1, and actual values.  Below, code says it is
"simulating a signal" by setting it to 0, and then resets it to -1.  Can you
explain more about the lifecycle of this variable?

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

This has never worked.  I have an issue to investigate it and fix it properly,
along with providing tests.  I think we were blocking on a change in actual
signal or platform code that kept the actual break from the debugger from ever
getting here.

+    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
+    // Prevent tight loop so this thread doesn't churn up CPU.
     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 us
       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 memory read of size llll, starting
+      // at address aaaa.
+      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 one or more bytes be overwritten,
+      // starting at a specific address.
+      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);
+
+      if (!port::IPlatform::SetMemory(addr, len, block)) err = FAILED;
+
+      pktOut->AddString("OK");
+      delete[] block;
+      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;
     }
 
-    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