This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Make offset field optional in RegisterInfo packet for Arm64
ClosedPublic

Authored by omjavaid on Nov 11 2020, 2:22 AM.

Details

Summary

This patch carries forward our aim to remove offset field from qRegisterInfo packets and XML register description. I have created a new function which returns if offset fields are dynamic meaning client can calculate offset on its own based on register number sequence and register size. For now this function only returns true for NativeRegisterContextLinux_arm64 but we can test this for other architectures and make it standard later.

As a consequence we do not send offset field from lldb-server (arm64 for now) while other stubs dont have an offset field so it wont effect them for now. On the client side we already have a mechanism to calculate the offset but a minor adjustment has been made to make offset increment only for primary registers.

Also some tests have been adjusted for this change.

Diff Detail

Event Timeline

omjavaid created this revision.Nov 11 2020, 2:22 AM
omjavaid requested review of this revision.Nov 11 2020, 2:22 AM

This looks pretty straightforward, modulo the inline comment about the centralization of the offset computations....

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
567

This part seems dubious. It would be better if it were not tied to the architecture, but to the fact that the stub did not provide the register offset for these registers. It would also be better if it was placed together with the rest of the code for computing the register offsets.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
572

Which, I guess, means here.

4441

And here (feel free to factor this into some kind of a utility function).

This looks pretty straightforward, modulo the inline comment about the centralization of the offset computations....

So the current approach of finalizing offsets of pseudo registers after receiving/processing all registers is because in case of SVE we define FPU register set first containing V, D, S pseudo registers first and then define SVE registers set where their primary register Z is defined.

We have choice of either documenting that all pseudo register should come after their corresponding primary register are defined and make this change to SVE register definitions as well where we are not enforcing this principle mainly due to consistency with non-SVE vs SVE register sets..

That's a good point. Maybe this does need to be a two-pass algorithm (first compute the offsets of primary registers, then fill out subregs). But that doesn't mean the two passes should be in two completely separate files. It would still be better if that was done in a single place. Maybe we can move all the offset computation out of the parsing loop and into a separate helper function. Doing the computation inside the loop is not really correct anyway, as the registers in the target.xml don't have to come in ascending regnum order (all stubs probably send them that way, but I don't think they _have_ to do that).

That's a good point. Maybe this does need to be a two-pass algorithm (first compute the offsets of primary registers, then fill out subregs). But that doesn't mean the two passes should be in two completely separate files. It would still be better if that was done in a single place. Maybe we can move all the offset computation out of the parsing loop and into a separate helper function. Doing the computation inside the loop is not really correct anyway, as the registers in the target.xml don't have to come in ascending regnum order (all stubs probably send them that way, but I don't think they _have_ to do that).

I guess GDB standard does enforce ascending order, here is what it says about regnum:

"The register’s number. If omitted, a register’s number is one greater than that of the previous register (either in the current feature or in a preceding feature); the first register in the target description defaults to zero. This register number is used to read or write the register; e.g. it is used in the remote p and P packets, and registers appear in the g and G packets in order of increasing register number."

https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html

I guess GDB standard does enforce ascending order, here is what it says about regnum:

"The register’s number. If omitted, a register’s number is one greater than that of the previous register (either in the current feature or in a preceding feature); the first register in the target description defaults to zero. This register number is used to read or write the register; e.g. it is used in the remote p and P packets, and registers appear in the g and G packets in order of increasing register number."

https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html

I've just read that paragraph before writing that, but that's not how I would interpret it. What I think that says is:

  • in the g packet, registers appear in the increasing register number order. (I think we agree on that part)
  • if the register number is omitted, the registers are assigned increasing register numbers. Here, I think the bold part is very important, as it means the rest of the sentence describes default/fallback behavior. If the stub specifies the register number explicitly, then I'd say it's free to send order the register descriptions any way it likes. It can even combine things and set explicit numbers for some register, but not others...

I guess GDB standard does enforce ascending order, here is what it says about regnum:

"The register’s number. If omitted, a register’s number is one greater than that of the previous register (either in the current feature or in a preceding feature); the first register in the target description defaults to zero. This register number is used to read or write the register; e.g. it is used in the remote p and P packets, and registers appear in the g and G packets in order of increasing register number."

https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html

I've just read that paragraph before writing that, but that's not how I would interpret it. What I think that says is:

  • in the g packet, registers appear in the increasing register number order. (I think we agree on that part)
  • if the register number is omitted, the registers are assigned increasing register numbers. Here, I think the bold part is very important, as it means the rest of the sentence describes default/fallback behavior. If the stub specifies the register number explicitly, then I'd say it's free to send order the register descriptions any way it likes. It can even combine things and set explicit numbers for some register, but not others...

