[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 6 inline comments as done.
omjavaid added a comment.

In D77047#2125220 <https://reviews.llvm.org/D77047#2125220>, @labath wrote:

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


I am working to fix this with D80105 <https://reviews.llvm.org/D80105> by defining dynamic register infos array based on register sets which will be requirement for Pointer Authentication and MTE. But I wanted to first get done with SVE and come back to register infos refactoring later.



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:282-290
+  if (!m_per_vq_reg_infos.count(sve_vq)) {
+
+    m_per_vq_reg_infos.insert(std::make_pair(
+        sve_vq, std::vector<lldb_private::RegisterInfo>(
+                    g_register_infos_arm64_sve_le,
+                    g_register_infos_arm64_sve_le + m_register_info_count)));
+
----------------
labath wrote:
> I'd probably try relying on the fact that `operator[]` constructs an empty vector, and that the empty vector is not a valid register info value. Something like:
> ```
> std::vector<RegisterInfo> &new_reg_info = m_per_vq_reg_infos[sve_vq];
> if (new_reg_info.empty()) {
>   new_reg_info = llvm::makeArrayRef(g_register_infos_arm64_sve_le, m_register_info_count);
>   ...
> }
> m_register_info_p = new_reg_info.data();
> ```
> 
> That would make this less of a mouthful and also avoid looking up the same key in the map three or four times.
ACK


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:17-22
+enum class SVE_STATE {
+  SVE_STATE_UNKNOWN,
+  SVE_STATE_DISABLED,
+  SVE_STATE_FPSIMD,
+  SVE_STATE_FULL
+};
----------------
labath wrote:
> If this is going to be a class enum (which I think is a good idea), then there's no need to repeat the type name in the actual enumerators. It also seems like this could/should follow the lldb naming conventions (SveState::Disabled, etc).
ACK


================
Comment at: lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h:261
+  k_num_fpr_registers_arm64 = k_last_fpr_arm64 - k_first_fpr_arm64 + 1,
+
+  k_first_sve_arm64 = exc_far_arm64,
----------------
labath wrote:
> I guess these changes should be reverted now?
ACK


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+    if (IsSVEZ(reg_num))
+      sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+    else if (reg_num == GetRegNumFPSR())
----------------
labath wrote:
> I'm confused by this function. If I understant the SVE_PT macros and the logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, then they both seem to encode the same information. And it seems to me that this function should just be `reg_infos[reg_num].offset - some_constant`, which is the same thing that we're doing for the arm FP registers when SVE is disabled, and also for most other architectures too.
> 
> Why is that not the case? Am I missing something? If they are not encoding the same thing, could they be made to encode the same thing?
This function calculates offset of a particular register in core note data. SVE data in core dump is similar to what PTrace emits and offsets into this data is not linear. SVE macros are used to access those offsets based on register numbers and currently selected vector length.
Also for the purpose of ease we have linear offsets in SVE register infos and it helps us simplify register data once it makes way to GDBRemoteRegisterContext on the client side.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
 
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
+
----------------
labath wrote:
> Please group the header next to other `Plugins/Process/Utility` header
ACK


================
Comment at: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py:326-327
 
+    #@skipIf(triple='^mips')
+    #@skipIfLLVMTargetMissing("AArch64")
+    def test_aarch64_sve_regs(self):
----------------
labath wrote:
> What's up with these?
Sorry Typo


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77047/new/

https://reviews.llvm.org/D77047