This is an archive of the discontinued LLVM Phabricator instance.

Accept g packet responses that don't supply all registers
ClosedPublic

Authored by jasonmolenda on Nov 18 2019, 6:50 PM.

Details

Summary

This was encountered while debugging a cortex m4 board with a Segger J-Link v 6.54. It only supports g/G for reading/writing registers, and it supports an xml register description. The register description includes the GPRs, the exception registers, and the floating point registers. But g only provides the general purpose registers.

lldb accepts a g response that is too large for the expected register context size, but it treats a too-small payload as an error. This change accepts a too-small payload, marking registers not included in the payload as being unavailable for retrieval (so "register read msp" will return the error that the register is unavailable).

The J-Link will accept a G packet (set registers) for the entire register context, so I didn't try to change the write registers path to recognize a register buffer where all the final registers are unavailable, and truncate the payload it sends.

I wrote a small gdb_remote_client test for this.

Diff Detail

Event Timeline

jasonmolenda created this revision.Nov 18 2019, 6:50 PM

It might be worth adding an extra conditional to check for a buffer of 0 bytes returned. I haven't checked if ReadAllRegisters() would return success if a 0-length register context was received.

labath added a subscriber: labath.Nov 19 2019, 12:29 AM
labath added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
215

Should this be something like reginfo->byte_offset+reg_info->byte_size < ... ?

jasonmolenda added a reviewer: labath.

Updated GDBRemoteRegisterContext::ReadRegisterBytes to only mark registers as valid if the full contents for the register are included in the g packet. Updated gdbclientutils.py to strip off the thread specifier from the G packet, if it's present. Change the test file to check that the G packet either includes the full register context or just the general purpose registers.

labath accepted this revision.Nov 20 2019, 3:00 AM

looks good to me

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
215–220

maybe just m_reg_valid[i] = reginfo->byte_offset + reginfo->byte_size <= buffer_sp->GetByteSize()

This revision is now accepted and ready to land.Nov 20 2019, 3:00 AM
jasonmolenda marked an inline comment as done.Nov 20 2019, 9:43 AM

Thanks for looking this over Pavel.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
215

Yeah, that was my first thought too, then I thought, SURELY we'll have the correct # of bytes for complete registers, even if it's less registers than expected. But that's maybe not a great assumption. I'll change it.

215–220

I think I like the more verbose form, but I don't feel strongly about it. It takes up more screen real estate but I think it's easier to understand at a glance - purely a personal opinion. I'm sure they compile to the same code.

This revision was automatically updated to reflect the committed changes.