I gave thought and looked at GDB handling of g/G packet offsets.
Important points to note:

  1. GDB assigns register numbers if they are not provided, with scheme last regnum + 1.
  2. GDB maintains registers list in order of their placement in XML regardless of any specific register numbers.
  3. GDB sends g/G packet based on register numbers in order of increasing register numbers. For this gdb creates a sorted list based of register numbers and then assigns offsets accordingly.

In LLDB we are trying to support legacy offset field which means offset calculation has to be done in location where we parse an XML register entry (ParseRegisters) or qRegisterInfo packet (ProcessGDBRemote::BuildDynamicRegisterInfo).

Also both of above functions are just inserting into register's information list maintained by DynamicRegisterInfo class so there is not enough leverage available to make offset calculation in one single location without giving up offset field altogether. We may write a helper function to do offset post processing for pseudo registers but that will still be a part of DynamicRegisterInfo class and may be called from ParseRegisters and ProcessGDBRemote::BuildDynamicRegisterInfo.

IMO, lets keep this current patch as it is unless we decide on giving up offset field for all architectures.

I disagree, and my main reason for that is that this approach requires hardcoding the "supported architectures" for which we do this fixup in the DynamicRegisterInfo class. Besides being wrong as a matter of priniciple, I suspect this will also be a problem in practice -- for example, I have no idea what will happen when this code starts interacting with the aarch64 debugserver (which will still be sending the offsets). Doing this decision based on whether the server sends the offsets avoids this issue, and the process can be seen as an extension of what we're already doing when communicating with offset-less stubs.

In LLDB we are trying to support legacy offset field

I wouldn't go so far as to call this a "legacy" field. Let's just say that for this particular problem on this architecture, we chose to stop using the offset field.

which means offset calculation has to be done in location where we parse an XML register entry (ParseRegisters) or qRegisterInfo packet (ProcessGDBRemote::BuildDynamicRegisterInfo).

Why is that? I don't see the connection here.

I don't see why the offsets could not be computed in the DynamicRegisterInfo class. The ParseRegisters/BuildDynamicRegisterInfo functions could just refrain from filling in the offset fields (leave them as invalid, or something) if the server did not provide them. Then DynamicRegisterInfo could use that invalid value to know which offsets need filling in.

In fact, if we wanted to be truly faithful to the gdb algorithm you described ("gdb creates a sorted list based of register numbers and then assigns offsets accordingly"), then it seems like we would _have to_ do this as a separate step, once all register numbers are known.

omjavaid updated this revision to Diff 305680.Nov 17 2020, 1:02 AM

This update tries to fix concerns raised by removing AArch64 specific code and makes offset assignment more generic.

We now have a new flag which is set to true if offset field was present and on basis of that we assign offsets to Primary and Pseudo registers.

If offset is not specified for a primary register then its offset is equal to last_register_offset + last_register_size. This is what LLDB was doing already we just restrict this scheme to primary registers only.

If offset is not specified for a pseudo register then fall-back offset is same as the offset of its first value_reg in value_regs list.

For assigning offsets to pseudo registers all primary registers should have valid offsets. Primary registers are assigned offsets in ParseRegisters and ProcessGDBRemote::BuildDynamicRegisterInfo while pseudo register offsets are update once their value_regs are known in DynamicRegisterInfo::Finalize

I disagree, and my main reason for that is that this approach requires hardcoding the "supported architectures" for which we do this fixup in the DynamicRegisterInfo class. Besides being wrong as a matter of priniciple, I suspect this will also be a problem in practice -- for example, I have no idea what will happen when this code starts interacting with the aarch64 debugserver (which will still be sending the offsets). Doing this decision based on whether the server sends the offsets avoids this issue, and the process can be seen as an extension of what we're already doing when communicating with offset-less stubs.

In LLDB we are trying to support legacy offset field

I wouldn't go so far as to call this a "legacy" field. Let's just say that for this particular problem on this architecture, we chose to stop using the offset field.

which means offset calculation has to be done in location where we parse an XML register entry (ParseRegisters) or qRegisterInfo packet (ProcessGDBRemote::BuildDynamicRegisterInfo).

Why is that? I don't see the connection here.

I don't see why the offsets could not be computed in the DynamicRegisterInfo class. The ParseRegisters/BuildDynamicRegisterInfo functions could just refrain from filling in the offset fields (leave them as invalid, or something) if the server did not provide them. Then DynamicRegisterInfo could use that invalid value to know which offsets need filling in.

In fact, if we wanted to be truly faithful to the gdb algorithm you described ("gdb creates a sorted list based of register numbers and then assigns offsets accordingly"), then it seems like we would _have to_ do this as a separate step, once all register numbers are known.

