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

[Pavel Labath via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D22629#498135, @clayborg wrote:

> 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.


Agreed. I have a follow-up almost ready. I think I'll be able to upload it as a WIP later in the day. I think it will be easier to explain the idea with a concrete example.


================
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;
----------------
clayborg wrote:
> if you add an assert, please use lldbassert
This comment shows up in the wrong spot, I am moving it to the place where the assert() is

================
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();
+}
----------------
clayborg wrote:
> Do we need to call lock.DidInterrupt() twice?
The call is cheap so I don't think it matters, but I've reshuffled it a bit and now it's one.

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:297
@@ +296,3 @@
+{
+    assert(m_acquired);
+    {
----------------
The `assert()` (and two more in the `lock()` function). I am going  to change it back to lldbassert, but this checks a very critical part of the internal locking logic, and I would very much doubt our ability recover if these conditions are not met.

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:31
@@ +30,3 @@
+        virtual void
+        HandleAsyncMisc(llvm::StringRef data) = 0;
+        virtual void
----------------
clayborg wrote:
> 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.
In the previous case it does not matter (right now (*)), but it this case it actually saves one string copy: The caller does a substr(1) on the received packet (to remove the leading 'A'), which is a cheap operation on a StringRef, but not on a string. 

(*) It does however, future-proof the api. If the caller or the callee end up using something other than a std::string in the future, the API will remain the same, and cheap, as probably StringRef can be used to convert to/from that.

In general, I would reverse the logic here: there is nothing that can be done with a string that you cannot do with a StringRef(**), so why use the former? It's also consistent with the llvm guidelines.

(**) The only exception here is the c_str() function, but if you use StringRefs everywhere then you don't need to call it. Also, mitigating circumstances are: - a lot of our apis already use const char*+length style, which is just a hand-rolled StringRef and is cheap to convert back and forth; by the time you end up calling c_str(), you have probably have already saved a couple of copies, so doing one copy there is not that bad.


https://reviews.llvm.org/D22629