omjavaid updated this revision to Diff 231408. omjavaid added a comment. Sorry for late response on this. I got side tracked into other issues and wanted performance testing before writing an update.
I agree with @labath about making caching for limited to reads only and register writes are written all the way in latest update. Also according to performance tests these changes show no significant improvement on real hardware but if underlying hardware is a simulator like QEMU some improvement does occur. This is the main reason I am keeping these changes intact for register reads but have removed for register writes. I have tested this patch on Ubuntu Bionic running on QEMU and Thunder X1 arm64 server. LGTM? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69371/new/ https://reviews.llvm.org/D69371 Files: lldb/include/lldb/Host/common/NativeRegisterContext.h lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h =================================================================== --- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h +++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h @@ -40,6 +40,8 @@ Status WriteAllRegisterValues(const lldb::DataBufferSP &data_sp) override; + void 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,17 @@ 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; + }; + + bool m_gpr_is_valid; + bool m_fpu_is_valid; + + GPR m_gpr_arm64; // 64-bit general purpose registers. RegInfo m_reg_info; FPU m_fpr; // floating-point registers including extended register sets. Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp =================================================================== --- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp +++ lldb/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]; - } + offset = reg_info->byte_offset; + assert(offset < GetGPRSize()); + src = (uint8_t *)GetGPRBuffer() + offset; - error = ReadRegisterRaw(full_reg, reg_value); + } else if (IsFPR(reg)) { - 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); + error = ReadFPR(); + if (error.Fail()) + return error; - // 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; - } + 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,48 +226,55 @@ Status NativeRegisterContextLinux_arm64::WriteRegister( const RegisterInfo *reg_info, const RegisterValue ®_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); - - 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); - } + uint8_t *dst; + uint32_t offset; - Status error = WriteFPR(); + if (IsGPR(reg)) { + error = ReadGPR(); if (error.Fail()) return error; - return Status(); - } + offset = reg_info->byte_offset; + assert(offset < GetGPRSize()); + dst = (uint8_t *)GetGPRBuffer() + offset; - 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); + + error = WriteGPR(); + if (error.Fail()) + return error; + } else if (IsFPR(reg)) { + + error = ReadFPR(); + if (error.Fail()) + return error; + + offset = CalculateFprOffset(reg_info); + assert(offset < GetFPRSize()); + dst = (uint8_t *)GetFPRBuffer() + offset; + + ::memcpy(dst, reg_value.GetBytes(), reg_info->byte_size); + error = WriteFPR(); + if (error.Fail()) + return error; + + } else + return Status("failed - register wasn't recognized to be a GPR or an FPR, " + "write strategy unknown"); + + return error; } Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues( @@ -286,9 +291,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 +302,8 @@ const lldb::DataBufferSP &data_sp) { Status error; + InvalidateAllRegisters(); + if (!data_sp) { error.SetErrorStringWithFormat( "NativeRegisterContextLinux_x86_64::%s invalid data_sp provided", @@ -320,14 +327,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 +839,69 @@ &hwbType, &ioVec, ioVec.iov_len); } -Status NativeRegisterContextLinux_arm64::DoReadRegisterValue( - uint32_t offset, const char *reg_name, uint32_t size, - RegisterValue &value) { +Status NativeRegisterContextLinux_arm64::ReadGPR() { 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; - ioVec.iov_base = ®s; - ioVec.iov_len = sizeof regs; - error = NativeProcessLinux::PtraceWrapper( - PTRACE_GETREGSET, m_thread.GetID(), ®set, &ioVec, sizeof regs); - if (error.Success()) { - value.SetBytes((void *)(((unsigned char *)(®s)) + 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 = ®s; - ioVec.iov_len = sizeof regs; - error = NativeProcessLinux::PtraceWrapper( - PTRACE_GETREGSET, m_thread.GetID(), ®set, &ioVec, sizeof regs); - if (error.Success()) { - value.SetBytes((void *)(((unsigned char *)(regs)) + offset), 8, - m_thread.GetProcess().GetByteOrder()); - } - } - return error; -} + ioVec.iov_base = GetGPRBuffer(); + ioVec.iov_len = GetGPRSize(); -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; + error = ReadRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS); - ioVec.iov_base = ®s; - ioVec.iov_len = sizeof regs; - error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, ®set, - &ioVec, sizeof regs); - - if (error.Success()) { - ::memcpy((void *)(((unsigned char *)(®s)) + offset), value.GetBytes(), - 16); - error = NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, tid, ®set, - &ioVec, sizeof regs); - } - } else { - elf_gregset_t regs; - int regset = NT_PRSTATUS; - struct iovec ioVec; - - ioVec.iov_base = ®s; - ioVec.iov_len = sizeof regs; - error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, ®set, - &ioVec, sizeof regs); - if (error.Success()) { - ::memcpy((void *)(((unsigned char *)(®s)) + offset), value.GetBytes(), - 8); - error = NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, tid, ®set, - &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; + + m_gpr_is_valid = false; + 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; + + m_fpu_is_valid = false; + ioVec.iov_base = GetFPRBuffer(); ioVec.iov_len = GetFPRSize(); + return WriteRegisterSet(&ioVec, GetFPRSize(), NT_FPREGSET); } +void NativeRegisterContextLinux_arm64::InvalidateAllRegisters() { + m_gpr_is_valid = false; + m_fpu_is_valid = false; +} + uint32_t NativeRegisterContextLinux_arm64::CalculateFprOffset( const RegisterInfo *reg_info) const { return reg_info->byte_offset - Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp =================================================================== --- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp +++ lldb/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: lldb/include/lldb/Host/common/NativeRegisterContext.h =================================================================== --- lldb/include/lldb/Host/common/NativeRegisterContext.h +++ lldb/include/lldb/Host/common/NativeRegisterContext.h @@ -109,6 +109,8 @@ lldb::addr_t dst_addr, size_t dst_len, const RegisterValue ®_value); + virtual void InvalidateAllRegisters(){}; + // 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