labath added a comment.
There's always a chance for ODR violations, particularly with header files
defining static objects. This looks better though what I really wanted was to
keep the non-sve register info array (`g_register_infos_arm64_le`) completely
out of the path of the sve header. Like, by using three headers, one defining
`g_register_infos_arm64_le` (and stuff specific to that like the exception
registers), one defining the sve array, and the third one which would contain
the things common to both. Nonetheless, I think we can now move on to aspects
of this patch. Please see my inline comments.
================
Comment at:
lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:38-40
+ uint32_t SetRegisterInfoMode(uint32_t mode) {
+ return m_register_info_interface_up->SetRegisterInfoMode(mode);
+ }
----------------
labath wrote:
> I can't find any callers of this function. Who is calling it?
It still seems to me that this function is not used (in this patch at least).
================
Comment at:
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:261
+ return set_index < k_num_register_sets;
+ else
+ return set_index < (k_num_register_sets - 1);
----------------
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h:64-68
+ virtual uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0) {
+ return 0;
+ }
+
+ virtual uint32_t GetRegisterInfoMode() const { return 0; }
----------------
I don't believe these functions should be present on the generic interface. The
only caller and the only implementation of them is already arm64-specific, so
it should be possible to arrange it such that they don't need to go through the
base class interface. For instance the `RegisterContextCorePOSIX_arm64`
constructor could accept an `RegisterInfoPOSIX_arm64` instance to indicate what
it needs to work with, and then downcast `m_register_info_up` (via a helper
function?) when it needs to call these. Solutions without downcasting are
possible too, though may require a bit more refactoring.
================
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) {
----------------
no else after return
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:53-55
+ void ConfigureRegisterContext();
+
+ uint32_t CalculateSVEOffset(uint32_t reg_num) const;
----------------
Do these need to be public? If so, why?
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:76
+
+ mutable SVE_STATE m_sve_state;
+ struct user_sve_header m_sve_header;
----------------
Why is this mutable?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77047/new/
https://reviews.llvm.org/D77047
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
