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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
