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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
