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