labath marked an inline comment as done.
labath added a comment.
Thanks. This looks is starting to look good now. I didn't get a chance to
review all of the changes, but here's a few more things that I noticed.
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:72-76
+ uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0);
+
+ uint32_t GetRegisterInfoMode() const;
+
+ bool RegisterInfoModeIsValid(uint32_t mode) {
----------------
Now that this is arm-specific, I am wondering if there isn't a better name for
this than "register info 'mode'". Maybe it could be just "enable/disable SVE" ?
Specifically, if the validity of the "mode" is checked by the caller, then we
can avoid this `RegisterInfoModeIsValid` business...
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:72-89
+ if (IsSVEZReg(reg_num))
+ return (reg_num - sve_z0_arm64) * 16;
+ else if (reg_num == fpu_fpsr_arm64)
+ return 32 * 16;
+ else if (reg_num == fpu_fpcr_arm64)
+ return (32 * 16) + 4;
+ } else if (m_sve_state == SVE_STATE::SVE_STATE_FULL) {
----------------
omjavaid wrote:
> labath wrote:
> > no else after return
> Ack.
Well.. that's one way of handing that.. :) It's not necessarily bad, but I was
imagining you would just delete all the "else"s...
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:21
RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
Thread &thread, RegisterInfoInterface *register_info,
const DataExtractor &gpregset, llvm::ArrayRef<CoreNote> notes)
----------------
It would be also nice if this constructor accepted a `RegisterInfoPOSIX_arm64`
instead of plain `RegisterInfoInterface`. That would make it much harder to
misuse this class. For that we'd need to slightly refactor the construction
code in ThreadElfCore. For example, we could swap the order of the "os" and
"machine" switches and then join the two "machine" switches that construct
RegisterInfoInterfaces and RegisterContexts.
Maybe you could do that as a separate patch?
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17-22
+#include <asm/ptrace.h>
+
+#ifndef SVE_PT_REGS_SVE
+#define INCLUDE_LINUX_PTRACE_DEFINITIONS_FOR_SVE_ARM64
+#include "Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h"
+#endif
----------------
This is including platform-specific headers, but elf cores should be readable
from any host. I guess you should just re-define the relevant constants
somewhere. (I'm not sure what they are.)
================
Comment at: lldb/source/Utility/ARM64_DWARF_Registers.h:54-146
+ // 34-45 reserved
+
+ // 64-bit SVE Vector granule pseudo register
+ vg = 46,
+
+ // VG ́8-bit SVE first fault register
+ ffr = 47,
----------------
All of these numbers are from the official spec, right? Maybe you could just
commit this straight away so it does not clutter this review.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77047/new/
https://reviews.llvm.org/D77047
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