I have made some updates which are sufficient for current AArch64 scenario where offset field is not present. For ordering registers based on register numbers I think we may skip doing that for now though i intend to come back to it after getting SVE through.

Yes, this is pretty much what I hoped for. Thanks.

Besides relaxing existing tests, it would be nice to have a positive test that checks for the behavior that we want. Maybe a gdb-client test which checks that the register values are extracted from the appropriate place from within a g packet (send custom target.xml, and a g response, and ensure the register values come out what we expect)?

I have made some updates which are sufficient for current AArch64 scenario where offset field is not present. For ordering registers based on register numbers I think we may skip doing that for now though i intend to come back to it after getting SVE through.

That seems fair.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
480

How about we initialize to UINT32_MAX here, and then just don't update the field if the offset is not set.

574

The use of LLDB_INVALID_INDEX32 is not really correct (this is not an index) and the rest of the code (e.g. line 507) already uses UINT32_MAX.

Yes, this is pretty much what I hoped for. Thanks.

Besides relaxing existing tests, it would be nice to have a positive test that checks for the behavior that we want. Maybe a gdb-client test which checks that the register values are extracted from the appropriate place from within a g packet (send custom target.xml, and a g response, and ensure the register values come out what we expect)?

I have made some updates which are sufficient for current AArch64 scenario where offset field is not present. For ordering registers based on register numbers I think we may skip doing that for now though i intend to come back to it after getting SVE through.

That seems fair.

Let me take a look into writing a test case for this change.

Moreover, I have tried what you suggested about initializing all register offset with LLDB_INVALID_INDEX32 unless an offset is explicitly specified. The problem is that once we make that change in ProcessGDBRemote.cpp more changes are required elsewhere and many tests start failing. I kept plugging those tests with adjustments untill I decided on current solution which required less no of changes.

DynamicRegisterInfo::AddRegister also calculated the maximum offset if we take maximum offset calculation then it impacts DynamicRegisterInfo::SetRegisterInfo which is using Finalize but calculating offsets itself. Also oid GDBRemoteDynamicRegisterInfo::HardcodeARMRegisters also use DynamicRegisterInfo::AddRegister but calculates offsets by itself.

omjavaid updated this revision to Diff 307954.Nov 26 2020, 9:16 PM

This update improves on last patch by adding support for register ordering in increasing order of register number even if register numbers are used randomly in target xml. Also we set offset to LLDB_INVALID_INDEX32 while initializing and if an offset is provided by remote it will be updated. However if no offset was provided an appropriate offset will be calculated incrementally by traversing a map which sorts registers in increasing order of remote register numbers.

A AArch64 test is included which places cpsr at the top of target xml description with regnum field populated. Also w registers derive their offset from corresponding x registers. All register values are checked with a provided g packet result.

Looks great. One question about the member variable...

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h
90 ↗(On Diff #307954)

If, I understand correctly, this is only used inside the Finalize function? Could this be a local variable instead of a member?

Looks great. One question about the member variable...

m_remote_to_local_regnum_map is used by AddRegister where we add a (key,value) pair for each of registers. We are doing this to maintain a list of register sorted in increasing order of remote register numbers. This list is then used in Finalize where we are calculating offsets. Keeping this map as member saves us from generating a sorted list of remote register numbers.

Looks great. One question about the member variable...

m_remote_to_local_regnum_map is used by AddRegister where we add a (key,value) pair for each of registers. We are doing this to maintain a list of register sorted in increasing order of remote register numbers. This list is then used in Finalize where we are calculating offsets. Keeping this map as member saves us from generating a sorted list of remote register numbers.

Yes, but there's no reason that this _must_ be done in AddRegister, is there?

You could just build a local map directly inside the finalize function, right?

Looks great. One question about the member variable...

m_remote_to_local_regnum_map is used by AddRegister where we add a (key,value) pair for each of registers. We are doing this to maintain a list of register sorted in increasing order of remote register numbers. This list is then used in Finalize where we are calculating offsets. Keeping this map as member saves us from generating a sorted list of remote register numbers.

Yes, but there's no reason that this _must_ be done in AddRegister, is there?

You could just build a local map directly inside the finalize function, right?

Yes this just avoid an extra iteration over m_regs list.

omjavaid updated this revision to Diff 308339.Nov 30 2020, 5:43 AM

Updated after incorporating suggested changes.

labath accepted this revision.Nov 30 2020, 5:56 AM

Yes, but there's no reason that this _must_ be done in AddRegister, is there?

You could just build a local map directly inside the finalize function, right?

Yes this just avoid an extra iteration over m_regs list.

This is much better. The iteration doesn't cost much (if anything), but the member variable increases size of the object permentently.

This revision is now accepted and ready to land.Nov 30 2020, 5:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 2:20 PM