Page MenuHomePhabricator

[lldb-server][LLGS] Support 'g' packets
ClosedPublic

Authored by guiandrade on May 21 2019, 2:54 PM.

Details

Summary

This change adds a 'g' packet handler to lldb-server
This is useful for the case when all registers are being fetched every step and the RTT is non-negligible.

Event Timeline

guiandrade created this revision.May 21 2019, 2:54 PM

Using 'g' packets seems fine to me and we might be able to just enable it. We will need to ensure that this doesn't regress stepping times so we will need to get people to try this out on their systems first. Most of our targets expedite enough registers in the stop reply packet where we don't need to read that many registers. Are you using lldb-server as your GDB remote binary or another GDB server?

lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
307–316 ↗(On Diff #200591)

I would make this a gdb-remote setting like "plugin.process.gdb-remote.packet-timeout" but named "plugin.process.gdb-remote.use-g-packet". Then we don't need a #define. It would be fine to use this all the time for most targets I believe. So it might be worth trying this out. We will need to watch stepping times.

lldb/source/Utility/StringExtractorGDBRemote.cpp
379–382

What if another packet starts with 'g'?

jasonmolenda added inline comments.
lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
307–316 ↗(On Diff #200591)

One way to force the use of g/G is to not support p/P in the Remote Serial Protocol stub. I've seen some stubs that behave this way. g is the older packet, and IMO is a more fragile one because it requires complete agreement about the layout of the register context. for lldb + lldbserver or lldb + debugserver, this isn't such a concern, but with interop with random other Remote Serial Protocol implementations, p/P is much simpler.

I understand the perf concerns of using p/P for multiple registers - as Greg says, ideally the RSP stub will include the base register context for the stopped thread in the stop packet aka T packet aka ? packet. For debugging with multiple threads, we use the jThreadsInfo packet to send the GPR register contents and likely stack memory needed for a simple stack unwind to address these issues.

labath added a subscriber: mgorny.May 22 2019, 1:08 AM

I think it would be good to split this patch into two:

  • implementing g packet in lldb-server
  • deciding when lldb sends the g packet

For the first part, I don't see any reason why lldb-server should *not* support the g packet.

The second part, as others have pointed out is a bit more tricky, as the g packet may not always be a win. For that, it would be nice to know more about your use case. When you say "all registers are being fetched every step", do you mean that lldb does that on its own, or that you (as in, something external to lldb) for whatever reason want to have all registers read out after each step?

If it's the first thing, then we can probably do something even better, and make sure that lldb-server sends the required registers directly in the stop-reply packet.

If it's the second thing then we can play around with the conditions under which lldb can use the g packet. Eg. it definitely sounds like register read --all would benefit from this packet, but maybe register read $specific_register might not. Or this may not even matter, as for the most common values of $specific_register, we should have the data available from the stop-reply packets, and others aren't really used that often...

lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteGPacket.py
0

This test is pretty weak, as all it does is check that "some" data was received. It would be much better to implement something similar to what @mgorny did in https://reviews.llvm.org/D61072 and other patches, only at lldb-server level. I.e., have an inferior which sets as many registers as possible to known values, and then have the test assert that. There should already be some code which parses qRegisterInfo responses in the test suite somewhere, so all you'd need is to fetch the offsets from there, and check that the values are indeed correct.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1912–1954

There's a NativeRegisterContext::ReadAllRegisterValues function. Can you check if that returns the data in the format that you need here?

Even if it doesn't, there's a good chance we could tweak it so that it does, as I believe it's now only used for implementing QSave/RestoreRegisterState, and the format we use for saving that is completely up to us.

guiandrade marked an inline comment as done.
guiandrade edited the summary of this revision. (Show Details)

Making this revision be responsible only for adding the 'g' packet handler and tests. Also, I'm starting to implement a stronger test to verify the actual content of the packets returned by the new handler.

Thank you so much for the feedback!

Using 'g' packets seems fine to me and we might be able to just enable it. We will need to ensure that this doesn't regress stepping times so we will need to get people to try this out on their systems first. Most of our targets expedite enough registers in the stop reply packet where we don't need to read that many registers. Are you using lldb-server as your GDB remote binary or another GDB server?

I am using lldb-server, @clayborg.

I think it would be good to split this patch into two:

  • implementing g packet in lldb-server
  • deciding when lldb sends the g packet

For the first part, I don't see any reason why lldb-server should *not* support the g packet.

The second part, as others have pointed out is a bit more tricky, as the g packet may not always be a win. For that, it would be nice to know more about your use case. When you say "all registers are being fetched every step", do you mean that lldb does that on its own, or that you (as in, something external to lldb) for whatever reason want to have all registers read out after each step?

If it's the first thing, then we can probably do something even better, and make sure that lldb-server sends the required registers directly in the stop-reply packet.

If it's the second thing then we can play around with the conditions under which lldb can use the g packet. Eg. it definitely sounds like register read --all would benefit from this packet, but maybe register read $specific_register might not. Or this may not even matter, as for the most common values of $specific_register, we should have the data available from the stop-reply packets, and others aren't really used that often...

@labath, I have a debug engine that populates a UI responsible for displaying register contents, hence I end up having to fetch all of them every step if the user decides to leave it on. I understand that using g packets isn't the best way to go every time, but I think if we can find a nice way to decide when to use them, it would be a win. So I will separate this patch into two as you suggested to make it easier to discuss each part.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
585

@labath, is it something like this you have in mind? If it is, where should we add the file that contains the inferior that sets the registers?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1912–1954

Apparently it is not:

Reading each register individually
Using reg_ctx.ReadAllRegisterValues(.).

Yeah, it would be convenient to be able to reuse that function, but then wouldn't that involve modifying several ReadAllRegisterValues implementations in order to do so?

lldb/source/Utility/StringExtractorGDBRemote.cpp
379–382

I removed the if because I thought the packet could have the thread id appended to it in ldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L535. If that's really the case, should we still try to make it more robust to avoid prefix collisions?

Does your feature print *all* the registers including floating point/vector registers? Or just the main general purpose registers? In debugserver, we expedite the register values for the stopping thread:

1558572448.355979919 < 624> read packet: $T05thread:3ef471;threads:3ef471;thread-pcs:10001e939;00:d803000000000000;01:0080536cff7f0000;02:d803000000000000;03:b0e0bfeffe7f0000;04:0000000000000000;05:2900000000000000;06:a0e2bfeffe7f0000;07:a8e0bfeffe7f0000;08:0000000000000000;09:0000000000000000;0a:0000000000000000;0b:0000000000000000;0c:00e2bfeffe7f0000;0d:b0e2bfeffe7f0000;0e:0000000000000000;0f:2900000000000000;10:39e9010001000000;11:4602000000000000;12:2b00000000000000;13:0000000000000000;14:0000000000000000;metype:6;mecount:2;medata:2;medata:0;memory:0x7ffeefbfe2a0=10e7bfeffe7f000006528a6dff7f0000;memory:0x7ffeefbfe710=40e7bfeffe7f000086838b6dff7f0000;#00

because the cost of sending all the register values every time is miniscule. When we are at a "public stop" (visible to the user), we may need to get information about *all* the threads - for this, we use the JSON-formatted jThreadsInfo packet which gives you the GPR register values for every thread (in this example, there is only one thread) -

1558572448.379079103 < 16> send packet: $jThreadsInfo#c1
1558572448.379371881 <1609> read packet: $[{"tid":4125809,"metype":6,"medata":[2,0],"reason":"exception","qaddr":4295855808,"associated_with_dispatch_queue":true,"dispatch_queue_t":140735794523072,"qname":"com.apple.main-thread","qkind":"serial","qserialnum":1,"registers":{"0":"0000000000000000","1":"c0f9bfeffe7f0000","2":"0200000000000000","3":"1000000000000000","4":"0006000000000000","5":"d34c566cff7f0000","6":"50e5bfeffe7f0000","7":"f8e4bfeffe7f0000","8":"4400000000000000","9":"4002000000000000","10":"03000000ffffffff","11":"4602000000000000","12":"583a566cff7f0000","13":"753a566cff7f0000","14":"0000000000000000","15":"0000000000000000","16":"66b7a56dff7f0000","17":"4602000000000000","18":"2b00000000000000","19":"0000000000000000","20":"0000000000000000"}],"memory":[{"address":140732920751440,"bytes":"80e5bfeffe7f000032d0846dff7f0000"}],{"address":140732920751488,"bytes":"a0e5bfeffe7f0000f490856dff7f0000"}],{"address":140732920751520,"bytes":"d0e5bfeffe7f000091e7766aff7f0000"}],{"address":140732920751568,"bytes":"70e6bfeffe7f0000fe15896dff7f0000"}],{"address":140732920751728,"bytes":"b0e6bfeffe7f0000de14896dff7f0000"}],{"address":140732920751792,"bytes":"10e7bfeffe7f0000fd7e8a6dff7f0000"}],{"address":140732920751888,"bytes":"40e7bfeffe7f0000cf838b6dff7f0000"}],{"address":140732920751936,"bytes":"d0edbfeffe7f00009869010001000000"}],{"address":140732920753616,"bytes":"40f7bfeffe7f00003a40010001000000"}],{"address":140732920756032,"bytes":"60f8bfeffe7f000027e2000001000000"}],{"address":140732920756320,"bytes":"88f8bfeffe7f000025e0000001000000"}],{"address":140732920756360,"bytes":"00000000000000000100000000000000"}]]}]]#00

