omjavaid created this revision.
omjavaid added reviewers: labath, clayborg.
Herald added a subscriber: kristof.beyls.

This patch simplifies register accesses in NativeRegisterContextLinux_arm64 and 
also adds some bare minimum caching to avoid multiple calls to ptrace during a 
stop.

Linux ptrace returns data in the form of structures containing GPR/FPR data. 
This means that one single call is enough to read all GPRs or FPRs. We do that 
once per stop and keep reading from or writing to the buffer that we have in 
NativeRegisterContextLinux_arm64 class. Before a resume or detach we write all 
buffers back.

This is tested on aarch64 thunder x1 with Ubuntu 18.04. Also tested regressions 
on x86_64.


https://reviews.llvm.org/D69371

Files:
  include/lldb/Host/common/NativeRegisterContext.h
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
===================================================================
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
@@ -40,6 +40,8 @@
 
   Status WriteAllRegisterValues(const lldb::DataBufferSP &data_sp) override;
 
+  Status InvalidateAllRegisters() override;
+
   // Hardware breakpoints/watchpoint management functions
 
   uint32_t NumSupportedHardwareBreakpoints() override;
@@ -77,11 +79,6 @@
   enum DREGType { eDREGTypeWATCH = 0, eDREGTypeBREAK };
 
 protected:
-  Status DoReadRegisterValue(uint32_t offset, const char *reg_name,
-                             uint32_t size, RegisterValue &value) override;
-
-  Status DoWriteRegisterValue(uint32_t offset, const char *reg_name,
-                              const RegisterValue &value) override;
 
   Status ReadGPR() override;
 
@@ -125,8 +122,14 @@
     uint32_t fpcr;
   };
 
-  uint64_t m_gpr_arm64[k_num_gpr_registers_arm64]; // 64-bit general purpose
-                                                   // registers.
+  struct GPR {
+    uint64_t x[31];
+    uint64_t sp;
+    uint64_t pc;
+    uint64_t pstate;
+  };
+
+  GPR m_gpr_arm64; // 64-bit general purpose registers.
   RegInfo m_reg_info;
   FPU m_fpr; // floating-point registers including extended register sets.
 
@@ -147,6 +150,9 @@
   uint32_t m_max_hbp_supported;
   bool m_refresh_hwdebug_info;
 
+  bool m_gpr_is_valid;
+  bool m_fpu_is_valid;
+
   bool IsGPR(unsigned reg) const;
 
   bool IsFPR(unsigned reg) const;
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
===================================================================
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -152,6 +152,9 @@
   m_max_hwp_supported = 16;
   m_max_hbp_supported = 16;
   m_refresh_hwdebug_info = true;
