[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb6f9d7b8fb2e: Cleanup and speedup 
NativeRegisterContextLinux_arm64 (authored by omjavaid).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D69371?vs=232501=232586#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp

Index: lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -241,6 +241,9 @@
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
 data = signo;
 
+  // Before thread resumes, clear any cached register data structures
+  GetRegisterContext().InvalidateAllRegisters();
+
   return NativeProcessLinux::PtraceWrapper(PTRACE_CONT, GetID(), nullptr,
reinterpret_cast(data));
 }
@@ -262,6 +265,9 @@
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
 data = signo;
 
+  // Before thread resumes, clear any cached register data structures
+  GetRegisterContext().InvalidateAllRegisters();
+
   // If hardware single-stepping is not supported, we just do a continue. The
   // breakpoint on the next instruction has been setup in
   // NativeProcessLinux::Resume.
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 _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 ) override;
-
-  Status DoWriteRegisterValue(uint32_t offset, const char *reg_name,
-  const RegisterValue ) 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,39 @@
 
   const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
 
-  if (IsFPR(reg)) {
-error = ReadFPR();
-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 (reg == LLDB_INVALID_REGNUM)
+return Status("no lldb regnum for %s", reg_info && reg_info->name
+   ? reg_info->name
+   : "");
+
+  uint8_t *src;
+  uint32_t offset;
 
-if (is_subreg) {
-  // Read the full aligned 64-bit register.
-  full_reg = reg_info->invalidate_regs[0];
+  if (IsGPR(reg)) {
+if (!m_gpr_is_valid) {
+  error = ReadGPR();
+  if (error.Fail())
+return error;
 }
 
-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);
+  } else 

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:32
+  // Invalidates cached values in register context data structures
+  virtual void InvalidateAllRegisters(){};
+

no semicolon here


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

I think this looks good now. Thanks for your patience.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 232501.
omjavaid added a comment.

Hi

I have updated this patch after incorporating previous suggestions like moving 
InvalidateAllRegisters declaration to NativeRegisterContextLinux and also 
relocated calls to InvalidateAllRegisters from NativeProcessLinux to 
NativeThreadLinux. We now call InvalidateAllRegisters just before we resume or 
single step via ptrace. Other than that on any register write register will be 
be invalidated by WriteGPR and WriteFPR as was happening previously. We dont 
really need a invalidate on detach because thread wont exist after that so I 
have removed it InvalidateAllRegisters which was being called at detach.

Also testing for GPR or FPR validity now happens outside WriteGPR and WriteFPR.

Thoughts?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp

Index: lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -241,6 +241,9 @@
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
 data = signo;
 
+  // Before thread resumes, clear any cached register data structures
+  GetRegisterContext().InvalidateAllRegisters();
+
   return NativeProcessLinux::PtraceWrapper(PTRACE_CONT, GetID(), nullptr,
reinterpret_cast(data));
 }
@@ -262,6 +265,9 @@
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
 data = signo;
 
+  // Before thread resumes, clear any cached register data structures
+  GetRegisterContext().InvalidateAllRegisters();
+
   // If hardware single-stepping is not supported, we just do a continue. The
   // breakpoint on the next instruction has been setup in
   // NativeProcessLinux::Resume.
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 _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 ) override;
-
-  Status DoWriteRegisterValue(uint32_t offset, const char *reg_name,
-  const RegisterValue ) 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,39 @@
 
   const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
 