I think for your use case, having lldb-server provide all the values up front (if it isn't already) is the approach you should take.

(the only thing that's might be unexpected is that the register values & memory data are sent as *strings* - JSON only represents numbers in base10; this is sending the register & memory data in target-endian order, little endian here, in the hex-bytes strings.)

labath added inline comments.May 23 2019, 1:55 AM
lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
585

Yes, that looks pretty much like what I had in mind. About the inferior, what I think we should do here is move this test into a separate subfolder. Then the test can have it's own Makefile, cpp file, etc..

If you need to share some code between this test and the new one, you can put the common stuff into lldbgdbserverutils.py or gdbremote_testcase.py. Or, if the only two tests using the common code are this one and the p test below, then maybe you can move the p test too, so that both register reading tests live together.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1912–1954

Ok, I think i see what ReadAllRegisterValues is doing. Let's keep the implementation here then

1916

You could just use a std::vector<uint8_t>, and memcpy data of each register into it (resizing as needed). Using a std::map for this thing seems pretty wasteful.

1946–1954

Then this would just be response.PutBytesAsRawHex(vector.data(), vector.size())

Add inferior to to set the values of a few registers to known values so we can verify the 'g' packet looks good. Also, start testing with/without thread suffix .

guiandrade marked 3 inline comments as done.May 27 2019, 2:45 PM

I have a couple of comments inline, but overall I think this looks pretty good now.

It would be interesting to also test reading %ymm registers, as those are stored in a funny way in the cpu context (and so we are most likely to get them wrong), however, I don't think this has to be done now.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py
140–147

Right now, these tests will only work on x86_64, so you'll need to add something like @skipIf(archs=no_match(["x86_64"])).

+ @omjavaid in case he's interested in contributing an arm version of these.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1936–1937

It doesn't look like this reinterpret_cast is needed. You should be able to feed reg_value.GetBytes() directly to memcpy...

lldb/source/Utility/StringExtractorGDBRemote.cpp
379–382

I think this is fine because *we* don't support any other packet starting with g. So, we can just classify that packet as g, and later fail due to a syntax error. If we end up supporting another packet like that, we can refine the classification logic then.

This is also the same pattern used by other single-letter packets. (eg. below).

Remove redundant cast from GDBRemoteCommunicationServerLLGS::Handle_g and add annotation to TestGdbRemoteGPacket::g_returns_correct_data so it gets skipped if the running architecture isn't x86_64

guiandrade marked 2 inline comments as done.May 28 2019, 5:05 PM
labath accepted this revision.May 28 2019, 11:59 PM
This revision is now accepted and ready to land.May 28 2019, 11:59 PM

BTW, do you have commit access, or you need someone to commit this for you?

BTW, do you have commit access, or you need someone to commit this for you?

I do not, I would need someone to help me commit that.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 12:22 AM