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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: docs/lldb-gdb-remote.txt:212
 //----------------------------------------------------------------------
+// QTrace:1:type:<type>;
+//
----------------
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? 


================
Comment at: docs/lldb-gdb-remote.txt:239
+//
+//  Each tracing instance is identified by a user id which is returned
+//  as the reply to this packet. In case the tracing failed to begin an
----------------
labath wrote:
> can we call this "trace id"? I'd like to avoid people confusing this with *real* UIDs.
trace id would be nice


================
Comment at: docs/lldb-gdb-remote.txt:269
+//
+//  An empty response is sent in case of success else an error code is
+//  returned.
----------------
labath wrote:
> Empty response means "packet not supported". You should send OK in this case.
Agreed


================
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) {
----------------
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?


https://reviews.llvm.org/D32585