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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
