Page MenuHomePhabricator

[LLDB] Support custom expedited register set in gdb-remote
ClosedPublic

Authored by omjavaid on Jun 30 2020, 3:30 AM.

Details

Summary

This patch adds capability to introduce a custom expedited register set in gdb remote. Currently we sent register set 0 as expedited register but for the case of AArch64 SVE we intend to send additional information about register set configuration stored in vg register. This will happen only when SVE mode is selected so we need a way to inform gdb-remote which register set to use when.

Diff Detail

Event Timeline

omjavaid created this revision.Jun 30 2020, 3:30 AM

Looking at the dependent patch, I see that you need to conjure up a fake register set to make this work. That works, but doesn't seem entirely optimal. This expedition function doesn't really care about register sets, it just needs a way to get a list of registers. What if we just made the interface return that? (e.g. ArrayRef<uint32_t> GetExpeditedRegisters())

And instead of handing the nullptr specially in the caller, we could just make the default implementation of this function return the first (zeroth) register set...

omjavaid updated this revision to Diff 287323.Aug 24 2020, 3:21 AM

This reworks previous implementation by returning a vector containing register numbers to be expedited. Default case is minimal set of generic registers or complete register set 0.

In the child rev D82855 for NativeRegisterContextLinux_arm64 we call the base function and push_back vg reg num into register number vector before return.

labath accepted this revision.Aug 26 2020, 1:44 AM

I like this a lot. LGTM with some small fixes inline.

lldb/source/Host/common/NativeRegisterContext.cpp
428–434

Remove LLDB_INVALID_REGNUM from the list. Then iterate as for(uint32_t gen_reg: k_expedited_registers).

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
515–517

Let's change the result type to Optional<Object> and return None here -- now that we support customizing the expedited registers, I think it's reasonable to allow a register context to say it does not want to expedite any registers. (The current for of the code already kind of supports that, but the use of Expected makes it sound like that is an erroneous state.)

793–794

I don't think this if is needed now -- we could remove it and save one level of indentation.

This revision is now accepted and ready to land.Aug 26 2020, 1:44 AM
omjavaid updated this revision to Diff 288942.Aug 31 2020, 7:07 AM

Fixed review comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 4:35 AM