[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