Yes, we talked about this approach a few times. Greg went with the jstopinfo addition to the T packet to keep the changes relatively small, but we're all in agreement that we should create a new JSON formatted stop info packet and dump the T packet altogether.
That's one of those changes where we're taking a big step away from traditional gdb-remote protocol which is the only reason Greg hesitated to do it. There's always the concern about lldb compatibility with traditional gdb-remote implementations -- we don't test that very thoroughly/frequently/consistently, and it can be easy to add assumptions that we have the lldb extensions to the protocol. None of this is an argument against a JSON formatted thread reply packet. Greg and I both think that this is the best approach - it was more work than he wanted to take on when he was implementing this addition to the T packet. J > On Jul 31, 2015, at 2:46 AM, Pavel Labath <lab...@google.com> wrote: > > Thank you all for the replies, very interesting information there. > Given that the register caching approach will not work everywhere, I > agree we should stick to sending the PC on all platforms for > consistency. > > Regarding the size of the stop-reply packet, I have a question. Is > there any reason we are tied to the current format of the stop-reply > packet as a whole? Couldn't we just change it as a whole? For example, > we could add a QThreadStopInJSON packet, which will switch the server > to send the stop-replies in the current jThreadsInfo format (perhaps > prefixed by a T or something, to make it more easily detectable). This > would solve the problem of the size increase, and it would enable us > to easily add more data to the packet in the future. > > What do you think? > > cheers, > pl > > > On 30 July 2015 at 19:10, Jason Molenda <ja...@molenda.com> wrote: >> We were discussing the same thing last week - I think we all agree that >> approach #1 is the way to go. My main concern here is that jstopinfo is >> ascii-hex encoded. JSON is already pretty verbose (particularly with all >> the numbers being base 10, sigh) but then we double the size of the string. >> Originally when we listed the stop reasons for all the threads were were >> including a list of all threads and their stop reasons (even if it is >> 'none') and the payload was enormous. I was playing with ideas like given >> that the T packet already gives us a list of thread ids outside the >> jstopinfo payload, maybe we could include an array of thread pc values but >> that was not a good idea even though it would be compact. I think we'll >> need to include a dictionary of threadid:pc's in there. We could have >> mitigated the expansion a little by adopting base64 encoding instead of >> asciihex (133% increase as opposed to 200% increase) but base64 isn't used >> anywhere else in the protocol so it wo! uld have been weird to adopt it in this one place. >> >> J >> >>> On Jul 30, 2015, at 5:09 AM, Pavel Labath <lab...@google.com> wrote: >>> >>> Hello Greg, Jim, >>> >>> after adding jstopinfo support to LLGS, i am still getting a bunch of >>> register read packets which access the program counter. These are >>> coming from Thread::SetupForResume(), where we need to check whether a >>> thread is on a breakpoint before letting it run. I would like to get >>> rid of those packets (my preliminary tests show I can gain about 10% >>> stepping speed improvement on my test app with 20 threads). I see two >>> ways to go about this: >>> 1. send the program counters as a part of the jstopinfo field. I will >>> need to change the format of the field a bit, because the current >>> assumption is that you do not include the threads which don't have a >>> stop reason there, but we need the registers for every thread. >>> 2. cache the registers on the client side. These queries happen after >>> we have previously done a vCont:s (as a part of ThreadPlanStepOver), >>> so the client can determine that the other threads have not executed, >>> and the registers are unchanged. We would still avoid caching the stop >>> reason, since on OSX this can change even if the thread is not >>> running, but I hope the registers remain the same. >>> >>> All in all, the first approach is probably more easier to implement, >>> but the second one sounds better to me architecturally, and has the >>> benefit of caching all registers, and not just the PC. >>> >>> Do you have any thoughts on this? >>> >>> cheers, >>> pl >>> >>> >>> On 23 July 2015 at 10:10, Pavel Labath <lab...@google.com> wrote: >>>> This revision was automatically updated to reflect the committed changes. >>>> labath marked 2 inline comments as done. >>>> Closed by commit rL242997: Add jstopinfo support to llgs (authored by >>>> labath). >>>> >>>> Changed prior to commit: >>>> http://reviews.llvm.org/D11415?vs=30348&id=30464#toc >>>> >>>> Repository: >>>> rL LLVM >>>> >>>> http://reviews.llvm.org/D11415 >>>> >>>> Files: >>>> lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp >>>> lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp >>>> lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h >>>> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp >>>> >>> _______________________________________________ >>> lldb-commits mailing list >>> lldb-commits@cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >> _______________________________________________ lldb-commits mailing list lldb-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
