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

[omjavaid]

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.

[labath]

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).

[omjavaid]

In D91241#2391072, @labath wrote:

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..

[labath]

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).

[omjavaid]

In D91241#2391126, @labath wrote:

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

[labath]

In D91241#2391156, @omjavaid wrote:

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...

[omjavaid]

In D91241#2391245, @labath wrote:

In D91241#2391156, @omjavaid wrote:

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.

[labath]

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 D91241#2396445, @omjavaid wrote:

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]

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

[omjavaid]

In D91241#2397083, @labath wrote:

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 D91241#2396445, @omjavaid wrote:

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.

[labath]

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)?

In D91241#2398995, @omjavaid wrote:

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.

[omjavaid]

In D91241#2402491, @labath wrote:

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)?

In D91241#2398995, @omjavaid wrote:

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]

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.

[labath]

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?

[omjavaid]

In D91241#2422100, @labath wrote:

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.

[labath]

In D91241#2422403, @omjavaid wrote:

In D91241#2422100, @labath wrote:

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?

[omjavaid]

In D91241#2422517, @labath wrote:

In D91241#2422403, @omjavaid wrote:

In D91241#2422100, @labath wrote:

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 after incorporating suggested changes.

[labath]

In D91241#2422571, @omjavaid wrote:

In D91241#2422517, @labath wrote:

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.

lldb/include/lldb/Host/common/NativeRegisterContext.h

public:
   // Subclasses should not override these
   virtual lldb::tid_t GetThreadID() const;
 
   virtual NativeThreadProtocol &GetThread() { return m_thread; }
 
   virtual std::vector<uint32_t>
   GetExpeditedRegisters(ExpeditedRegs expType) const;
 
+  virtual bool RegisterOffsetIsDynamic() const { return false; }
+
   const RegisterInfo *GetRegisterInfoByName(llvm::StringRef reg_name,
                                             uint32_t start_idx = 0);
 
   const RegisterInfo *GetRegisterInfo(uint32_t reg_kind, uint32_t reg_num);
 
   lldb::addr_t GetPC(lldb::addr_t fail_value = LLDB_INVALID_ADDRESS);
 
   virtual lldb::addr_t

lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py

class GdbRemoteTestCaseBase(TestBase):
     def assert_valid_reg_info(self, reg_info):
         # Assert we know about all the reginfo keys parsed.
         for key in reg_info:
             self.assertTrue(key in self._KNOWN_REGINFO_KEYS)
 
         # Check the bare-minimum expected set of register info keys.
         self.assertTrue("name" in reg_info)
         self.assertTrue("bitsize" in reg_info)
+
+        if not self.getArchitecture() == 'aarch64':
             self.assertTrue("offset" in reg_info)
+
         self.assertTrue("encoding" in reg_info)
         self.assertTrue("format" in reg_info)
 
     def find_pc_reg_info(self, reg_infos):
         lldb_reg_index = 0
         for reg_info in reg_infos:
             if ("generic" in reg_info) and (reg_info["generic"] == "pc"):
                 return (lldb_reg_index, reg_info)

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

public:
 
   Status WriteAllRegisterValues(const lldb::DataBufferSP &data_sp) override;
 
   void InvalidateAllRegisters() override;
 
   std::vector<uint32_t>
   GetExpeditedRegisters(ExpeditedRegs expType) const override;
 
+  bool RegisterOffsetIsDynamic() const override { return true; }
+
   // Hardware breakpoints/watchpoint management functions
 
   uint32_t NumSupportedHardwareBreakpoints() override;
 
   uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override;
 
   bool ClearHardwareBreakpoint(uint32_t hw_idx) override;
 

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

void DynamicRegisterInfo::AddRegister(RegisterInfo &reg_info,
   }
 
   m_regs.push_back(reg_info);
   uint32_t set = GetRegisterSetIndexByName(set_name, true);
   assert(set < m_sets.size());
   assert(set < m_set_reg_nums.size());
   assert(set < m_set_names.size());
   m_set_reg_nums[set].push_back(reg_num);