+
+  m_gpr_is_valid = false;
+  m_fpu_is_valid = false;
 }
 
 uint32_t NativeRegisterContextLinux_arm64::GetRegisterSetCount() const {
@@ -185,41 +188,36 @@
 
   const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
 
-  if (IsFPR(reg)) {
-    error = ReadFPR();
+  if (reg == LLDB_INVALID_REGNUM)
+    return Status("no lldb regnum for %s", reg_info && reg_info->name
+                                               ? reg_info->name
+                                               : "<unknown register>");
+
+  uint8_t *src;
+  uint32_t offset;
+
+  if (IsGPR(reg)) {
+    error = ReadGPR();
     if (error.Fail())
       return error;
-  } else {
-    uint32_t full_reg = reg;
-    bool is_subreg = reg_info->invalidate_regs &&
-                     (reg_info->invalidate_regs[0] != LLDB_INVALID_REGNUM);
-
-    if (is_subreg) {
-      // Read the full aligned 64-bit register.
-      full_reg = reg_info->invalidate_regs[0];
-    }
 
-    error = ReadRegisterRaw(full_reg, reg_value);
+    offset = reg_info->byte_offset;
+    assert(offset < GetGPRSize());
+    src = (uint8_t *)GetGPRBuffer() + offset;
 
-    if (error.Success()) {
-      // If our read was not aligned (for ah,bh,ch,dh), shift our returned
-      // value one byte to the right.
-      if (is_subreg && (reg_info->byte_offset & 0x1))
-        reg_value.SetUInt64(reg_value.GetAsUInt64() >> 8);
-
-      // If our return byte size was greater than the return value reg size,
-      // then use the type specified by reg_info rather than the uint64_t
-      // default
-      if (reg_value.GetByteSize() > reg_info->byte_size)
-        reg_value.SetType(reg_info);
-    }
-    return error;
-  }
+  } else if (IsFPR(reg)) {
+
+    error = ReadFPR();
+    if (error.Fail())
+      return error;
+
+    offset = CalculateFprOffset(reg_info);
+    assert(offset < GetFPRSize());
+    src = (uint8_t *)GetFPRBuffer() + offset;
+  } else
+    return Status("failed - register wasn't recognized to be a GPR or an FPR, "
+                  "write strategy unknown");
 
-  // Get pointer to m_fpr variable and set the data from it.
-  uint32_t fpr_offset = CalculateFprOffset(reg_info);
-  assert(fpr_offset < sizeof m_fpr);
-  uint8_t *src = (uint8_t *)&m_fpr + fpr_offset;
   reg_value.SetFromMemoryData(reg_info, src, reg_info->byte_size,
                               eByteOrderLittle, error);
 
@@ -228,54 +226,54 @@
 
 Status NativeRegisterContextLinux_arm64::WriteRegister(
     const RegisterInfo *reg_info, const RegisterValue &reg_value) {
+  Status error;
+
   if (!reg_info)
     return Status("reg_info NULL");
 
-  const uint32_t reg_index = reg_info->kinds[lldb::eRegisterKindLLDB];
-  if (reg_index == LLDB_INVALID_REGNUM)
+  const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+
+  if (reg == LLDB_INVALID_REGNUM)
     return Status("no lldb regnum for %s", reg_info && reg_info->name
                                                ? reg_info->name
                                                : "<unknown register>");
 
-  if (IsGPR(reg_index))
-    return WriteRegisterRaw(reg_index, reg_value);
+  uint8_t *dst;
+  uint32_t offset;
 
-  if (IsFPR(reg_index)) {
-    // Get pointer to m_fpr variable and set the data to it.
-    uint32_t fpr_offset = CalculateFprOffset(reg_info);
-    assert(fpr_offset < sizeof m_fpr);
-    uint8_t *dst = (uint8_t *)&m_fpr + fpr_offset;
-    switch (reg_info->byte_size) {
-    case 2:
-      *(uint16_t *)dst = reg_value.GetAsUInt16();
-      break;
-    case 4:
-      *(uint32_t *)dst = reg_value.GetAsUInt32();
-      break;
-    case 8:
-      *(uint64_t *)dst = reg_value.GetAsUInt64();
-      break;
-    default:
-      assert(false && "Unhandled data size.");
-      return Status("unhandled register data size %" PRIu32,
-                    reg_info->byte_size);
-    }
+  if (IsGPR(reg)) {
+    error = ReadGPR();
+    if (error.Fail())
+      return error;
+
+    offset = reg_info->byte_offset;
+    assert(offset < GetGPRSize());
+    dst = (uint8_t *)GetGPRBuffer() + offset;
+
+  } else if (IsFPR(reg)) {
 
-    Status error = WriteFPR();
+    error = ReadFPR();
     if (error.Fail())
       return error;
 
-    return Status();
-  }
+    offset = CalculateFprOffset(reg_info);
+    assert(offset < GetFPRSize());
+    dst = (uint8_t *)GetFPRBuffer() + offset;
+  } else
+    return Status("failed - register wasn't recognized to be a GPR or an FPR, "
+                  "write strategy unknown");
 
-  return Status("failed - register wasn't recognized to be a GPR or an FPR, "
-                "write strategy unknown");
+  ::memcpy(dst, reg_value.GetBytes(), reg_info->byte_size);
+
+  return error;
 }
 
 Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
   Status error;
 
+  InvalidateAllRegisters();
+
   data_sp.reset(new DataBufferHeap(REG_CONTEXT_SIZE, 0));
   error = ReadGPR();
   if (error.Fail())
@@ -286,9 +284,9 @@
     return error;
 
   uint8_t *dst = data_sp->GetBytes();
-  ::memcpy(dst, &m_gpr_arm64, GetGPRSize());
+  ::memcpy(dst, GetGPRBuffer(), GetGPRSize());
   dst += GetGPRSize();
-  ::memcpy(dst, &m_fpr, sizeof(m_fpr));
+  ::memcpy(dst, GetFPRBuffer(), GetFPRSize());
 
   return error;
 }
