[Lldb-commits] [PATCH] D22629: Rewrite gdb-remote's SendContinuePacketAndWaitForResponse

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

This provides a clearer implementation of our async code that grew over the years and was bolted on, thanks for digging into this.

Looks good a few changes, see inlined comments.

I have some reservations about your future changes to add concurrent packets. We should discuss this at length before you try to make any changes on the mailing list.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:57
@@ +56,3 @@
+                std::lock_guard<std::mutex> lock(m_mutex);
+                if (m_async_count == 0)
+                    continue;
----------------
if you add an assert, please use lldbassert

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:148-150
@@ +147,5 @@
+    Lock lock(*this, true);
+    if (lock.DidInterrupt())
+        m_should_stop = true;
+    return lock.DidInterrupt();
+}
----------------
Do we need to call lock.DidInterrupt() twice?

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:261-268
@@ +260,10 @@
+    std::string inferior_stdout;
+    inferior_stdout.reserve(packet.GetBytesLeft() / 2);
+
+    uint8_t ch;
+    while (packet.GetHexU8Ex(ch))
+    {
+        if (ch != 0)
+            inferior_stdout.append(1, (char)ch);
+    }
+    delegate.HandleAsyncStdout(inferior_stdout);
----------------
We can probably switch over using a new function that was added after this code was created:
```
size_t
StringExtractor::GetHexByteString (std::string &str);
```

So this  could be:

```
inferior_stdout.reserve(packet.GetBytesLeft() / 2);
packet. GetHexByteString(inferior_stdout.reserve);
```



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:28-29
@@ +27,4 @@
+        virtual ~ContinueDelegate();
+        virtual void
+        HandleAsyncStdout(llvm::StringRef out) = 0;
+        virtual void
----------------
Why not just use "const std::string &" here instead of llvm::StringRef? It is OK to use std::string in internal APIs and llvm::StringRef buys us nothing.

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:31
@@ +30,3 @@
+        virtual void
+        HandleAsyncMisc(llvm::StringRef data) = 0;
+        virtual void
----------------
Why not just use "const std::string &" here instead of llvm::StringRef? It is OK to use std::string in internal APIs and llvm::StringRef buys us nothing.


https://reviews.llvm.org/D22629