[Lldb-commits] [PATCH] D22629: Rewrite gdb-remote's SendContinuePacketAndWaitForResponse
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Wed Jul 27 13:01:05 PDT 2016
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
More information about the lldb-commits
mailing list