-  if (IsFPR(reg)) {
-error = ReadFPR();
-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 (reg == LLDB_INVALID_REGNUM)
+return Status("no lldb regnum for %s", reg_info && reg_info->name
+   ? reg_info->name
+   : "");
+
+  uint8_t *src;
+  uint32_t offset;
 
-if (is_subreg) {
-  // Read the full aligned 64-bit register.
-  full_reg = reg_info->invalidate_regs[0];
+  if (IsGPR(reg)) {
+if (!m_gpr_is_valid) {
+  error = ReadGPR();
+  if (error.Fail())
+return error;

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks. I now agree with this change in principle, but I don't think its ready 
for an LGTM yet.

The main thing I'd like to discuss is this `InvalidateAllRegisters` function. I 
think we should come up with a story of when is this function expected to be 
called, and document it somewhere. Right now, the placement of call sites seems 
pretty random. For instance, it's not clear to me why one would need to call it 
in `NativeProcessLinux::SetupSoftwareSingleStepping`.

The reason we want to call this function is because the values that the kernel 
holds for this thread have (potentially) changed, and so we want to ensure we 
don't hold stale values. The way I see it, the values can change when:
a) we do a "register write" operation. It looks like you have this part 
covered, and the invalidation happens pretty close to the actual "write" syscall
b) when the thread gets a chance to run. I think this what the other 
`InvalidateAllRegisters` are trying to cover, but it's hard to tell because 
they are very far from the place where the actual thread gets resumed. In 
particular, the call in `NativeProcessLinux::Resume` seems troublesome, as it 
will call `NativeThreadLinux::Resume` *after* it "invalidates" the caches. 
However, `NativeProcessLinux::Resume` is responsible for setting the 
watchpoints, and this involves register read/writes, which may end up 
re-populating some caches. I guess that doesn't matter on arm because of how 
watchpoints are set, but I think it would make it easier to reason about these 
things if the invalidation happened right before we resume the thread, or 
immediately after it stops.




Comment at: lldb/include/lldb/Host/common/NativeRegisterContext.h:112-113
 
+  virtual void InvalidateAllRegisters(){};
+
   // Subclasses should not override these

Maybe this could be a method on `NativeRegisterContextLinux`. We can always 
later lift it to the main class if we find a reason to do that...



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:255-258
+error = WriteGPR();
+if (error.Fail())
+  return error;
+  } else if (IsFPR(reg)) {

There's nothing happening after this if block, so you could write this so that 
it always returns (`return WriteGPR()`), and then drop the "else" from the next 
condition.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:305-306
 
+  InvalidateAllRegisters();
+
   if (!data_sp) {

I guess this isn't needed, since Write[FG]PR already handle invalidation on 
their own?



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:874
+
+  if (!m_fpu_is_valid) {
+struct iovec ioVec;

Please change this to an early return (as well as the ReadGPR above).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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 _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 ) override;
-
-  Status DoWriteRegisterValue(uint32_t offset, const char *reg_name,
-  const RegisterValue ) 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
+   : "");
+
+  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 

Re: [Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Pavel Labath via lldb-commits]

On 28/10/2019 11:57, Omair Javaid wrote:

On Fri, 25 Oct 2019 at 17:53, Pavel Labath via Phabricator
 wrote:


labath added a comment.

In D69371#1721077 , @omjavaid wrote:


We ll be dealing with Linux user mode and mostly aarch64 data registers except 
for cpsr, fpsr and fpcr. I think we should be fine but let me confirm this 
again from documentation.



Right, but you're creating a general interface for all architectures, not just 
several aarch64 registers. Even if they don't make use of that facility now, it 
would be good to make sure they can do that in the future.

For instance, on x86, the kernel may decide to reject 
https://github.com/torvalds/linux/blob/master/arch/x86/kernel/ptrace.c#L187 some values 
of some registers, and silently ignore some bits in others 
https://github.com/torvalds/linux/blob/master/arch/x86/kernel/ptrace.c#L349. That's why I 
think it would be better to commit changes to memory automatically/immediately, and 
minimize the chances that subsequent "read" operations will return data which 
does not reflect the actual values held by the OS.


So I gave  fixed or undefined bits a thought and also considered
implications of reading/writing certain status or control registers.
User visible registers dont really have big implications and we can
afford to keep user-corrupted values until resume as in theory all
state changes are going to happen on resume and target/thread state is
halted.


I don't agree with that assessment. While doing the register write 
"late" will not corrupt the state of the application, it will make it 
harder for the user to figure out what's going on. If we return the 
ptrace error immediately, the user will get an error message relating to 
the write command. If we don't do that, the command will appear to 
succeed, and even subsequent "read" commands will return new "bogus" 
value. If we postpone the write, we will get an error while processing 
the "continue" command. At that point we we only have two options: 
return an error (which will be interpreted as a failure to resume the 
process) or drop it (and leave the user wondering why did the register 
values suddenly change back).




But even if we don't want the user to be writing fixed value bit
fields, we can easily choose to invalidate register caches in case of
certain registers.

For example
if (regno == cpsr)
InvalidateAllRegisters().

In case of arm64, NativeRegisterContextLinux_arm64::WriteRegister may
call NativeRegisterContextLinux_arm64::InvalidateAllRegisters() if a
register like cpsr, fpsr or fpcr is being written.
Other architectures can use similar implementation or ignore register
caching altogether.



Yes, that is certainly possible, but it increases the code complexity ( 
== likelihood of something going wrong). Which is why I was asking what 
is the use case you are optimizing for. Is it reading of registers? Is 
it writing? Unwinding? Expression evaluation? Have you got any numbers? etc.


Register reads are a lot more common than writes, so I expect we can go 
a long way by just making sure the read operations don't cause ptrace 
traffic. As caching reads is a much safer option, I'd like to better 
understand the tradeoff here.


BTW, another thing which you might be interested in is 
. Once that patch lands, register reads 
should be done via the $g packet. That will increase the read speed much 
more than this patch, as it will cut out the link latency too (though I 
am still not opposed to caching stuff at lldb-server layer too).


pl
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Omair Javaid via lldb-commits]

On Fri, 25 Oct 2019 at 17:53, Pavel Labath via Phabricator
 wrote:
>
> labath added a comment.
>
> In D69371#1721077 , @omjavaid wrote:
>
> > We ll be dealing with Linux user mode and mostly aarch64 data registers 
> > except for cpsr, fpsr and fpcr. I think we should be fine but let me 
> > confirm this again from documentation.
>
>
> Right, but you're creating a general interface for all architectures, not 
> just several aarch64 registers. Even if they don't make use of that facility 
> now, it would be good to make sure they can do that in the future.
>
> For instance, on x86, the kernel may decide to reject 
> https://github.com/torvalds/linux/blob/master/arch/x86/kernel/ptrace.c#L187 
> some values of some registers, and silently ignore some bits in others 
> https://github.com/torvalds/linux/blob/master/arch/x86/kernel/ptrace.c#L349. 
> That's why I think it would be better to commit changes to memory 
> automatically/immediately, and minimize the chances that subsequent "read" 
> operations will return data which does not reflect the actual values held by 
> the OS.

So I gave  fixed or undefined bits a thought and also considered
implications of reading/writing certain status or control registers.
User visible registers dont really have big implications and we can
afford to keep user-corrupted values until resume as in theory all
state changes are going to happen on resume and target/thread state is
halted.

But even if we don't want the user to be writing fixed value bit
fields, we can easily choose to invalidate register caches in case of
certain registers.

For example
if (regno == cpsr)
   InvalidateAllRegisters().

In case of arm64, NativeRegisterContextLinux_arm64::WriteRegister may
call NativeRegisterContextLinux_arm64::InvalidateAllRegisters() if a
register like cpsr, fpsr or fpcr is being written.
Other architectures can use similar implementation or ignore register
caching altogether.

>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69371/new/
>
> https://reviews.llvm.org/D69371
>
>
>


-- 
Omair Javaid
www.linaro.org
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D69371#1721077 , @omjavaid wrote:

> We ll be dealing with Linux user mode and mostly aarch64 data registers 
> except for cpsr, fpsr and fpcr. I think we should be fine but let me confirm 
> this again from documentation.


Right, but you're creating a general interface for all architectures, not just 
several aarch64 registers. Even if they don't make use of that facility now, it 
would be good to make sure they can do that in the future.

For instance, on x86, the kernel may decide to reject 
https://github.com/torvalds/linux/blob/master/arch/x86/kernel/ptrace.c#L187 
some values of some registers, and silently ignore some bits in others 
https://github.com/torvalds/linux/blob/master/arch/x86/kernel/ptrace.c#L349. 
That's why I think it would be better to commit changes to memory 
automatically/immediately, and minimize the chances that subsequent "read" 
operations will return data which does not reflect the actual values held by 
the OS.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked an inline comment as done.
omjavaid added a comment.

We ll be dealing with Linux user mode and mostly aarch64 data registers except 
for cpsr, fpsr and fpcr. I think we should be fine but let me confirm this 
again from documentation.

Further more, motivation behind these changes is SVE register set which may 
exchange high volume of data due to large register sizes.




Comment at: 
source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h:153-154
 
+  bool m_gpr_is_valid;
+  bool m_fpu_is_valid;
+

labath wrote:
> Please move these next to the members they are guarding.
Alright.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The idea seems nice, however I am wondering, if it is not going a bit too far. 
In not sure of the exact situation on arm64, but in general, there are 
registers which only accept some bit patterns. What exactly does ptrace do in 
this situation? Postponing the write until we resume/detach means we lose the 
opportunity to report any errors that would result from the ptrace call.

What's the exact case you're optimising for? Would it be enough to just cache 
the reads, but then as soon as we write something, we immediately call ptrace 
to ensure that the value was written successfully? It sounds like this could be 
enough as the majority of the operations should be reads...




Comment at: 
source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h:153-154
 
+  bool m_gpr_is_valid;
+  bool m_fpu_is_valid;
+

Please move these next to the members they are guarding.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69371/new/

https://reviews.llvm.org/D69371



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69371: [ARM64] Cleanup and speedup NativeRegisterContextLinux_arm64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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 _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 ) override;
-
-  Status DoWriteRegisterValue(uint32_t offset, const char *reg_name,
-  const RegisterValue ) 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
+   : "");
+
+  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