andrew added inline comments.

================
Comment at: 
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp:73
 
-Status NativeRegisterContextFreeBSD_arm64::ReadRegisterSet(uint32_t set) {
-  switch (set) {
-  case RegisterInfoPOSIX_arm64::GPRegSet:
-    return NativeProcessFreeBSD::PtraceWrapper(PT_GETREGS, m_thread.GetID(),
-                                               m_reg_data.data());
-  case RegisterInfoPOSIX_arm64::FPRegSet:
-    return NativeProcessFreeBSD::PtraceWrapper(
-        PT_GETFPREGS, m_thread.GetID(),
-        m_reg_data.data() + sizeof(RegisterInfoPOSIX_arm64::GPR));
-  }
-  llvm_unreachable("NativeRegisterContextFreeBSD_arm64::ReadRegisterSet");
+bool NativeRegisterContextFreeBSD_arm64::IsGPR(unsigned reg) const {
+  if (GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) ==
----------------
mgorny wrote:
> These helpers don't seem very helpful. Isn't it better to use `switch` on the 
> value returned by `GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg)`?
Using a switch will be messy when adding pointer authentication or MTE 
registers as we need to call `RegisterInfoPOSIX_arm64::IsPAuthReg` [1] to check 
if a given register is a pointer authentication register, and similar for MTE. 
This is because they are dynamically added as needed.

[1] 
https://github.com/llvm/llvm-project/blob/76e47d4/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp#L392-L396


================
Comment at: 
lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp:133
+  if (IsGPR(reg)) {
+    error = ReadGPR();
+    if (error.Fail())
----------------
mgorny wrote:
> To be honest, it seems counterproductive to restore the duplication that I've 
> removed before. Is there any real advantage to having two methods here, and 
> calling them separately from inside the `if` instead of calling 
> `ReadRegisterSet(set)` before the `if`?
Because of the dynamic nature of the register sets we may not have a usable 
value to pass into `ReadRegisterSet` and any logic to decide which register set 
is being read in `ReadRegisterSet` would need to be repeated here and below 
when writing a register.

Calling the function `ReadRegisterSet` will also be confusing when I add 
`PT_GETREGSET` and `PT_SETREGSET` [1] to FreeBSD. This will be used to access 
the pointer authentication address masks on FreeBSD (and future registers, e.g. 
MTE & SVE).

See [2] for a WIP patch with how I plan to extend this for code & data address 
masks needed by pointer authentication.

[1] https://reviews.freebsd.org/D19831
[2] 
https://github.com/zxombie/llvm-project/commit/b801eca#diff-3d6a219398418eb59bf8104bda44a111f47d29081e076b4ce69336882e482cc2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110569

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

Reply via email to