Wasm debugging with LLDB: send and receive GDB-remote packets

This changelist adds the logic to format, decode, sends and receive packets in
the format specified by the GDB-remote protocol
(https://sourceware.org/gdb/onlinedocs/gdb/Overview.html#Overview).

Build with: v8_enable_wasm_gdb_remote_debugging = true
Run with: --wasm-gdb-remote

Bug: chromium:1010467
Change-Id: Ibc9c6713c561d06847b472fab591c208c193199f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1929409
Commit-Queue: Paolo Severini <paolosev@microsoft.com>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67196}

BUILD.gn

@@ -3119,17 +3119,20 @@ v8_source_set("v8_base_without_compiler") {
 
   if (v8_enable_wasm_gdb_remote_debugging) {
     sources += [
+      "src/debug/wasm/gdb-server/gdb-remote-util.cc",
+      "src/debug/wasm/gdb-server/gdb-remote-util.h",
       "src/debug/wasm/gdb-server/gdb-server-thread.cc",
       "src/debug/wasm/gdb-server/gdb-server-thread.h",
       "src/debug/wasm/gdb-server/gdb-server.cc",
       "src/debug/wasm/gdb-server/gdb-server.h",
+      "src/debug/wasm/gdb-server/packet.cc",
+      "src/debug/wasm/gdb-server/packet.h",
       "src/debug/wasm/gdb-server/session.cc",
       "src/debug/wasm/gdb-server/session.h",
       "src/debug/wasm/gdb-server/target.cc",
       "src/debug/wasm/gdb-server/target.h",
       "src/debug/wasm/gdb-server/transport.cc",
       "src/debug/wasm/gdb-server/transport.h",
-      "src/debug/wasm/gdb-server/util.h",
     ]
   }
 

src/debug/wasm/gdb-server/OWNERS

+paolosev@microsoft.com
+
+# COMPONENT: Blink>JavaScript>WebAssembly
\ No newline at end of file

src/debug/wasm/gdb-server/gdb-remote-util.cc

+// Copyright 2020 the V8 project 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 "src/debug/wasm/gdb-server/gdb-remote-util.h"
+using std::string;
+
+namespace v8 {
+namespace internal {
+namespace wasm {
+namespace gdb_server {
+
+// GDB expects lower case values.
+static const char kHexChars[] = "0123456789abcdef";
+
+void UInt8ToHex(uint8_t byte, char chars[2]) {
+  DCHECK(chars);
+  chars[0] = kHexChars[byte >> 4];
+  chars[1] = kHexChars[byte & 0xF];
+}
+
+bool HexToUInt8(const char chars[2], uint8_t* byte) {
+  uint8_t o1, o2;
+  if (NibbleToUInt8(chars[0], &o1) && NibbleToUInt8(chars[1], &o2)) {
+    *byte = (o1 << 4) + o2;
+    return true;
+  }
+
+  return false;
+}
+
+bool NibbleToUInt8(char ch, uint8_t* byte) {
+  DCHECK(byte);
+
+  // Check for nibble of a-f
+  if ((ch >= 'a') && (ch <= 'f')) {
+    *byte = (ch - 'a' + 10);
+    return true;
+  }
+
+  // Check for nibble of A-F
+  if ((ch >= 'A') && (ch <= 'F')) {
+    *byte = (ch - 'A' + 10);
+    return true;
+  }
+
+  // Check for nibble of 0-9
+  if ((ch >= '0') && (ch <= '9')) {
+    *byte = (ch - '0');
+    return true;
+  }
+
+  // Not a valid nibble representation
+  return false;
+}
+
+std::string Mem2Hex(const uint8_t* mem, size_t count) {
+  std::vector<char> result(count * 2 + 1);
+  for (size_t i = 0; i < count; i++) UInt8ToHex(*mem++, &result[i * 2]);
+  result[count * 2] = '\0';
+  return result.data();
+}
+
+std::string Mem2Hex(const std::string& str) {
+  return Mem2Hex(reinterpret_cast<const uint8_t*>(str.data()), str.size());
+}
+
+}  // namespace gdb_server
+}  // namespace wasm
+}  // namespace internal
+}  // namespace v8

src/debug/wasm/gdb-server/util.h → src/debug/wasm/gdb-server/gdb-remote-util.h

@@ -2,10 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef V8_DEBUG_WASM_GDB_SERVER_UTIL_H_
-#define V8_DEBUG_WASM_GDB_SERVER_UTIL_H_
+#ifndef V8_DEBUG_WASM_GDB_SERVER_GDB_REMOTE_UTIL_H_
+#define V8_DEBUG_WASM_GDB_SERVER_GDB_REMOTE_UTIL_H_
 
 #include <string>
+#include <vector>
 #include "src/flags/flags.h"
 #include "src/utils/utils.h"
 
@@ -19,9 +20,25 @@ namespace gdb_server {
     if (FLAG_trace_wasm_gdb_remote) PrintF("[gdb-remote] " __VA_ARGS__); \
   } while (false)
 
+// Convert from 0-255 to a pair of ASCII chars (0-9,a-f).
+void UInt8ToHex(uint8_t byte, char chars[2]);
+
+// Convert a pair of hex chars into a value 0-255 or return false if either
+// input character is not a valid nibble.
+bool HexToUInt8(const char chars[2], uint8_t* byte);
+
+// Convert from ASCII (0-9,a-f,A-F) to 4b unsigned or return false if the
+// input char is unexpected.
+bool NibbleToUInt8(char ch, uint8_t* byte);
+
+// Convert the memory pointed to by {mem} into a hex string in GDB-remote
+// format.
+std::string Mem2Hex(const uint8_t* mem, size_t count);
+std::string Mem2Hex(const std::string& str);
+
 }  // namespace gdb_server
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8
 
-#endif  // V8_DEBUG_WASM_GDB_SERVER_UTIL_H_
+#endif  // V8_DEBUG_WASM_GDB_SERVER_GDB_REMOTE_UTIL_H_