@@ -297,6 +295,8 @@
     const lldb::DataBufferSP &data_sp) {
   Status error;
 
+  InvalidateAllRegisters();
+
   if (!data_sp) {
     error.SetErrorStringWithFormat(
         "NativeRegisterContextLinux_x86_64::%s invalid data_sp provided",
@@ -320,14 +320,14 @@
                                    __FUNCTION__);
     return error;
   }
-  ::memcpy(&m_gpr_arm64, src, GetRegisterInfoInterface().GetGPRSize());
+  ::memcpy(GetGPRBuffer(), src, GetRegisterInfoInterface().GetGPRSize());
 
   error = WriteGPR();
   if (error.Fail())
     return error;
 
   src += GetRegisterInfoInterface().GetGPRSize();
-  ::memcpy(&m_fpr, src, sizeof(m_fpr));
+  ::memcpy(GetFPRBuffer(), src, GetFPRSize());
 
   error = WriteFPR();
   if (error.Fail())
@@ -832,117 +832,82 @@
                                            &hwbType, &ioVec, ioVec.iov_len);
 }
 
-Status NativeRegisterContextLinux_arm64::DoReadRegisterValue(
-    uint32_t offset, const char *reg_name, uint32_t size,
-    RegisterValue &value) {
-  Status error;
-  if (offset > sizeof(struct user_pt_regs)) {
-    offset -= sizeof(struct user_pt_regs);
-    if (offset > sizeof(struct user_fpsimd_state)) {
-      error.SetErrorString("invalid offset value");
-      return error;
-    }
-    elf_fpregset_t regs;
-    int regset = NT_FPREGSET;
-    struct iovec ioVec;
+Status NativeRegisterContextLinux_arm64::ReadGPR() {
+  Status error;
 
-    ioVec.iov_base = &regs;
-    ioVec.iov_len = sizeof regs;
-    error = NativeProcessLinux::PtraceWrapper(
-        PTRACE_GETREGSET, m_thread.GetID(), &regset, &ioVec, sizeof regs);
-    if (error.Success()) {
-      value.SetBytes((void *)(((unsigned char *)(&regs)) + offset), 16,
-                     m_thread.GetProcess().GetByteOrder());
-    }
-  } else {
-    elf_gregset_t regs;
-    int regset = NT_PRSTATUS;
+  if (!m_gpr_is_valid) {
     struct iovec ioVec;
 
-    ioVec.iov_base = &regs;
-    ioVec.iov_len = sizeof regs;
-    error = NativeProcessLinux::PtraceWrapper(
-        PTRACE_GETREGSET, m_thread.GetID(), &regset, &ioVec, sizeof regs);
-    if (error.Success()) {
-      value.SetBytes((void *)(((unsigned char *)(regs)) + offset), 8,
-                     m_thread.GetProcess().GetByteOrder());
-    }
-  }
-  return error;
-}
-
-Status NativeRegisterContextLinux_arm64::DoWriteRegisterValue(
-    uint32_t offset, const char *reg_name, const RegisterValue &value) {
-  Status error;
-  ::pid_t tid = m_thread.GetID();
-  if (offset > sizeof(struct user_pt_regs)) {
-    offset -= sizeof(struct user_pt_regs);
-    if (offset > sizeof(struct user_fpsimd_state)) {
-      error.SetErrorString("invalid offset value");
-      return error;
-    }
-    elf_fpregset_t regs;
-    int regset = NT_FPREGSET;
-    struct iovec ioVec;
+    ioVec.iov_base = GetGPRBuffer();
+    ioVec.iov_len = GetGPRSize();
 
-    ioVec.iov_base = &regs;
-    ioVec.iov_len = sizeof regs;
-    error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, &regset,
-                                              &ioVec, sizeof regs);
-
-    if (error.Success()) {
-      ::memcpy((void *)(((unsigned char *)(&regs)) + offset), value.GetBytes(),
-               16);
-      error = NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, tid, &regset,
-                                                &ioVec, sizeof regs);
-    }
-  } else {
-    elf_gregset_t regs;
-    int regset = NT_PRSTATUS;
-    struct iovec ioVec;
+    error = ReadRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS);
 
-    ioVec.iov_base = &regs;
-    ioVec.iov_len = sizeof regs;
-    error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, &regset,
-                                              &ioVec, sizeof regs);
-    if (error.Success()) {
-      ::memcpy((void *)(((unsigned char *)(&regs)) + offset), value.GetBytes(),
-               8);
-      error = NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, tid, &regset,
-                                                &ioVec, sizeof regs);
-    }
+    if (error.Success())
+      m_gpr_is_valid = true;
   }
