[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and ptrace support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77047#1996198 <https://reviews.llvm.org/D77047#1996198>, @omjavaid wrote:

> This update includes core file register access for SVE registers. Register descriptions can now be tested with core dump.


Cool. Could you please split off the stuff necessary for making core files work into a separate patch, and have the "native" patch be on top of that. I want to get an idea of how much of new code is included in the second patch.

> @labath Should I post QEMU SVE setup instructions somewhere if you want to have a go at running included test cases.?

I think that would be very valuable. Not really much for this patch (I probably wont get around to running it), but for any future contributors (inlcuding myself) whose patches break some arm stuff (be it SVE-related or not).

In D77047#1996202 <https://reviews.llvm.org/D77047#1996202>, @omjavaid wrote:

> Also about licensing issues of included macros I am in contact with original authors from ARM and I hope they will be able to help sign off this patch for LLVM submission.


Awesome.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1191
+          size_t vq = sve_vq_from_vl(m_sve_header.vl);
+          SetRegisterInfoMode(vq);
+          m_sve_ptrace_payload.resize(SVE_PT_SIZE(vq, SVE_PT_REGS_SVE));
----------------
omjavaid wrote:
> SetRegisterInfoMode(vq); is getting called over here.
Ok, I see. That makes sense. The part that seems pretty unfortunate here is that you've needed to add a fairly strange function to the generic (`RegisterInfoInterface)`  interface even though the caller and callee are in arm64-specific code. Let's see if we can avoid that.

What if, instead of remangling the register infos in `RegisterInfoPOSIX_arm64` every time that `SetRegisterInfoMode` is called, the "mode" was a argument to the class constructor, and changing of the mode was implemented by creating a new "info" object? I.e., this code would instead of calling `SetRegisterInfoMode(vq);`, do something like `m_register_info_interface_up = std::make_unique<RegisterInfoPOSIX_arm64>(vq);`

Would that work?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeThreadLinux.h:42-43
   NativeRegisterContextLinux &GetRegisterContext() override {
+    if (m_reg_context_up && IsStopped(nullptr))
+      m_reg_context_up->ConfigureRegisterContext();
+
----------------
omjavaid wrote:
> labath wrote:
> > Why can't this work be done in the register context constructor? I believe we are always creating a Thread (and its reg context) while it is stopped...
> ConfigureRegisterContext makes sure that sve register offsets and sizes are correctly updated and synced from ptrace on every stop. If SVE registers reconfigure themselves we ll have to update that information into our dynamic register infos array. Although for the context of this patch it assumes it will never change its register size during execution. But I will follow up patch that implements that behaviour.
Ok, that makes sense now. The thing on my mind in that case is that this functionality overlaps a lot with `InvalidateAllRegisters` we've added a couple of patches back. I think it would be nice to have just one function which "reinitializes" a register context. `InvalidateAllRegisters` wouldn't work right now, because it's called before a resume, but if we changed it to be called after a stop, it would be able to do both things.

Would it be possible to change it so that this `InvalidateAllRegisters` is called after a stop, e.g. in `NativeThreadLinux::SetStopped`. I am sorry for the churn, I know it was my idea to call it before a resume -- it seemed like a reasonable thing to do at the time, but not any more.. :(


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

https://reviews.llvm.org/D77047