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).
Ack.
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