-  return error;
-}
 
-Status NativeRegisterContextLinux_arm64::ReadGPR() {
-  struct iovec ioVec;
-  ioVec.iov_base = GetGPRBuffer();
-  ioVec.iov_len = GetGPRSize();
-  return ReadRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS);
+  return error;
 }
 
 Status NativeRegisterContextLinux_arm64::WriteGPR() {
   struct iovec ioVec;
+
   ioVec.iov_base = GetGPRBuffer();
   ioVec.iov_len = GetGPRSize();
+
   return WriteRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS);
 }
 
 Status NativeRegisterContextLinux_arm64::ReadFPR() {
-  struct iovec ioVec;
-  ioVec.iov_base = GetFPRBuffer();
-  ioVec.iov_len = GetFPRSize();
-  return ReadRegisterSet(&ioVec, GetFPRSize(), NT_FPREGSET);
+  Status error;
+
+  if (!m_fpu_is_valid) {
+    struct iovec ioVec;
+
+    ioVec.iov_base = GetFPRBuffer();
+    ioVec.iov_len = GetFPRSize();
+
+    error = ReadRegisterSet(&ioVec, GetFPRSize(), NT_FPREGSET);
+
+    if (error.Success())
+      m_fpu_is_valid = true;
+  }
+
+  return error;
 }
 
 Status NativeRegisterContextLinux_arm64::WriteFPR() {
   struct iovec ioVec;
+
   ioVec.iov_base = GetFPRBuffer();
   ioVec.iov_len = GetFPRSize();
+
   return WriteRegisterSet(&ioVec, GetFPRSize(), NT_FPREGSET);
 }
 
+Status NativeRegisterContextLinux_arm64::InvalidateAllRegisters() {
+  Status error;
+
+  if (m_gpr_is_valid)
+    error = WriteGPR();
+
+  if (error.Fail())
+    return error;
+
+  m_gpr_is_valid = false;
+
+  if (m_fpu_is_valid)
+    error = WriteFPR();
+
+  if (error.Fail())
+    return error;
+
+  m_fpu_is_valid = false;
+
+  return error;
+}
+
 uint32_t NativeRegisterContextLinux_arm64::CalculateFprOffset(
     const RegisterInfo *reg_info) const {
   return reg_info->byte_offset -
Index: source/Plugins/Process/Linux/NativeProcessLinux.cpp
===================================================================
--- source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -941,6 +941,8 @@
   Status error;
   NativeRegisterContext& register_context = thread.GetRegisterContext();
 
+  register_context.InvalidateAllRegisters();
+
   std::unique_ptr<EmulateInstruction> emulator_up(
       EmulateInstruction::FindPlugin(m_arch, eInstructionTypePCModifying,
                                      nullptr));
@@ -1056,6 +1058,8 @@
   for (const auto &thread : m_threads) {
     assert(thread && "thread list should not contain NULL threads");
 
+    thread->GetRegisterContext().InvalidateAllRegisters();
+
     const ResumeAction *const action =
         resume_actions.GetActionForThread(thread->GetID(), true);
 
@@ -1113,6 +1117,7 @@
     return error;
 
   for (const auto &thread : m_threads) {
+    thread->GetRegisterContext().InvalidateAllRegisters();
     Status e = Detach(thread->GetID());
     if (e.Fail())
       error =
Index: include/lldb/Host/common/NativeRegisterContext.h
===================================================================
--- include/lldb/Host/common/NativeRegisterContext.h
+++ include/lldb/Host/common/NativeRegisterContext.h
@@ -109,6 +109,8 @@
                              lldb::addr_t dst_addr, size_t dst_len,
                              const RegisterValue &reg_value);
 
+  virtual Status InvalidateAllRegisters() { return Status(); };
+
   // Subclasses should not override these
   virtual lldb::tid_t GetThreadID() const;
 
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to