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

Muhammad Omair Javaid via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Mar 8 04:04:57 PST 2021

omjavaid added inline comments.

Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:81
+  Status error = ReadSVEHeader();
+  uint32_t opt_regset = RegisterInfoPOSIX_arm64::eRegsetEnableDefault;
+  if (error.Success()) {
rovka wrote:
> 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()?
I have given this a thought and although Flags class doesnt completely fit in we still can adjust our logic to use it. I guess using Flags will look neat. I ll update. Thanks

Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:88
+    opt_regset |= RegisterInfoPOSIX_arm64::eRegsetEnableSVE;
+    GetRegisterInfo().ConfigureRegisterInfos(opt_regset);
+  } else {
rovka wrote:
> Nitpick: You should hoist the call out of the if, so it's easier to add other regsets in the future.
This piece will be updated in a child patch of this series where we add logic for Pauth and MTE regsets. 

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;
rovka wrote:
> Nitpick: Please use square brackets instead of parantheses, otherwise it's confusing (parentheses suggest an open interval).



More information about the lldb-commits mailing list