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 ®set_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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