src/debug/wasm/gdb-server/gdb-server-thread.cc

@@ -32,7 +32,7 @@ bool GdbServerThread::StartAndInitialize() {
   // this operation happensat most once per process and only when the
   // --wasm-gdb-remote flag is set.
   start_semaphore_.Wait();
-  return true;
+  return !!target_;
 }
 
 void GdbServerThread::CleanupThread() {

src/debug/wasm/gdb-server/gdb-server-thread.h

@@ -47,7 +47,7 @@ class GdbServerThread : public v8::base::Thread {
 
   base::Mutex mutex_;
   // Protected by {mutex_}:
-  std::unique_ptr<Transport> transport_;
+  std::unique_ptr<TransportBase> transport_;
   std::unique_ptr<Target> target_;
 
   DISALLOW_COPY_AND_ASSIGN(GdbServerThread);

src/debug/wasm/gdb-server/gdb-server.cc

@@ -12,17 +12,18 @@ namespace internal {
 namespace wasm {
 namespace gdb_server {
 
-GdbServer::GdbServer() {
-  DCHECK(!thread_);
+// static
+std::unique_ptr<GdbServer> GdbServer::Create() {
   DCHECK(FLAG_wasm_gdb_remote);
 
-  thread_ = std::make_unique<GdbServerThread>(this);
-  // TODO(paolosev): does StartSynchronously hurt performances?
-  if (!thread_->StartAndInitialize()) {
+  std::unique_ptr<GdbServer> gdb_server(new GdbServer());
+  gdb_server->thread_ = std::make_unique<GdbServerThread>(gdb_server.get());
+  if (!gdb_server->thread_->StartAndInitialize()) {
     TRACE_GDB_REMOTE(
         "Cannot initialize thread, GDB-remote debugging will be disabled.\n");
-    thread_ = nullptr;
+    return nullptr;
   }
+  return gdb_server;
 }
 
 GdbServer::~GdbServer() {

src/debug/wasm/gdb-server/gdb-server.h

@@ -20,16 +20,20 @@ namespace gdb_server {
 // the Wasm engine.
 class GdbServer {
  public:
-  // Spawns a "GDB-remote" thread that will be used to communicate with the
-  // debugger. This should be called once, the first time a Wasm module is
-  // loaded in the Wasm engine.
-  GdbServer();
+  // Factory method: creates and returns a GdbServer. Spawns a "GDB-remote"
+  // thread that will be used to communicate with the debugger.
+  // May return null on failure.
+  // This should be called once, the first time a Wasm module is loaded in the
+  // Wasm engine.
+  static std::unique_ptr<GdbServer> Create();
 
   // Stops the "GDB-remote" thread and waits for it to complete. This should be
   // called once, when the Wasm engine shuts down.
   ~GdbServer();
 
  private:
+  GdbServer() {}
+
   std::unique_ptr<GdbServerThread> thread_;
 
   DISALLOW_COPY_AND_ASSIGN(GdbServer);

src/debug/wasm/gdb-server/packet.cc

+// Copyright 2020 the V8 project 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 "src/debug/wasm/gdb-server/packet.h"
+#include "src/debug/wasm/gdb-server/gdb-remote-util.h"
+
+namespace v8 {
+namespace internal {
+namespace wasm {
+namespace gdb_server {
+
+Packet::Packet() {
+  seq_ = -1;
+  Clear();
+}
+
+void Packet::Clear() {
+  data_.clear();
+  read_index_ = 0;
+}
+
+void Packet::Rewind() { read_index_ = 0; }
+
+bool Packet::EndOfPacket() const { return (read_index_ >= GetPayloadSize()); }
+
+void Packet::AddRawChar(char ch) { data_.push_back(ch); }
+
+void Packet::AddWord8(uint8_t byte) {
+  char seq[2];
+  UInt8ToHex(byte, seq);
+  AddRawChar(seq[0]);
+  AddRawChar(seq[1]);
+}
+
+void Packet::AddBlock(const void* ptr, uint32_t len) {
+  DCHECK(ptr);
+
+  const char* p = (const char*)ptr;
+
+  for (uint32_t offs = 0; offs < len; offs++) {
+    AddWord8(p[offs]);
+  }
+}
+
+void Packet::AddHexString(const char* str) {
+  DCHECK(str);
+
+  while (*str) {
+    AddWord8(*str);
+    str++;
+  }
+}
+
+void Packet::AddNumberSep(uint64_t val, char sep) {
+  char out[sizeof(val) * 2];
+  char temp[2];
+
+  // Check for -1 optimization
+  if (val == static_cast<uint64_t>(-1)) {
+    AddRawChar('-');
+    AddRawChar('1');
+  } else {
+    int nibbles = 0;
+
+    // In the GDB remote protocol numbers are formatted as big-endian hex
+    // strings. Leading zeros can be skipped.
+    // For example the value 0x00001234 is formatted as "1234".
+    for (size_t a = 0; a < sizeof(val); a++) {
+      uint8_t byte = static_cast<uint8_t>(val & 0xFF);
+
+      // Stream in with bytes reversed, starting with the least significant.
+      // So if we have the value 0x00001234, we store 4, then 3, 2, 1.
+      // Note that the characters are later reversed to be in big-endian order.
+      UInt8ToHex(byte, temp);
+      out[nibbles++] = temp[1];
+      out[nibbles++] = temp[0];
+
+      // Get the next 8 bits;
+      val >>= 8;
+
+      // Suppress leading zeros, so we are done when val hits zero
+      if (val == 0) {
+        break;
+      }
+    }
+
+    // Strip the high zero for this byte if present.
+    if ((nibbles > 1) && (out[nibbles - 1] == '0')) nibbles--;
+
+    // Now write it out reverse to correct the order
+    while (nibbles) {
+      nibbles--;
+      AddRawChar(out[nibbles]);
+    }
+  }
+
+  // If we asked for a separator, insert it
+  if (sep) AddRawChar(sep);
+}
+
+bool Packet::GetNumberSep(uint64_t* val, char* sep) {
+  uint64_t out = 0;
+  char ch;
+  if (!GetRawChar(&ch)) {
+    return false;
+  }
+
+  // Numbers are formatted as a big-endian hex strings.
+  // The literals "0" and "-1" as special cases.
+
+  // Check for -1
+  if (ch == '-') {
+    if (!GetRawChar(&ch)) {
+      return false;
+    }
+
+    if (ch == '1') {
+      *val = (uint64_t)-1;
+
+      ch = 0;
+      GetRawChar(&ch);
+      if (sep) {
+        *sep = ch;
+      }
+      return true;
+    }
+    return false;
+  }
+
+  do {
+    uint8_t nib;
+
+    // Check for separator
+    if (!NibbleToUInt8(ch, &nib)) {
+      break;
+    }
+
+    // Add this nibble.
+    out = (out << 4) + nib;
+
+    // Get the next character (if availible)
+    ch = 0;
+    if (!GetRawChar(&ch)) {
+      break;
+    }
+  } while (1);
+
+  // Set the value;
+  *val = out;
+
+  // Add the separator if the user wants it...
+  if (sep != nullptr) *sep = ch;
+
+  return true;
+}
+
+bool Packet::GetRawChar(char* ch) {
+  DCHECK(ch != nullptr);
+
+  if (read_index_ >= GetPayloadSize()) return false;
+
+  *ch = data_[read_index_++];
+
+  // Check for RLE X*N, where X is the value, N is the reps.
+  if (*ch == '*') {
+    if (read_index_ < 2) {
+      TRACE_GDB_REMOTE("Unexpected RLE at start of packet.\n");
+      return false;
+    }
+
+    if (read_index_ >= GetPayloadSize()) {
+      TRACE_GDB_REMOTE("Unexpected EoP during RLE.\n");
+      return false;
+    }
+
+    // GDB does not use "CTRL" characters in the stream, so the
+    // number of reps is encoded as the ASCII value beyond 28
+    // (which when you add a min rep size of 4, forces the rep
+    // character to be ' ' (32) or greater).
+    int32_t cnt = (data_[read_index_] - 28);
+    if (cnt < 3) {
+      TRACE_GDB_REMOTE("Unexpected RLE length.\n");
+      return false;
+    }
+
+    // We have just read '*' and incremented the read pointer,
+    // so here is the old state, and expected new state.
+    //
+    //   Assume N = 5, we grow by N - size of encoding (3).
+    //
+    // OldP:       R  W
+    // OldD:  012X*N89 = 8 chars
+    // Size:  012X*N89__ = 10 chars
+    // Move:  012X*__N89 = 10 chars
+    // Fill:  012XXXXX89 = 10 chars
+    // NewP:       R    W  (shifted 5 - 3)
+
+    // First, store the remaining characters to the right into a temp string.
+    std::string right = data_.substr(read_index_ + 1);
+    // Discard the '*' we just read
+    data_.erase(read_index_ - 1);
+    // Append (N-1) 'X' chars
+    *ch = data_[read_index_ - 2];
+    data_.append(cnt - 1, *ch);
+    // Finally, append the remaining characters
+    data_.append(right);
+  }
+  return true;
+}
+
+bool Packet::GetWord8(uint8_t* value) {
+  DCHECK(value);
+
+  // Get two ASCII hex values and convert them to ints
+  char seq[2];
+  if (!GetRawChar(&seq[0]) || !GetRawChar(&seq[1])) {
+    return false;
+  }
+  return HexToUInt8(seq, value);
+}
+
+bool Packet::GetBlock(void* ptr, uint32_t len) {
+  DCHECK(ptr);
+
+  uint8_t* p = reinterpret_cast<uint8_t*>(ptr);
+  bool res = true;
+
+  for (uint32_t offs = 0; offs < len; offs++) {
+    res = GetWord8(&p[offs]);
+    if (false == res) {
+      break;
+    }
+  }
+
+  return res;
+}
+
+bool Packet::GetString(std::string* str) {
+  if (EndOfPacket()) {
+    return false;
+  }
+
+  *str = data_.substr(read_index_);
+  read_index_ = GetPayloadSize();
+  return true;
+}
+
+bool Packet::GetHexString(std::string* str) {
+  // Decode a string encoded as a series of 2-hex digit pairs.
+
+  if (EndOfPacket()) {
+    return false;
+  }
+
+  // Pull values until we hit a separator
+  str->clear();
+  char ch1;
+  while (GetRawChar(&ch1)) {
+    uint8_t nib1;
+    if (!NibbleToUInt8(ch1, &nib1)) {
+      read_index_--;
+      break;
+    }
+    char ch2;
+    uint8_t nib2;
+    if (!GetRawChar(&ch2) || !NibbleToUInt8(ch2, &nib2)) {
+      return false;
+    }
+    *str += static_cast<char>((nib1 << 4) + nib2);
+  }
+  return true;
+}
+
+const char* Packet::GetPayload() const { return data_.c_str(); }
+
+size_t Packet::GetPayloadSize() const { return data_.size(); }
+
+bool Packet::GetSequence(int32_t* ch) const {
+  DCHECK(ch);
+
+  if (seq_ != -1) {
+    *ch = seq_;
+    return true;
+  }
+
+  return false;
+}
+
+void Packet::ParseSequence() {
+  size_t saved_read_index = read_index_;
+  unsigned char seq;
+  char ch;
+  if (GetWord8(&seq) && GetRawChar(&ch)) {
+    if (ch == ':') {
+      SetSequence(seq);
+      return;
+    }
+  }
+  // No sequence number present, so reset to original position.
+  read_index_ = saved_read_index;
+}
+
+void Packet::SetSequence(int32_t val) { seq_ = val; }
+
+void Packet::SetError(ErrDef error) {
+  Clear();
+  AddRawChar('E');
+  AddWord8(static_cast<uint8_t>(error));
+}
+
+std::string Packet::GetPacketData() const {
+  char chars[2];
+  const char* ptr = GetPayload();
+  size_t size = GetPayloadSize();
+
+  std::stringstream outstr;
+
+  // Signal start of response
+  outstr << '$';
+
+  char run_xsum = 0;
+
+  // If there is a sequence, send as two nibble 8bit value + ':'
+  int32_t seq;
+  if (GetSequence(&seq)) {
+    UInt8ToHex(seq, chars);
+    outstr << chars[0];
+    run_xsum += chars[0];
+    outstr << chars[1];
+    run_xsum += chars[1];
+
+    outstr << ':';
+    run_xsum += ':';
+  }
+
+  // Send the main payload
+  for (size_t offs = 0; offs < size; ++offs) {
+    outstr << ptr[offs];
+    run_xsum += ptr[offs];
+  }
+
+  // Send XSUM as two nibble 8bit value preceeded by '#'
+  outstr << '#';
+  UInt8ToHex(run_xsum, chars);
+  outstr << chars[0];
+  outstr << chars[1];
+
+  return outstr.str();
+}
+
+}  // namespace gdb_server
+}  // namespace wasm
+}  // namespace internal
+}  // namespace v8

src/debug/wasm/gdb-server/packet.h

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_DEBUG_WASM_GDB_SERVER_PACKET_H_
+#define V8_DEBUG_WASM_GDB_SERVER_PACKET_H_
+
+#include <string>
+#include <vector>
+#include "src/base/macros.h"
+
+namespace v8 {
+namespace internal {
+namespace wasm {
+namespace gdb_server {
+
+class V8_EXPORT_PRIVATE Packet {
+ public:
+  Packet();
+
+  // Empty the vector and reset the read/write pointers.
+  void Clear();
+
+  // Reset the read pointer, allowing the packet to be re-read.
+  void Rewind();
+
+  // Return true of the read pointer has reached the write pointer.
+  bool EndOfPacket() const;
+
+  // Store a single raw 8 bit value
+  void AddRawChar(char ch);
+
+  // Store a block of data as hex pairs per byte
+  void AddBlock(const void* ptr, uint32_t len);
+
+  // Store a byte as a 2 chars block.
+  void AddWord8(uint8_t val);
+
+  // Store a number up to 64 bits, formatted as a big-endian hex string with
+  // preceeding zeros removed.  Since zeros can be removed, the width of this
+  // number is unknown, and the number is always followed by a NULL or a
+  // separator (non hex digit).
+  void AddNumberSep(uint64_t val, char sep);
+
+  // Add a string stored as a stream of ASCII hex digit pairs.  It is safe
+  // to use any non-null character in this stream.  If this does not terminate
+  // the packet, there should be a seperator (non hex digit) immediately
+  // following.
+  void AddHexString(const char* str);
+
+  // Retrieve a single character if available
+  bool GetRawChar(char* ch);
+
+  // Retrieve "len" ASCII character pairs.
+  bool GetBlock(void* ptr, uint32_t len);
+
+  // Retrieve a 8, 16, 32, or 64 bit word as pairs of hex digits.  These
+  // functions will always consume bits/4 characters from the stream.
+  bool GetWord8(uint8_t* val);
+
+  // Retrieve a number (formatted as a big-endian hex string) and a separator.
+  // If 'sep' is null, the separator is consumed but thrown away.
+  bool GetNumberSep(uint64_t* val, char* sep);
+
+  // Get a string from the stream
+  bool GetString(std::string* str);
+  bool GetHexString(std::string* str);
+
+  // Return a pointer to the entire packet payload
+  const char* GetPayload() const;
+  size_t GetPayloadSize() const;
+
+  // Returns true and the sequence number, or false if it is unset.
+  bool GetSequence(int32_t* seq) const;
+
+  // Parses sequence number in package data and moves read pointer past it.
+  void ParseSequence();
+
+  // Set the sequence number.
+  void SetSequence(int32_t seq);
+
+  enum class ErrDef { None = 0, BadFormat = 1, BadArgs = 2, Failed = 3 };
+  void SetError(ErrDef);
+
+  // Returns the full content of a GDB-remote packet, in the format:
+  //    $payload#checksum
+  // where the two-digit checksum is computed as the modulo 256 sum of all
+  // characters between the leading ‘$’ and the trailing ‘#’.
+  std::string GetPacketData() const;
+
+ private:
+  int32_t seq_;
+  std::string data_;
+  size_t read_index_;
+};
+
+}  // namespace gdb_server
+}  // namespace wasm
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_DEBUG_WASM_GDB_SERVER_PACKET_H_

src/debug/wasm/gdb-server/session.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "src/debug/wasm/gdb-server/session.h"
+#include "src/debug/wasm/gdb-server/packet.h"
 #include "src/debug/wasm/gdb-server/transport.h"
 
 namespace v8 {
@@ -10,7 +11,8 @@ namespace internal {
 namespace wasm {
 namespace gdb_server {
 
-Session::Session(Transport* transport) : io_(transport), connected_(true) {}
+Session::Session(TransportBase* transport)
+    : io_(transport), connected_(true), ack_enabled_(true) {}
 
 void Session::WaitForDebugStubEvent() { io_->WaitForDebugStubEvent(); }
 
@@ -34,14 +36,110 @@ bool Session::GetChar(char* ch) {
   return true;
 }
 
-bool Session::GetPacket() {
+bool Session::SendPacket(Packet* pkt) {
   char ch;
-  if (!GetChar(&ch)) return false;
+  do {
+    std::string data = pkt->GetPacketData();
+
+    TRACE_GDB_REMOTE("TX %s\n", data.c_str());
+    if (!io_->Write(data.data(), static_cast<int32_t>(data.length()))) {
+      return false;
+    }
+
+    // If ACKs are off, we are done.
+    if (!ack_enabled_) {
+      break;
+    }
+
+    // Otherwise, poll for '+'
+    if (!GetChar(&ch)) {
+      return false;
+    }
+
+    // Retry if we didn't get a '+'
+  } while (ch != '+');
 
-  // discard the input
   return true;
 }
 
+bool Session::GetPayload(Packet* pkt, uint8_t* checksum) {
+  pkt->Clear();
+  *checksum = 0;
+
+  // Stream in the characters
+  char ch;
+  while (GetChar(&ch)) {
+    if (ch == '#') {
+      // If we see a '#' we must be done with the data.
+      return true;
+    } else if (ch == '$') {
+      // If we see a '$' we must have missed the last cmd, let's retry.
+      TRACE_GDB_REMOTE("RX Missing $, retry.\n");
+      *checksum = 0;
+      pkt->Clear();
+    } else {
+      // Keep a running XSUM.
+      *checksum += ch;
+      pkt->AddRawChar(ch);
+    }
+  }
+  return false;
+}
+
+bool Session::GetPacket(Packet* pkt) {
+  while (true) {
+    // Toss characters until we see a start of command
+    char ch;
+    do {
+      if (!GetChar(&ch)) {
+        return false;
+      }
+    } while (ch != '$');
+
+    uint8_t running_checksum = 0;
+    if (!GetPayload(pkt, &running_checksum)) {
+      return false;
+    }
+
+    // Get two nibble checksum
+    uint8_t trailing_checksum = 0;
+    char chars[2];
+    if (!GetChar(&chars[0]) || !GetChar(&chars[1]) ||
+        !HexToUInt8(chars, &trailing_checksum)) {
+      return false;
+    }
+
+    TRACE_GDB_REMOTE("RX $%s#%c%c\n", pkt->GetPayload(), chars[0], chars[1]);
+
+    pkt->ParseSequence();
+
+    // If ACKs are off, we are done.
+    if (!ack_enabled_) {
+      return true;
+    }
+
+    // If the XSUMs don't match, signal bad packet
+    if (trailing_checksum == running_checksum) {
+      char out[3] = {'+', 0, 0};
+
+      // If we have a sequence number
+      int32_t seq;
+      if (pkt->GetSequence(&seq)) {
+        // Respond with sequence number
+        UInt8ToHex(seq, &out[1]);
+        return io_->Write(out, 3);
+      } else {
+        return io_->Write(out, 1);
+      }
+    } else {
+      // Resend a bad XSUM and look for retransmit
+      TRACE_GDB_REMOTE("RX Bad XSUM, retry\n");
+      io_->Write("-", 1);
+      // retry...
+    }
+  }
+}
+
 }  // namespace gdb_server
 }  // namespace wasm
 }  // namespace internal

src/debug/wasm/gdb-server/session.h

@@ -12,22 +12,24 @@ namespace internal {
 namespace wasm {
 namespace gdb_server {
 
-class Transport;
+class Packet;
+class TransportBase;
 
 // Represents a gdb-remote debugging session.
-class Session {
+class V8_EXPORT_PRIVATE Session {
  public:
-  explicit Session(Transport* transport);
+  explicit Session(TransportBase* transport);
+
+  // Attempt to send a packet and optionally wait for an ACK from the receiver.
+  bool SendPacket(Packet* packet);
 
   // Attempt to receive a packet.
-  // For the moment this method is only used to check whether the TCP connection
-  // is still active; all bytes read are discarded.
-  bool GetPacket();
+  bool GetPacket(Packet* packet);
 
   // Return true if there is data to read.
   bool IsDataAvailable() const;
 
-  // Return true if the connection still valid.
+  // Return true if the connection is still valid.
   bool IsConnected() const;
 
   // Shutdown the connection.
@@ -42,11 +44,23 @@ class Session {
   // Signal that the debuggee execution stopped because of a trap or breakpoint.
   bool SignalThreadEvent();
 
+  // By default, when either the debugger or the GDB-stub sends a packet,
+  // the first response expected is an acknowledgment: either '+' (to indicate
+  // the packet was received correctly) or '-' (to request retransmission).
+  // When a transport is reliable, the debugger may request that acknowledgement
+  // be disabled by means of the 'QStartNoAckMode' packet.
+  void EnableAck(bool ack_enabled) { ack_enabled_ = ack_enabled; }
+
  private:
+  // Read a single character from the transport.
   bool GetChar(char* ch);
 
-  Transport* io_;   // Transport object not owned by the Session.
-  bool connected_;  // Is the connection still valid.
+  // Read the content of a packet, from a leading '$' to a trailing '#'.
+  bool GetPayload(Packet* pkt, uint8_t* checksum);
+
+  TransportBase* io_;  // Transport object not owned by the Session.
+  bool connected_;     // Is the connection still valid.
+  bool ack_enabled_;   // If true, emit or wait for '+' from RSP stream.
 
   DISALLOW_COPY_AND_ASSIGN(Session);
 };

src/debug/wasm/gdb-server/target.cc

@@ -5,10 +5,11 @@
 #include "src/debug/wasm/gdb-server/target.h"
 
 #include "src/base/platform/time.h"
+#include "src/debug/wasm/gdb-server/gdb-remote-util.h"
 #include "src/debug/wasm/gdb-server/gdb-server.h"
+#include "src/debug/wasm/gdb-server/packet.h"
 #include "src/debug/wasm/gdb-server/session.h"
 #include "src/debug/wasm/gdb-server/transport.h"
-#include "src/debug/wasm/gdb-server/util.h"
 
 namespace v8 {
 namespace internal {
@@ -52,13 +53,34 @@ void Target::ProcessCommands() {
     return;
   }
 
-  // TODO(paolosev)
-  // For the moment just discard any packet we receive from the debugger.
+  // Now we are ready to process commands.
+  // Loop through packets until we process a continue packet or a detach.
+  Packet recv, reply;
   do {
-    if (!session_->GetPacket()) continue;
+    if (!session_->GetPacket(&recv)) continue;
+    reply.Clear();
+    if (ProcessPacket(&recv, &reply)) {
+      // If this is a continue type command, break out of this loop.
+      break;
+    }
+    // Otherwise send the response.
+    session_->SendPacket(&reply);
   } while (session_->IsConnected());
 }
 
+bool Target::ProcessPacket(const Packet* pktIn, Packet* pktOut) {
+  // Pull out the sequence.
+  int32_t seq = -1;
+  if (pktIn->GetSequence(&seq)) {
+    pktOut->SetSequence(seq);
+  }
+
+  // Ignore all commands and returns an error.
+  pktOut->SetError(Packet::ErrDef::Failed);
+
+  return false;
+}
+
 }  // namespace gdb_server
 }  // namespace wasm
 }  // namespace internal

src/debug/wasm/gdb-server/target.h

@@ -14,6 +14,7 @@ namespace wasm {
 namespace gdb_server {
 
 class GdbServer;
+class Packet;
 class Session;
 
 // Class Target represents a debugging target. It contains the logic to decode
@@ -41,6 +42,14 @@ class Target {
   // This method should be called when the debuggee has suspended its execution.
   void ProcessCommands();
 
+  // This function always succeedes, since all errors
+  // are reported as an error string of "E<##>" where
+  // the two digit number.  The error codes are not
+  // not documented, so this implementation uses
+  // ErrDef as errors codes.  This function returns
+  // true a request to continue (or step) is processed.
+  bool ProcessPacket(const Packet* pktIn, Packet* pktOut);
+
   enum class Status { Running, Terminated };
   std::atomic<Status> status_;
 

src/debug/wasm/gdb-server/transport.h

@@ -8,7 +8,7 @@
 #include <sstream>
 #include <vector>
 #include "src/base/macros.h"
-#include "src/debug/wasm/gdb-server/util.h"
+#include "src/debug/wasm/gdb-server/gdb-remote-util.h"
 
 #if _WIN32
 #include <windows.h>
@@ -47,7 +47,7 @@ namespace internal {
 namespace wasm {
 namespace gdb_server {
 
-class Transport;
+class SocketTransport;
 
 // Acts as a factory for Transport objects bound to a specified TCP port.
 class SocketBinding {
@@ -61,7 +61,7 @@ class SocketBinding {
   bool IsValid() const { return socket_handle_ != InvalidSocket; }
 
   // Create a transport object from this socket binding
-  std::unique_ptr<Transport> CreateTransport();
+  std::unique_ptr<SocketTransport> CreateTransport();
 
   // Get port the socket is bound to.
   uint16_t GetBoundPort();
@@ -70,10 +70,12 @@ class SocketBinding {
   SocketHandle socket_handle_;
 };
 
-class TransportBase {
+class V8_EXPORT_PRIVATE TransportBase {
  public:
-  explicit TransportBase(SocketHandle s);
-  virtual ~TransportBase();
+  virtual ~TransportBase() {}
+
+  // Waits for an incoming connection on the bound port.
+  virtual bool AcceptConnection() = 0;
 
   // Read {len} bytes from this transport, possibly blocking until enough data
   // is available.
@@ -81,23 +83,46 @@ class TransportBase {
   // Returns true on success.
   // Returns false if the connection is closed; in that case the {dst} may have
   // been partially overwritten.
-  bool Read(char* dst, int32_t len);
+  virtual bool Read(char* dst, int32_t len) = 0;
 
   // Write {len} bytes to this transport.
   // Return true on success, false if the connection is closed.
-  bool Write(const char* src, int32_t len);
+  virtual bool Write(const char* src, int32_t len) = 0;
 
   // Return true if there is data to read.
-  bool IsDataAvailable() const;
+  virtual bool IsDataAvailable() const = 0;
+
+  // If we are connected to a debugger, gracefully closes the connection.
+  // This should be called when a debugging session gets closed.
+  virtual void Disconnect() = 0;
 
   // Shuts down this transport, gracefully closing the existing connection and
   // also closing the listening socket. This should be called when the GDB stub
   // shuts down, when the program terminates.
-  void Close();
+  virtual void Close() = 0;
 
-  // If a socket connection with a debugger is present, gracefully closes it.
-  // This should be called when a debugging session gets closed.
-  virtual void Disconnect();
+  // Blocks waiting for one of these two events to occur:
+  // - A network event (a new packet arrives, or the connection is dropped),
+  // - A thread event is signaled (the execution stopped because of a trap or
+  // breakpoint).
+  virtual void WaitForDebugStubEvent() = 0;
+
+  // Signal that this transport should leave an alertable wait state because
+  // the execution of the debuggee was stopped because of a trap or breakpoint.
+  virtual bool SignalThreadEvent() = 0;
+};
+
+class Transport : public TransportBase {
+ public:
+  explicit Transport(SocketHandle s);
+  ~Transport() override;
+
+  // TransportBase
+  bool Read(char* dst, int32_t len) override;
+  bool Write(const char* src, int32_t len) override;
+  bool IsDataAvailable() const override;
+  void Disconnect() override;
+  void Close() override;
 
  protected:
   // Copy buffered data to *dst up to len bytes and update dst and len.
@@ -116,25 +141,16 @@ class TransportBase {
 
 #if _WIN32
 
-class Transport : public TransportBase {
+class SocketTransport : public Transport {
  public:
-  explicit Transport(SocketHandle s);
-  ~Transport() override;
-
-  // Waits for an incoming connection on the bound port.
-  bool AcceptConnection();
-
-  // Blocks waiting for one of these two events to occur:
-  // - A network event (a new packet arrives, or the connection is dropped),
-  // - A thread event is signaled (the execution stopped because of a trap or
-  // breakpoint).
-  void WaitForDebugStubEvent();
-
-  // Signal that this transport should leave an alertable wait state because
-  // the execution of the debuggee was stopped because of a trap or breakpoint.
-  bool SignalThreadEvent();
+  explicit SocketTransport(SocketHandle s);
+  ~SocketTransport() override;
 
+  // TransportBase
+  bool AcceptConnection() override;
   void Disconnect() override;
+  void WaitForDebugStubEvent() override;
+  bool SignalThreadEvent() override;
 
  private:
   bool ReadSomeData() override;
@@ -142,27 +158,20 @@ class Transport : public TransportBase {
   HANDLE socket_event_;
   HANDLE faulted_thread_event_;
 
-  DISALLOW_COPY_AND_ASSIGN(Transport);
+  DISALLOW_COPY_AND_ASSIGN(SocketTransport);
 };
 
 #else  // _WIN32
 
-class Transport : public TransportBase {
+class SocketTransport : public Transport {
  public:
-  explicit Transport(SocketHandle s);
-  ~Transport() override;
-
-  // Waits for an incoming connection on the bound port.
-  bool AcceptConnection();
+  explicit SocketTransport(SocketHandle s);
+  ~SocketTransport() override;
 
-  // Blocks waiting for one of these two events to occur:
-  // - A network event (a new packet arrives, or the connection is dropped),
-  // - The debuggee suspends execution because of a trap or breakpoint.
-  void WaitForDebugStubEvent();
-
-  // Signal that this transport should leave an alertable wait state because
-  // the execution of the debuggee was stopped because of a trap or breakpoint.
-  bool SignalThreadEvent();
+  // TransportBase
+  bool AcceptConnection() override;
+  void WaitForDebugStubEvent() override;
+  bool SignalThreadEvent() override;
 
  private:
   bool ReadSomeData() override;
@@ -170,7 +179,7 @@ class Transport : public TransportBase {
   int faulted_thread_fd_read_;
   int faulted_thread_fd_write_;
 
-  DISALLOW_COPY_AND_ASSIGN(Transport);
+  DISALLOW_COPY_AND_ASSIGN(SocketTransport);
 };
 
 #endif  // _WIN32

src/wasm/wasm-engine.cc

@@ -390,7 +390,7 @@ WasmEngine::WasmEngine() : code_manager_(FLAG_wasm_max_code_space * MB) {}
 WasmEngine::~WasmEngine() {
 #ifdef V8_ENABLE_WASM_GDB_REMOTE_DEBUGGING
   // Synchronize on the GDB-remote thread, if running.
-  gdb_server_ = nullptr;
+  gdb_server_.reset();
 #endif  // V8_ENABLE_WASM_GDB_REMOTE_DEBUGGING
 
   // Synchronize on all background compile tasks.
@@ -953,7 +953,7 @@ std::shared_ptr<NativeModule> WasmEngine::NewNativeModule(
     std::shared_ptr<const WasmModule> module, size_t code_size_estimate) {
 #ifdef V8_ENABLE_WASM_GDB_REMOTE_DEBUGGING
   if (FLAG_wasm_gdb_remote && !gdb_server_) {
-    gdb_server_ = std::make_unique<gdb_server::GdbServer>();
+    gdb_server_ = gdb_server::GdbServer::Create();
   }
 #endif  // V8_ENABLE_WASM_GDB_REMOTE_DEBUGGING
 

test/unittests/BUILD.gn

@@ -308,6 +308,10 @@ v8_source_set("unittests_sources") {
     "zone/zone-unittest.cc",
   ]
 
+  if (v8_enable_wasm_gdb_remote_debugging) {
+    sources += [ "wasm/wasm-gdbserver-unittest.cc" ]
+  }
+
   if (v8_current_cpu == "arm") {
     sources += [
       "assembler/turbo-assembler-arm-unittest.cc",

test/unittests/wasm/wasm-gdbserver-unittest.cc

+// Copyright 2019 the V8 project 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>
+
+#include "src/debug/wasm/gdb-server/packet.h"
+#include "src/debug/wasm/gdb-server/session.h"
+#include "src/debug/wasm/gdb-server/transport.h"
+#include "test/unittests/test-utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace v8 {
+namespace internal {
+namespace wasm {
+namespace gdb_server {
+
+using ::testing::_;
+using ::testing::Return;
+using ::testing::SetArrayArgument;
+using ::testing::StrEq;
+
+class WasmGdbRemoteTest : public ::testing::Test {};
+
+TEST_F(WasmGdbRemoteTest, GdbRemotePacketAddChars) {
+  Packet packet;
+
+  // Read empty packet
+  bool end_of_packet = packet.EndOfPacket();
+  EXPECT_TRUE(end_of_packet);
+
+  // Add raw chars
+  packet.AddRawChar('4');
+  packet.AddRawChar('2');
+
+  std::string str;
+  packet.GetString(&str);
+  EXPECT_EQ("42", str);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemotePacketAddBlock) {
+  static const uint8_t block[] = {0x01, 0x02, 0x03, 0x04, 0x05,
+                                  0x06, 0x07, 0x08, 0x09};
+  static const size_t kLen = sizeof(block) / sizeof(uint8_t);
+  Packet packet;
+  packet.AddBlock(block, kLen);
+
+  uint8_t buffer[kLen];
+  bool ok = packet.GetBlock(buffer, kLen);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ(0, memcmp(block, buffer, kLen));
+
+  packet.Rewind();
+  std::string str;
+  ok = packet.GetString(&str);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ("010203040506070809", str);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemotePacketAddString) {
+  Packet packet;
+  packet.AddHexString("foobar");
+
+  std::string str;
+  bool ok = packet.GetString(&str);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ("666f6f626172", str);
+
+  packet.Clear();
+  packet.AddHexString("GDB");
+  ok = packet.GetString(&str);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ("474442", str);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemotePacketAddNumbers) {
+  Packet packet;
+
+  static const uint64_t u64_val = 0xdeadbeef89abcdef;
+  static const uint8_t u8_val = 0x42;
+  packet.AddNumberSep(u64_val, ';');
+  packet.AddWord8(u8_val);
+
+  std::string str;
+  packet.GetString(&str);
+  EXPECT_EQ("deadbeef89abcdef;42", str);
+
+  packet.Rewind();
+  uint64_t val = 0;
+  char sep = '\0';
+  bool ok = packet.GetNumberSep(&val, &sep);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ(u64_val, val);
+  uint8_t b = 0;
+  ok = packet.GetWord8(&b);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ(u8_val, b);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemotePacketSequenceNumber) {
+  Packet packet_with_sequence_num;
+  packet_with_sequence_num.AddWord8(42);
+  packet_with_sequence_num.AddRawChar(':');
+  packet_with_sequence_num.AddHexString("foobar");
+
+  int32_t sequence_num = 0;
+  packet_with_sequence_num.ParseSequence();
+  bool ok = packet_with_sequence_num.GetSequence(&sequence_num);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ(42, sequence_num);
+
+  Packet packet_without_sequence_num;
+  packet_without_sequence_num.AddHexString("foobar");
+
+  packet_without_sequence_num.ParseSequence();
+  ok = packet_without_sequence_num.GetSequence(&sequence_num);
+  EXPECT_FALSE(ok);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemotePacketRunLengthEncoded) {
+  Packet packet1;
+  packet1.AddRawChar('0');
+  packet1.AddRawChar('*');
+  packet1.AddRawChar(' ');
+
+  std::string str1;
+  bool ok = packet1.GetHexString(&str1);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ("0000", std::string(packet1.GetPayload()));
+
+  Packet packet2;
+  packet2.AddRawChar('1');
+  packet2.AddRawChar('2');
+  packet2.AddRawChar('3');
+  packet2.AddRawChar('*');
+  packet2.AddRawChar(' ');
+  packet2.AddRawChar('a');
+  packet2.AddRawChar('b');
+
+  std::string str2;
+  ok = packet2.GetHexString(&str2);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ("123333ab", std::string(packet2.GetPayload()));
+}
+
+class MockTransport : public TransportBase {
+ public:
+  MOCK_METHOD0(AcceptConnection, bool());
+  MOCK_METHOD2(Read, bool(char*, int32_t));
+  MOCK_METHOD2(Write, bool(const char*, int32_t));
+  MOCK_METHOD0(IsDataAvailable, bool());
+  MOCK_CONST_METHOD0(IsDataAvailable, bool());
+  MOCK_METHOD0(Disconnect, void());
+  MOCK_METHOD0(Close, void());
+  MOCK_METHOD0(WaitForDebugStubEvent, void());
+  MOCK_METHOD0(SignalThreadEvent, bool());
+};
+
+TEST_F(WasmGdbRemoteTest, GdbRemoteSessionSendPacket) {
+  const char* ack_buffer = "+";
+
+  MockTransport mock_transport;
+  EXPECT_CALL(mock_transport, Write(StrEq("$474442#39"), 10))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_transport, Read(_, _))
+      .Times(1)
+      .WillOnce(
+          DoAll(SetArrayArgument<0>(ack_buffer, ack_buffer + 1), Return(true)));
+
+  Session session(&mock_transport);
+
+  Packet packet;
+  packet.AddHexString("GDB");
+  bool ok = session.SendPacket(&packet);
+  EXPECT_TRUE(ok);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemoteSessionSendPacketDisconnectOnNoAck) {
+  MockTransport mock_transport;
+  EXPECT_CALL(mock_transport, Write(StrEq("$474442#39"), 10))
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_transport, Read(_, _)).Times(1).WillOnce(Return(false));
+  EXPECT_CALL(mock_transport, Disconnect()).Times(1);
+
+  Session session(&mock_transport);
+
+  Packet packet;
+  packet.AddHexString("GDB");
+  bool ok = session.SendPacket(&packet);
+  EXPECT_FALSE(ok);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemoteSessionGetPacketCheckChecksum) {
+  const char* buffer_bad = "$47#00";
+  const char* buffer_ok = "$47#6b";
+
+  MockTransport mock_transport;
+  EXPECT_CALL(mock_transport, Read(_, _))
+      .WillOnce(
+          DoAll(SetArrayArgument<0>(buffer_bad, buffer_bad + 1), Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_bad + 1, buffer_bad + 2),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_bad + 2, buffer_bad + 3),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_bad + 3, buffer_bad + 4),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_bad + 4, buffer_bad + 5),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_bad + 5, buffer_bad + 6),
+                      Return(true)))
+      .WillOnce(
+          DoAll(SetArrayArgument<0>(buffer_ok, buffer_ok + 1), Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_ok + 1, buffer_ok + 2),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_ok + 2, buffer_ok + 3),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_ok + 3, buffer_ok + 4),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_ok + 4, buffer_ok + 5),
+                      Return(true)))
+      .WillOnce(DoAll(SetArrayArgument<0>(buffer_ok + 5, buffer_ok + 6),
+                      Return(true)));
+  EXPECT_CALL(mock_transport, Write(StrEq("-"), 1))  // Signal bad packet
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_transport, Write(StrEq("+"), 1))  // Signal ack
+      .Times(1)
+      .WillOnce(Return(true));
+
+  Session session(&mock_transport);
+
+  Packet packet;
+  bool ok = session.GetPacket(&packet);
+  EXPECT_TRUE(ok);
+  char ch;
+  ok = packet.GetBlock(&ch, 1);
+  EXPECT_TRUE(ok);
+  EXPECT_EQ('G', ch);
+}
+
+TEST_F(WasmGdbRemoteTest, GdbRemoteSessionGetPacketDisconnectOnReadFailure) {
+  MockTransport mock_transport;
+  EXPECT_CALL(mock_transport, Read(_, _)).Times(1).WillOnce(Return(false));
+  EXPECT_CALL(mock_transport, Disconnect()).Times(1);
+
+  Session session(&mock_transport);
+  Packet packet;
+  bool ok = session.GetPacket(&packet);
+  EXPECT_FALSE(ok);
+}
+
+}  // namespace gdb_server
+}  // namespace wasm
+}  // namespace internal
+}  // namespace v8