This is an archive of the discontinued LLVM Phabricator instance.

Use Process Plugin register indices when communicating with remote
ClosedPublic

Authored by fjricci on Apr 19 2016, 6:06 PM.

Details

Summary

eRegisterKindProcessPlugin is used to store the register
indices used by the remote, and eRegisterKindLLDB is used
to store the internal lldb register indices. However, we're currently
using the lldb indices instead of the process plugin indices
when sending p/P packets. This will break if the remote uses
non-contiguous register indices.

Diff Detail

Event Timeline

fjricci updated this revision to Diff 54303.Apr 19 2016, 6:06 PM
fjricci retitled this revision from to Use Process Plugin register indices when communicating with remote.
fjricci updated this object.
fjricci added reviewers: jasonmolenda, clayborg.
fjricci added subscribers: sas, lldb-commits.
jasonmolenda edited edge metadata.Apr 19 2016, 8:51 PM

This looks good to me. Do you think the ReadRegister decl in GDBRemoteCommunicationClient.h should have a comment noting that the reg_num is a number in the remote register numbering scheme (eRegisterKindProcessPlugin)? When lldb passes around register numbers internally, it is usually assuming the eRegisterKindLLDB register numbering convention.

(to be honest, I think we should have come up with a RegisterNumber class from the start - the presence of raw register numbers, with side knowledge about what register encoding should be used, makes it easy for mistakes to creep in.)

clayborg accepted this revision.Apr 20 2016, 9:53 AM
clayborg edited edge metadata.

Looks good. As to Jason's comment about a register number class: I wanted RegisterContext classes to be able to statically initialize an array or RegisterInfo structs without having to call C++ constructors. That is the main reason the register numbers are just an array.

This revision is now accepted and ready to land.Apr 20 2016, 9:53 AM

I can add a comment to the ReadRegister decl.

Alternatively, if you think it would be preferable, I could change the parameter name from reg_num to something more descriptive, like remote_reg_num or plugin_reg_num.

Or I could change ReadRegister to take a RegisterInfo instead of a register number, and that way it could select the appropriate ProcessPlugin regnum on its own, right when it generates the packet. This method actually might be most consistent with the rest of the changes in the patch.

This revision was automatically updated to reflect the committed changes.

Committed with comment added to ReadRegister declaration to specify that the argument is an eRegisterKindProcessPlugin register number