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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks. This looks much better, but there is still one thing which bugs me about the register info constructor. Currently, there are three paths through that constructor:

1. when we don't have any fancy registers (this is the original one)
2. when we have just SVE (added with the sve support)
3. when we have pauth et al. (added now)

Do we need all three of them? Is there anything which makes SVE special that it deserves its own register info array? Could it be just another "dynamic" register set like the other features? (If that's true, I might consider also removing the first code path, making it just a special case of an empty set of dynamic features.)

I also have some inline comments, but there are just simple stylistic issues, I hope.



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:205-208
       m_register_info_p(GetRegisterInfoPtr(target_arch)),
-      m_register_info_count(GetRegisterInfoCount(target_arch)) {
+      m_register_info_count(GetRegisterInfoCount(target_arch)),
+      m_register_set_p(g_reg_sets_arm64),
+      m_register_set_count(k_num_register_sets - 1),
----------------
It would be better to initialize these in the else branch of the `if (m_opt_regsets.AllSet(eRegsetMaskSVE))` statement


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:218
+  if (m_opt_regsets.AnySet(eRegsetMaskDynamic)) {
+    const lldb_private::RegisterInfo *reginfo_start, *reginfo_end;
+    const lldb_private::RegisterSet *regset_start = g_reg_sets_arm64;
----------------
ArrayRef<RegisterInfo> reginfo;


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:221-222
+    if (m_opt_regsets.AllSet(eRegsetMaskSVE)) {
+      reginfo_start = g_register_infos_arm64_sve_le;
+      reginfo_end = g_register_infos_arm64_sve_le + sve_ffr + 1;
+      m_per_regset_regnum_range[m_register_set_count] =
----------------
reginfo = makeArrayRef(g_register_infos_arm64_sve_le, sve_ffr);


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:227-228
+    } else {
+      reginfo_start = g_register_infos_arm64_le;
+      reginfo_end = g_register_infos_arm64_le + fpu_fpcr + 1;
+    }
----------------
ditto


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:231-232
+
+    std::copy(reginfo_start, reginfo_end,
+              std::back_inserter(m_dynamic_reg_infos));
+    std::copy(regset_start, regset_start + m_register_set_count,
----------------
llvm::copy(reginfo, std::back_inserter);


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:233-234
+              std::back_inserter(m_dynamic_reg_infos));
+    std::copy(regset_start, regset_start + m_register_set_count,
+              std::back_inserter(m_dynamic_reg_sets));
+
----------------
`std::copy_n(regset_start, m_register_set_count, ...)`


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:255
+  if (m_opt_regsets.AnySet(eRegsetMaskDynamic))
+    return m_register_info_count;
+
----------------
Can we make it so that `m_register_info_count` always contains a valid value?


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:243
+  for (const auto &regset_range : m_per_regset_regnum_range) {
+    if (reg_index >= regset_range.second.first &&
+        reg_index <= regset_range.second.second)
----------------
omjavaid wrote:
> DavidSpickett wrote:
> > Should the last condition be <? Given:
> > ```
> > m_per_regset_regnum_range[FPRegSet] = std::make_pair(fpu_v0, fpu_fpcr);
> > ```
> > Where the last index is the first non FP register.
> m_per_regset_regnum_range includes start and end register number of a register set with start and end inlucded.
> 
> fpu_fpcr is included in FPU regset. On non SVE targets Vx, Dx. Sx register plus fpcr and fpsr are part of FPU regset.
> 
> 
I was also surprised at the `<`, as ranges in c++ are normally half-open. It may be better to stick with that, even if it means adding a `+1` to the make_pair statement.


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

https://reviews.llvm.org/D96458