-  size_t end_reg_offset = reg_info.byte_offset + reg_info.byte_size;
-  if (m_reg_data_byte_size < end_reg_offset)
-    m_reg_data_byte_size = end_reg_offset;
 }
 
 void DynamicRegisterInfo::Finalize(const ArchSpec &arch) {
   if (m_finalized)
     return;
 
   m_finalized = true;
   const size_t num_sets = m_sets.size();
   for (size_t set = 0; set < num_sets; ++set) {
     assert(m_sets.size() == m_set_reg_nums.size());
     m_sets[set].num_registers = m_set_reg_nums[set].size();
     m_sets[set].registers = m_set_reg_nums[set].data();
   }
 
+  // We are going to create a map between remote (eRegisterKindProcessPlugin)
+  // and local (eRegisterKindLLDB) register numbers. This map will give us
+  // remote register numbers in increasing order for offset calculation.
+  std::map<uint32_t, uint32_t> remote_to_local_regnum_map;
+  for (const auto &reg : m_regs)
+    remote_to_local_regnum_map[reg.kinds[eRegisterKindProcessPlugin]] =
+        reg.kinds[eRegisterKindLLDB];
+
+  // At this stage we manually calculate g/G packet offsets of all primary
+  // registers, only if target XML or qRegisterInfo packet did not send
+  // an offset explicitly.
+  uint32_t reg_offset = 0;
+  for (auto const &regnum_pair : remote_to_local_regnum_map) {
+    if (m_regs[regnum_pair.second].byte_offset == LLDB_INVALID_INDEX32 &&
+        m_regs[regnum_pair.second].value_regs == nullptr) {
+      m_regs[regnum_pair.second].byte_offset = reg_offset;
+
+      reg_offset = m_regs[regnum_pair.second].byte_offset +
+                   m_regs[regnum_pair.second].byte_size;
+    }
+  }
+
   // sort and unique all value registers and make sure each is terminated with
   // LLDB_INVALID_REGNUM
 
   for (reg_to_regs_map::iterator pos = m_value_regs_map.begin(),
                                  end = m_value_regs_map.end();
        pos != end; ++pos) {
     if (pos->second.size() > 1) {
       llvm::sort(pos->second.begin(), pos->second.end());
       reg_num_collection::iterator unique_end =
           std::unique(pos->second.begin(), pos->second.end());
       if (unique_end != pos->second.end())
         pos->second.erase(unique_end, pos->second.end());
     }
     assert(!pos->second.empty());
     if (pos->second.back() != LLDB_INVALID_REGNUM)
       pos->second.push_back(LLDB_INVALID_REGNUM);
   }
 
   // Now update all value_regs with each register info as needed
   const size_t num_regs = m_regs.size();
   for (size_t i = 0; i < num_regs; ++i) {
-    if (m_value_regs_map.find(i) != m_value_regs_map.end())
+    if (m_value_regs_map.find(i) != m_value_regs_map.end()) {
       m_regs[i].value_regs = m_value_regs_map[i].data();
-    else
+      // Assign a valid offset to all pseudo registers if not assigned by stub.
+      // Pseudo registers with value_regs list populated will share same offset
+      // as that of their corresponding primary register in value_regs list.
+      if (m_regs[i].byte_offset == LLDB_INVALID_INDEX32) {
+        uint32_t value_regnum = m_regs[i].value_regs[0];
+        if (value_regnum != LLDB_INVALID_INDEX32)
+          m_regs[i].byte_offset =
+              GetRegisterInfoAtIndex(remote_to_local_regnum_map[value_regnum])
+                  ->byte_offset;
+      }
+    } else
       m_regs[i].value_regs = nullptr;
+
+    reg_offset = m_regs[i].byte_offset + m_regs[i].byte_size;
+    if (m_reg_data_byte_size < reg_offset)
+      m_reg_data_byte_size = reg_offset;
   }
 
   // Expand all invalidation dependencies
   for (reg_to_regs_map::iterator pos = m_invalidate_regs_map.begin(),
                                  end = m_invalidate_regs_map.end();
        pos != end; ++pos) {
     const uint32_t reg_num = pos->first;
 
void DynamicRegisterInfo::Finalize(const ArchSpec &arch) {
   bool generic_regs_specified = false;
   for (const auto &reg : m_regs) {
     if (reg.kinds[eRegisterKindGeneric] != LLDB_INVALID_REGNUM) {
       generic_regs_specified = true;
       break;
     }
   }
 
   if (!generic_regs_specified) {
     switch (arch.GetMachine()) {
     case llvm::Triple::aarch64:
     case llvm::Triple::aarch64_32:
     case llvm::Triple::aarch64_be:
       for (auto &reg : m_regs) {
         if (strcmp(reg.name, "pc") == 0)
           reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_PC;
         else if ((strcmp(reg.name, "fp") == 0) ||

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

GDBRemoteCommunicationServerLLGS::Handle_qRegisterInfo(
   response.PutChar(';');
 
   if (reg_info->alt_name && reg_info->alt_name[0]) {
     response.PutCString("alt-name:");
     response.PutCString(reg_info->alt_name);
     response.PutChar(';');
   }
 
-  response.Printf("bitsize:%" PRIu32 ";offset:%" PRIu32 ";",
-                  reg_info->byte_size * 8, reg_info->byte_offset);
+  response.Printf("bitsize:%" PRIu32 ";", reg_info->byte_size * 8);
+
+  if (!reg_context.RegisterOffsetIsDynamic())
+    response.Printf("offset:%" PRIu32 ";", reg_info->byte_offset);
 
   llvm::StringRef encoding = GetEncodingNameOrEmpty(*reg_info);
   if (!encoding.empty())
     response << "encoding:" << encoding << ';';
 
   llvm::StringRef format = GetFormatNameOrEmpty(*reg_info);
   if (!format.empty())
     response << "format:" << format << ';';
for (int reg_index = 0; reg_index < registers_count; reg_index++) {
 
     if (!reg_info) {
       LLDB_LOGF(log,
                 "%s failed to get register info for register index %" PRIu32,
                 "target.xml", reg_index);
       continue;
     }
 
-    response.Printf("<reg name=\"%s\" bitsize=\"%" PRIu32 "\" offset=\"%" PRIu32
-                    "\" regnum=\"%d\" ",
-                    reg_info->name, reg_info->byte_size * 8,
-                    reg_info->byte_offset, reg_index);
+    response.Printf("<reg name=\"%s\" bitsize=\"%" PRIu32 "\" regnum=\"%d\" ",
+                    reg_info->name, reg_info->byte_size * 8, reg_index);
+
+    if (!reg_context.RegisterOffsetIsDynamic())
+      response.Printf("offset=\"%" PRIu32 "\" ", reg_info->byte_offset);
 
     if (reg_info->alt_name && reg_info->alt_name[0])
       response.Printf("altname=\"%s\" ", reg_info->alt_name);
 
     llvm::StringRef encoding = GetEncodingNameOrEmpty(*reg_info);
     if (!encoding.empty())
       response << "encoding=\"" << encoding << "\" ";
 

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

protected:
 
   const char *GetDispatchQueueNameForThread(lldb::addr_t thread_dispatch_qaddr,
                                             std::string &dispatch_queue_name);
 
   DynamicLoader *GetDynamicLoader() override;
 
   bool GetGDBServerRegisterInfoXMLAndProcess(ArchSpec &arch_to_use,
                                              std::string xml_filename,
-                                             uint32_t &cur_reg_num,
-                                             uint32_t &reg_offset);
+                                             uint32_t &cur_reg_remote,
+                                             uint32_t &cur_reg_local);
 
   // Query remote GDBServer for register information
   bool GetGDBServerRegisterInfo(ArchSpec &arch);
 
   lldb::ModuleSP LoadModuleAtAddress(const FileSpec &file,
                                      lldb::addr_t link_map,
                                      lldb::addr_t base_addr,
                                      bool value_is_offset);

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

void ProcessGDBRemote::BuildDynamicRegisterInfo(bool force) {
 
   if (!arch_to_use.IsValid())
     arch_to_use = target_arch;
 
   if (GetGDBServerRegisterInfo(arch_to_use))
     return;
 
   char packet[128];
-  uint32_t reg_offset = 0;
+  uint32_t reg_offset = LLDB_INVALID_INDEX32;
   uint32_t reg_num = 0;
   for (StringExtractorGDBRemote::ResponseType response_type =
            StringExtractorGDBRemote::eResponse;
        response_type == StringExtractorGDBRemote::eResponse; ++reg_num) {
     const int packet_len =
         ::snprintf(packet, sizeof(packet), "qRegisterInfo%x", reg_num);
     assert(packet_len < (int)sizeof(packet));
     UNUSED_IF_ASSERT_DISABLED(packet_len);
if (m_gdb_comm.SendPacketAndWaitForResponse(packet, response, false) ==
         ConstString set_name;
         std::vector<uint32_t> value_regs;
         std::vector<uint32_t> invalidate_regs;
         std::vector<uint8_t> dwarf_opcode_bytes;
         RegisterInfo reg_info = {
             nullptr,       // Name
             nullptr,       // Alt name
             0,             // byte size
             reg_offset,    // offset
             eEncodingUint, // encoding
             eFormatHex,    // format
             {
                 LLDB_INVALID_REGNUM, // eh_frame reg num
                 LLDB_INVALID_REGNUM, // DWARF reg num
                 LLDB_INVALID_REGNUM, // generic reg num
                 reg_num,             // process plugin reg num
                 reg_num              // native register number
if (m_gdb_comm.SendPacketAndWaitForResponse(packet, response, false) ==
             assert(dwarf_opcode_len == ret_val);
             UNUSED_IF_ASSERT_DISABLED(ret_val);
             reg_info.dynamic_size_dwarf_expr_bytes = dwarf_opcode_bytes.data();
           }
         }
 
         reg_info.byte_offset = reg_offset;
         assert(reg_info.byte_size != 0);
-        reg_offset += reg_info.byte_size;
+        reg_offset = LLDB_INVALID_INDEX32;
         if (!value_regs.empty()) {
           value_regs.push_back(LLDB_INVALID_REGNUM);
           reg_info.value_regs = value_regs.data();
         }
         if (!invalidate_regs.empty()) {
           invalidate_regs.push_back(LLDB_INVALID_REGNUM);
           reg_info.invalidate_regs = invalidate_regs.data();
         }
 
         reg_info.name = reg_name.AsCString();
         // We have to make a temporary ABI here, and not use the GetABI because
         // this code gets called in DidAttach, when the target architecture
         // (and consequently the ABI we'll get from the process) may be wrong.
         if (ABISP abi_sp = ABI::FindPlugin(shared_from_this(), arch_to_use))
           abi_sp->AugmentRegisterInfo(reg_info);
struct GdbServerTargetInfo {
   std::string arch;
   std::string osabi;
   stringVec includes;
   RegisterSetMap reg_set_map;
 };
 
 bool ParseRegisters(XMLNode feature_node, GdbServerTargetInfo &target_info,
                     GDBRemoteDynamicRegisterInfo &dyn_reg_info, ABISP abi_sp,
-                    uint32_t &cur_reg_num, uint32_t &reg_offset) {
+                    uint32_t &reg_num_remote, uint32_t &reg_num_local) {
   if (!feature_node)
     return false;
 
+  uint32_t reg_offset = LLDB_INVALID_INDEX32;
   feature_node.ForEachChildElementWithName(
-      "reg",
-      [&target_info, &dyn_reg_info, &cur_reg_num, &reg_offset,
-       &abi_sp](const XMLNode &reg_node) -> bool {
+      "reg", [&target_info, &dyn_reg_info, &reg_num_remote, &reg_num_local,
+              &reg_offset, &abi_sp](const XMLNode &reg_node) -> bool {
         std::string gdb_group;
         std::string gdb_type;
         ConstString reg_name;
         ConstString alt_name;
         ConstString set_name;
         std::vector<uint32_t> value_regs;
         std::vector<uint32_t> invalidate_regs;
         std::vector<uint8_t> dwarf_opcode_bytes;
         bool encoding_set = false;
         bool format_set = false;
         RegisterInfo reg_info = {
             nullptr,       // Name
             nullptr,       // Alt name
             0,             // byte size
             reg_offset,    // offset
             eEncodingUint, // encoding
             eFormatHex,    // format
             {
                 LLDB_INVALID_REGNUM, // eh_frame reg num
                 LLDB_INVALID_REGNUM, // DWARF reg num
                 LLDB_INVALID_REGNUM, // generic reg num
-                cur_reg_num,         // process plugin reg num
-                cur_reg_num          // native register number
+                reg_num_remote,      // process plugin reg num
+                reg_num_local        // native register number
             },
             nullptr,
             nullptr,
             nullptr, // Dwarf Expression opcode bytes pointer
             0        // Dwarf Expression opcode bytes length
         };
 
         reg_node.ForEachAttribute([&target_info, &gdb_group, &gdb_type,
feature_node.ForEachChildElementWithName(
             // If no register group name provided anywhere,
             // we'll create a 'general' register set
             set_name.SetCString("general");
           }
         }
 
         reg_info.byte_offset = reg_offset;
         assert(reg_info.byte_size != 0);
-        reg_offset += reg_info.byte_size;
+        reg_offset = LLDB_INVALID_INDEX32;
         if (!value_regs.empty()) {
           value_regs.push_back(LLDB_INVALID_REGNUM);
           reg_info.value_regs = value_regs.data();
         }
         if (!invalidate_regs.empty()) {
           invalidate_regs.push_back(LLDB_INVALID_REGNUM);
           reg_info.invalidate_regs = invalidate_regs.data();
         }
 
-        ++cur_reg_num;
+        reg_num_remote = reg_info.kinds[eRegisterKindProcessPlugin] + 1;
+        ++reg_num_local;
         reg_info.name = reg_name.AsCString();
         if (abi_sp)
           abi_sp->AugmentRegisterInfo(reg_info);
         dyn_reg_info.AddRegister(reg_info, reg_name, alt_name, set_name);
 
         return true; // Keep iterating through all "reg" elements
       });
   return true;
 }
 
 } // namespace
 
 // This method fetches a register description feature xml file from
 // the remote stub and adds registers/register groupsets/architecture
 // information to the current process.  It will call itself recursively
 // for nested register definition files.  It returns true if it was able
 // to fetch and parse an xml file.
 bool ProcessGDBRemote::GetGDBServerRegisterInfoXMLAndProcess(
-    ArchSpec &arch_to_use, std::string xml_filename, uint32_t &cur_reg_num,
-    uint32_t &reg_offset) {
+    ArchSpec &arch_to_use, std::string xml_filename, uint32_t &reg_num_remote,
+    uint32_t &reg_num_local) {
   // request the target xml file
   std::string raw;
   lldb_private::Status lldberr;
   if (!m_gdb_comm.ReadExtFeature(ConstString("features"),
                                  ConstString(xml_filename.c_str()), raw,
                                  lldberr)) {
     return false;
   }
if (xml_document.ParseMemory(raw.c_str(), raw.size(), xml_filename.c_str())) {
 
     if (arch_to_use.IsValid()) {
       // Don't use Process::GetABI, this code gets called from DidAttach, and
       // in that context we haven't set the Target's architecture yet, so the
       // ABI is also potentially incorrect.
       ABISP abi_to_use_sp = ABI::FindPlugin(shared_from_this(), arch_to_use);
       for (auto &feature_node : feature_nodes) {
         ParseRegisters(feature_node, target_info, this->m_register_info,
-                       abi_to_use_sp, cur_reg_num, reg_offset);
+                       abi_to_use_sp, reg_num_remote, reg_num_local);
       }
 
       for (const auto &include : target_info.includes) {
-        GetGDBServerRegisterInfoXMLAndProcess(arch_to_use, include, cur_reg_num,
-                                              reg_offset);
+        GetGDBServerRegisterInfoXMLAndProcess(arch_to_use, include,
+                                              reg_num_remote, reg_num_local);
       }
     }
   } else {
     return false;
   }
   return true;
 }
 
 // query the target of gdb-remote for extended target information returns
 // true on success (got register definitions), false on failure (did not).
 bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
   // Make sure LLDB has an XML parser it can use first
   if (!XMLDocument::XMLEnabled())
     return false;
 
   // check that we have extended feature read support
   if (!m_gdb_comm.GetQXferFeaturesReadSupported())
     return false;
 
-  uint32_t cur_reg_num = 0;
-  uint32_t reg_offset = 0;
-  if (GetGDBServerRegisterInfoXMLAndProcess (arch_to_use, "target.xml", cur_reg_num, reg_offset))
+  uint32_t reg_num_remote = 0;
+  uint32_t reg_num_local = 0;
+  if (GetGDBServerRegisterInfoXMLAndProcess(arch_to_use, "target.xml",
+                                            reg_num_remote, reg_num_local))
     this->m_register_info.Finalize(arch_to_use);
 
   return m_register_info.GetNumRegisters() > 0;
 }
 
 llvm::Expected<LoadedModuleInfoList> ProcessGDBRemote::GetLoadedModuleList() {
   // Make sure LLDB has an XML parser it can use first
   if (!XMLDocument::XMLEnabled())

lldb/test/API/functionalities/gdb_remote_client/TestAArch64XMLRegOffsets.py

+from __future__ import print_function
+from textwrap import dedent
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class MyResponder(MockGDBServerResponder):
+    def qXferRead(self, obj, annex, offset, length):
+        if annex == "target.xml":
+            return dedent("""\
+                <?xml version="1.0"?>
+                  <target version="1.0">
+                    <architecture>aarch64</architecture>
+                    <feature name="org.gnu.gdb.aarch64.core">
+                      <reg name="cpsr" regnum="33" bitsize="32"/>
+                      <reg name="x0" regnum="0" bitsize="64"/>
+                      <reg name="x1" bitsize="64"/>
+                      <reg name="x2" bitsize="64"/>
+                      <reg name="x3" bitsize="64"/>
+                      <reg name="x4" bitsize="64"/>
+                      <reg name="x5" bitsize="64"/>
+                      <reg name="x6" bitsize="64"/>
+                      <reg name="x7" bitsize="64"/>
+                      <reg name="x8" bitsize="64"/>
+                      <reg name="x9" bitsize="64"/>
+                      <reg name="x10" bitsize="64"/>
+                      <reg name="x11" bitsize="64"/>
+                      <reg name="x12" bitsize="64"/>
+                      <reg name="x13" bitsize="64"/>
+                      <reg name="x14" bitsize="64"/>
+                      <reg name="x15" bitsize="64"/>
+                      <reg name="x16" bitsize="64"/>
+                      <reg name="x17" bitsize="64"/>
+                      <reg name="x18" bitsize="64"/>
+                      <reg name="x19" bitsize="64"/>
+                      <reg name="x20" bitsize="64"/>
+                      <reg name="x21" bitsize="64"/>
+                      <reg name="x22" bitsize="64"/>
+                      <reg name="x23" bitsize="64"/>
+                      <reg name="x24" bitsize="64"/>
+                      <reg name="x25" bitsize="64"/>
+                      <reg name="x26" bitsize="64"/>
+                      <reg name="x27" bitsize="64"/>
+                      <reg name="x28" bitsize="64"/>
+                      <reg name="x29" bitsize="64"/>
+                      <reg name="x30" bitsize="64"/>
+                      <reg name="sp" bitsize="64"/>
+                      <reg name="pc" bitsize="64"/>
+                      <reg name="w0" bitsize="32" value_regnums="0" invalidate_regnums="0" regnum="34"/>
+                      <reg name="w1" bitsize="32" value_regnums="1" invalidate_regnums="1"/>
+                      <reg name="w2" bitsize="32" value_regnums="2" invalidate_regnums="2"/>
+                      <reg name="w3" bitsize="32" value_regnums="3" invalidate_regnums="3"/>
+                      <reg name="w4" bitsize="32" value_regnums="4" invalidate_regnums="4"/>
+                      <reg name="w5" bitsize="32" value_regnums="5" invalidate_regnums="5"/>
+                      <reg name="w6" bitsize="32" value_regnums="6" invalidate_regnums="6"/>
+                      <reg name="w7" bitsize="32" value_regnums="7" invalidate_regnums="7"/>
+                      <reg name="w8" bitsize="32" value_regnums="8" invalidate_regnums="8"/>
+                      <reg name="w9" bitsize="32" value_regnums="9" invalidate_regnums="9"/>
+                      <reg name="w10" bitsize="32" value_regnums="10" invalidate_regnums="10"/>
+                      <reg name="w11" bitsize="32" value_regnums="11" invalidate_regnums="11"/>
+                      <reg name="w12" bitsize="32" value_regnums="12" invalidate_regnums="12"/>
+                      <reg name="w13" bitsize="32" value_regnums="13" invalidate_regnums="13"/>
+                      <reg name="w14" bitsize="32" value_regnums="14" invalidate_regnums="14"/>
+                      <reg name="w15" bitsize="32" value_regnums="15" invalidate_regnums="15"/>
+                      <reg name="w16" bitsize="32" value_regnums="16" invalidate_regnums="16"/>
+                      <reg name="w17" bitsize="32" value_regnums="17" invalidate_regnums="17"/>
+                      <reg name="w18" bitsize="32" value_regnums="18" invalidate_regnums="18"/>
+                      <reg name="w19" bitsize="32" value_regnums="19" invalidate_regnums="19"/>
+                      <reg name="w20" bitsize="32" value_regnums="20" invalidate_regnums="20"/>
+                      <reg name="w21" bitsize="32" value_regnums="21" invalidate_regnums="21"/>
+                      <reg name="w22" bitsize="32" value_regnums="22" invalidate_regnums="22"/>
+                      <reg name="w23" bitsize="32" value_regnums="23" invalidate_regnums="23"/>
+                      <reg name="w24" bitsize="32" value_regnums="24" invalidate_regnums="24"/>
+                      <reg name="w25" bitsize="32" value_regnums="25" invalidate_regnums="25"/>
+                      <reg name="w26" bitsize="32" value_regnums="26" invalidate_regnums="26"/>
+                      <reg name="w27" bitsize="32" value_regnums="27" invalidate_regnums="27"/>
+                      <reg name="w28" bitsize="32" value_regnums="28" invalidate_regnums="28"/>
+                    </feature>
+                  </target>
+                """), False
+        else:
+            return None,
+
+    def readRegister(self, regnum):
+        return "E01"
+
+    def readRegisters(self):
+        return "20000000000000002000000000000000f0c154bfffff00005daa985a8fea0b48f0b954bfffff0000ad13cce570150b48380000000000000070456abfffff0000a700000000000000000000000000000001010101010101010000000000000000f0c154bfffff00000f2700000000000008e355bfffff0000080e55bfffff0000281041000000000010de61bfffff00005c05000000000000f0c154bfffff000090fcffffffff00008efcffffffff00008ffcffffffff00000000000000000000001000000000000090fcffffffff000000d06cbfffff0000f0c154bfffff00000100000000000000d0b954bfffff0000e407400000000000d0b954bfffff0000e40740000000000000100000"
+
+
+class TestAArch64XMLRegOffsets(GDBRemoteTestBase):
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    @skipIfLLVMTargetMissing("AArch64")
+    def test_register_gpacket_offsets(self):
+        """
+        Test that we correctly associate the register info with the eh_frame
+        register numbers.
+        """
+
+        target = self.createTarget("basic_eh_frame-aarch64.yaml")
+        self.server.responder = MyResponder()
+
+        if self.TraceOn():
+            self.runCmd("log enable gdb-remote packets")
+            self.addTearDownHook(
+                lambda: self.runCmd("log disable gdb-remote packets"))
+
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateStopped])
+
+        registerSet = process.GetThreadAtIndex(
+            0).GetFrameAtIndex(0).GetRegisters().GetValueAtIndex(0)
+
+        reg_val_dict = {
+            "x0": 0x0000000000000020, "x1": 0x0000000000000020,
+            "x2": 0x0000ffffbf54c1f0, "x3": 0x480bea8f5a98aa5d,
+            "x4": 0x0000ffffbf54b9f0, "x5": 0x480b1570e5cc13ad,
+            "x6": 0x0000000000000038, "x7": 0x0000ffffbf6a4570,
+            "x8": 0x00000000000000a7, "x9": 0x0000000000000000,
+            "x10": 0x0101010101010101, "x11": 0x0000000000000000,
+            "x12": 0x0000ffffbf54c1f0, "x13": 0x000000000000270f,
+            "x14": 0x0000ffffbf55e308, "x15": 0x0000ffffbf550e08,
+            "x16": 0x0000000000411028, "x17": 0x0000ffffbf61de10,
+            "x18": 0x000000000000055c, "x19": 0x0000ffffbf54c1f0,
+            "x20": 0x0000fffffffffc90, "x21": 0x0000fffffffffc8e,
+            "x22": 0x0000fffffffffc8f, "x23": 0x0000000000000000,
+            "x24": 0x0000000000001000, "x25": 0x0000fffffffffc90,
+            "x26": 0x0000ffffbf6cd000, "x27": 0x0000ffffbf54c1f0,
+            "x28": 0x0000000000000001, "x29": 0x0000ffffbf54b9d0,
+            "x30": 0x00000000004007e4, "sp": 0x0000ffffbf54b9d0,
+            "pc": 0x00000000004007e4, "cpsr": 0x00001000, "w0": 0x00000020,
+            "w1": 0x00000020, "w2": 0xbf54c1f0, "w3": 0x5a98aa5d,
+            "w4": 0xbf54b9f0, "w5": 0xe5cc13ad, "w6": 0x00000038,
+            "w7": 0xbf6a4570, "w8": 0x000000a7, "w9": 0x00000000,
+            "w10": 0x01010101, "w11": 0x00000000, "w12": 0xbf54c1f0,
+            "w13": 0x0000270f, "w14": 0xbf55e308, "w15": 0xbf550e08,
+            "w16": 0x00411028, "w17": 0xbf61de10, "w18": 0x0000055c,
+            "w19": 0xbf54c1f0, "w20": 0xfffffc90, "w21": 0xfffffc8e,
+            "w22": 0xfffffc8f, "w23": 0x00000000, "w24": 0x00001000,
+            "w25": 0xfffffc90, "w26": 0xbf6cd000, "w27": 0xbf54c1f0,
+            "w28": 0x00000001
+        }
+
+        for reg in registerSet:
+            self.assertEqual(reg.GetValueAsUnsigned(),
+                             reg_val_dict[reg.GetName()])

lldb/test/API/tools/lldb-server/registers-target-xml-reading/TestGdbRemoteTargetXmlPacket.py

def test_g_target_xml_returns_correct_data(self):
         self.assertTrue(len(target_xml_registers) == len(q_info_registers))
         for register in zip(target_xml_registers, q_info_registers):
             xml_info_reg = register[0]
             q_info_reg = register[1]
             self.assertEqual(q_info_reg["name"], xml_info_reg.get("name"))
             self.assertEqual(q_info_reg["set"], xml_info_reg.get("group"))
             self.assertEqual(q_info_reg["format"], xml_info_reg.get("format"))
             self.assertEqual(q_info_reg["bitsize"], xml_info_reg.get("bitsize"))
+
+            if not self.getArchitecture() == 'aarch64':
                 self.assertEqual(q_info_reg["offset"], xml_info_reg.get("offset"))
+
             self.assertEqual(q_info_reg["encoding"], xml_info_reg.get("encoding"))

lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp

Expected<RegisterInfo> RegisterInfoParser::create(StringRef Response) {
 
   Info.alt_name = ConstString(Elements["alt-name"]).GetCString();
 
   if (!to_integer(Elements["bitsize"], Info.byte_size, 10))
     return make_parsing_error("qRegisterInfo: bit-size");
   Info.byte_size /= CHAR_BIT;
 
   if (!to_integer(Elements["offset"], Info.byte_offset, 10))
-    return make_parsing_error("qRegisterInfo: offset");
+    Info.byte_offset = LLDB_INVALID_INDEX32;
 
   Info.encoding = Args::StringToEncoding(Elements["encoding"]);
   if (Info.encoding == eEncodingInvalid)
     return make_parsing_error("qRegisterInfo: encoding");
 
   Info.format = StringSwitch<Format>(Elements["format"])
                     .Case("binary", eFormatBinary)
                     .Case("decimal", eFormatDecimal)

lldb/unittests/tools/lldb-server/tests/TestClient.cpp

Error TestClient::qProcessInfo() {
   auto InfoOr = SendMessage<ProcessInfo>("qProcessInfo");
   if (!InfoOr)
     return InfoOr.takeError();
   m_process_info = std::move(*InfoOr);
   return Error::success();
 }
 
 Error TestClient::qRegisterInfos() {
+  uint32_t reg_offset = 0;
   for (unsigned int Reg = 0;; ++Reg) {
     std::string Message = formatv("qRegisterInfo{0:x-}", Reg).str();
     Expected<RegisterInfo> InfoOr = SendMessage<RegisterInfoParser>(Message);
     if (!InfoOr) {
       consumeError(InfoOr.takeError());
       break;
     }
     m_register_infos.emplace_back(std::move(*InfoOr));
+
+    if (m_register_infos[Reg].byte_offset == LLDB_INVALID_INDEX32)
+      m_register_infos[Reg].byte_offset = reg_offset;
+
+    reg_offset =
+        m_register_infos[Reg].byte_offset + m_register_infos[Reg].byte_size;
     if (m_register_infos[Reg].kinds[eRegisterKindGeneric] ==
         LLDB_REGNUM_GENERIC_PC)
       m_pc_register = Reg;
   }
   if (m_pc_register == LLDB_INVALID_REGNUM)
     return make_parsing_error("qRegisterInfo: generic");
   return Error::success();
 }