[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: docs/lldb-gdb-remote.txt:214
+//
+// BRIEF
+//  Packet for starting tracing of type lldb::TraceType. The following
----------------
I just noticed that none of our documentation uses doxygen?  Weird.


================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:396
+  virtual size_t GetTraceData(lldb::user_id_t uid, lldb::tid_t thread,
+                              Error &error, void *buf, size_t buf_size,
+                              size_t offset = 0) {
----------------
clayborg wrote:
> labath wrote:
> > llvm::MutableArrayRef<uint8_t> instead of buf+size pair. Maybe we could even pass in a reference to the MutableArrayRef so that the function can truncate it to the actual size?
> This is ok for internal functions. If we use a reference to a llvm::MutableArrayRef<> then we can return Error as the in/out mutable array ref reference can be updated with the right size right?
Agreed.
```
virtual Error GetTraceData(user_id_t uid, tid_t thread, MutableArrayRef<uint8_t> &Buffer, size_t offset=0);
```



================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:429-430
+  //------------------------------------------------------------------
+  virtual void GetTraceConfig(lldb::user_id_t uid, lldb::tid_t threadid,
+                              Error &error, TraceOptions &config) {
+    error.SetErrorString("Not implemented");
----------------
`virtual Error GetTraceConfig(user_id_t uid, tid_ threadid, TraceOptions &config)`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1110-1111
+
+  if (log)
+    log->Printf("GDBRemoteCommunicationServerLLGS::%s called", __FUNCTION__);
+
----------------
Can you use `LLDB_LOG()` macros here (and everywhere else in this patch)?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1113-1116
+  if (packet.GetStringRef().find("QTrace:1:type:") != 0)
+    return SendIllFormedResponse(packet, "QTrace:1: Ill formed packet ");
+
+  packet.SetFilePos(strlen("QTrace:1:type:"));
----------------
Can you add a function to `StringExtractor` that looks like this:

```
bool consume_front(StringRef Str) {
  StringRef S = GetStringRef();
  if (!S.starts_with(Str))
    return false;
  m_index += Str.size();
  return true;
}
```

Then you can change these 3 lines to:

```
  if (!packet.consume_front("QTrace:1:type:"))
    return SendIllFormedResponse(packet, "QTrace:1: Ill formed packet ");
```


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1138
+
+  StructuredData::DictionarySP custom_params(new StructuredData::Dictionary());
+  llvm::StringRef name, value;
----------------
Please use `auto custom_params = llvm::make_shared<StructuredData::Dictionary>();`

as it is more efficient from a memory standpoint.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1161
+  if (buffersize == std::numeric_limits<uint64_t>::max() ||
+      packet.GetBytesLeft() != 0) {
+
----------------
Can you use `!packet.empty()`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1201-1203
+  if (packet.GetStringRef().find("qTrace:conf:read:userid:") != 0)
+    return SendIllFormedResponse(packet, "qTrace: Ill formed packet ");
+  packet.SetFilePos(strlen("qTrace:conf:read:userid:"));
----------------
Use the `consume_front()` function I proposed earlier.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1285-1287
+  if (packet.GetStringRef().find("QTrace:0:userid:") != 0)
+    return SendIllFormedResponse(packet, "QTrace:0: Ill formed packet ");
+  packet.SetFilePos(strlen("QTrace:0:userid:"));
----------------
`consume_front()`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1344-1352
+  if (packet.GetStringRef().find("qTrace:buffer:read:userid:") == 0) {
+    tracetype = BufferData;
+    packet.SetFilePos(strlen("qTrace:buffer:read:userid:"));
+  } else if (packet.GetStringRef().find("qTrace:meta:read:userid:") == 0) {
+    tracetype = MetaData;
+    packet.SetFilePos(strlen("qTrace:meta:read:userid:"));
+  } else {
----------------
`consume_front`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1391
+
+  if (error_encountered || packet.GetBytesLeft() != 0 ||
+      byte_count == std::numeric_limits<size_t>::max() ||
----------------
`!packet.empty()`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1410-1411
+  if (tracetype == BufferData)
+    filled_data = m_debugged_process_sp->GetTraceData(
+        uid, tid, error, buf.data(), byte_count, offset);
+  else if (tracetype == MetaData)
----------------
If you make the change suggested earlier to convert this function to use array ref, then this line would be written:

```
MutableArrayRef<uint8_t> FilledData(buf);
if (tracetype == BufferData)
  error = m_debugged_process_sp->GetTraceData(uid, tid, FilledData, offset);
else
  error = m_debugged_process_sp->GetTraceMetaData(uid, tid, FilledData, offset);
```


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:181-182
+
+  virtual void StopTrace(lldb::user_id_t uid, lldb::tid_t thread_id,
+                         Error &error) override;
+
----------------
Return the `Error` instead of taking it as an output parameter.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:185
+  virtual size_t GetData(lldb::user_id_t uid, lldb::tid_t thread_id,
+                         Error &error, void *buf, size_t size,
+                         size_t offset = 0) override;
----------------
labath wrote:
> MutableArrayRef
Also, return the `Error` as before.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:188-197
+  virtual size_t GetMetaData(lldb::user_id_t uid, lldb::tid_t thread_id,
+                             Error &error, void *buf, size_t size,
+                             size_t offset = 0) override;
+
+  size_t GetTraceData(StreamString &packet, lldb::user_id_t uid,
+                      lldb::tid_t thread_id, Error &error, void *buf,
+                      size_t size, size_t offset = 0);
----------------
Same in all of these cases.  Try to return `Error` objects whenever possible.

Also remove the `virtual` keyword from all of these.  They are already marked `override`, so `virtual` is redundant.


https://reviews.llvm.org/D32585