[Lldb-commits] [PATCH] D11415: Add jstopinfo support to llgs

[Pavel Labath]

Thanks for the explanation. I will be away for two weeks, but then I
will look into this again, if somebody does not beat me to it :)

cheers,
pl

On 31 July 2015 at 17:28, Jason Molenda <jason at molenda.com> wrote:
> 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 <labath at 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 <jason at 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 would have been weird to adopt it in this one place.
>>>
>>> J
>>>
>>>> On Jul 30, 2015, at 5:09 AM, Pavel Labath <labath at 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 <labath at 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 at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>
>