[Lldb-commits] [PATCH] D96458: [LLDB] Add support for Arm64/Linux dynamic register sets

[Diana Picus via Phabricator via lldb-commits]

rovka added a comment.

Hi Omair,

I had a look and I think the commit message is slightly misleading because it says "Arm64 has dynamic features like SVE, Pointer Authentication and MTE", but then you don't set m_reg_infos_is_dynamic to true for SVE. This got me a bit confused at first. Maybe it would help to elaborate a bit on this point in the commit message (and maybe also in the comments if you think it makes sense).

I also have a few nits inline.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:81
+  Status error = ReadSVEHeader();
+  uint32_t opt_regset = RegisterInfoPOSIX_arm64::eRegsetEnableDefault;
+  if (error.Success()) {
----------------
I was about to suggest using lldb_private::Flags for this, but after looking a bit more through the code it's not clear how this is intended to be used. The definition of eRegsetEnableSVE doesn't look flag-like at all (I would expect to see 1 << 1 rather than just plain 1, to make it clear what future values should look like), and also in ConfigureRegisterContext you check if (opt_regsets > eRegsetEnableSVE), which to me doesn't seem very idiomatic for working with flags. 

What do you think about increasing the level of abstraction a bit and using a small wrapper class for the regset options, that can answer directly questions like IsRegsetDynamic()?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:88
+    opt_regset |= RegisterInfoPOSIX_arm64::eRegsetEnableSVE;
+    GetRegisterInfo().ConfigureRegisterInfos(opt_regset);
+  } else {
----------------
Nitpick: You should hoist the call out of the if, so it's easier to add other regsets in the future.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:132
+  // Contains pair of (start, end) register numbers of a register set with start
+  // and end included.
+  std::map<uint32_t, std::pair<uint32_t, uint32_t>> m_per_regset_regnum_range;
----------------
Nitpick: Please use square brackets instead of parantheses, otherwise it's confusing (parentheses suggest an open interval).


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

https://reviews.llvm.org/D96458