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

[Ravitheja Addepally via Phabricator via lldb-commits]

ravitheja marked 28 inline comments as done.
ravitheja added inline comments.


================
Comment at: docs/lldb-gdb-remote.txt:212
 //----------------------------------------------------------------------
+// QTrace:1:type:<type>;
+//
----------------
clayborg wrote:
> labath wrote:
> > ravitheja wrote:
> > > clayborg wrote:
> > > > Should we make all these new packets JSON based to start with? "jTrace"? If we have any need for JSON at all in this or the other new packets I would say lets just go with JSON packets. They are currently prefixed with "j". If we go this route we should specify the mandatory key/value pairs in the header doc. We should also allow a JSON dictionary from the trace config up at the SBTrace layer to make it into this packet? 
> > > I am fine prefixing the packets with "j", I did not understand what you meant by the last sentence. At the moment I am waiting for Pavel or Tamas to reply or anyone else and based on the complete feedback, I will upload a new patch if needed.
> > I think Greg meant you should also change the packet format to take a json argument instead of key-value pairs (The packet should be JTrace if it modifies the state, right ?).
> > 
> > If there is a gdb equivalent which matches this functionality sufficiently closely, then I would prefer to stick to that. However, given that we already are attaching a json field to the packet, I think it would make sense to go json all the way.
> I think the entire packet should be:
> ```
> $jTrace:XXXX
> ```
> where XXXX is a JSON dictionary. Ravitheja had talked before about adding an implementation specific JSON dictionary into the JSON that comes in to configure the trace at the lldb::SB layer. I was just saying it would be nice if this implementation specific JSON was sent down as part of this packet so the GDB server can use it. Maybe we just send the entire thing that came in at the SB layer? 
Yes thats what is currently being done, the JSON Dictionary obtained at SB layer is being passed down to the server through the remote packets.


================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:13
 
+#include "lldb/Core/TraceOptions.h"
 #include "lldb/Host/MainLoop.h"
----------------
labath wrote:
> You are missing the traceOptions file from this patch.
The traceOptions are already merged into lldb code, please refer to ->https://reviews.llvm.org/D29581


================
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");
----------------
zturner wrote:
> `virtual Error GetTraceConfig(user_id_t uid, tid_ threadid, TraceOptions &config)`
The threadid is already inside the TraceOptions, which needs to be set by the user, I will update the comments here.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1161
+  if (buffersize == std::numeric_limits<uint64_t>::max() ||
+      packet.GetBytesLeft() != 0) {
+
----------------
zturner wrote:
> Can you use `!packet.empty()`
I think its not possible to use empty function coz it checks the size of m_packet string inside and not the current m_index.


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


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:178
 
+  virtual lldb::user_id_t StartTrace(lldb::TraceOptionsSP &options,
+                                     Error &error) override;
----------------
labath wrote:
> `const TraceOptions &options` ? Having a reference to a shared pointer seems like a huge overkill here.
Yes u are right my mistake.


https://reviews.llvm.org/D32585