[Lldb-commits] [PATCH] D25864: Fix arm64 floating point register spill recording in UnwindPlan analysis

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

A few high level comments:

- I have the feeling you ported much more code over to use the LLDB register numbers then it would be strictly necessary. I am not sure if it is good or bad as it can help us consolidate the confusion around the different register numbering schema but it icnreases the size of this patch by a lot and also adds a lot of new boilerplate code.
- I would prefer to keep lldb-arm64-register-enums.h over the new file you added as your code contains a lot of logic in ARM64_LLDB_Registers.cpp (e.g. to string conversion) what seems unnecessary to me as the register tables you modified recently already contains all information to get from register number to register name
- Regarding Greg's concern I think we should have a single table (RegisterInfoInterface) containing all registers what can appear on the target and then the register context should be the one who detects which one is available and then returns the correct one accordingly. We already have functionality like this for x86_64 where RegisterInfos_x86_64.h and lldb-x86-register-enums.h defines all possibly available register and then NativeRegisterContextLinux_x86_64 detects if the AVX and MPX registers are available or not and then report the register list accordingly.

In mid/long term I think we should clean up the full register info handling code to simplify all of this in the following lines (vague ideas):

- Implement a single copy of RegisterInfoInterface for every architecture what returns the correct list of registers based on the architecture and sub-architecture
- Define register sets inside the RegisterInfoInterface instead of inside the RegistrerContext... classes
- Change EmulateInstruction... to use the RegisterInfoInterface to get a RegisterInfo object from the register numbers
- Change all RegisterContext... to use the information from the RegisterInfoInterface... classes instead of duplicating it into them

Doing all of the cleanup will be a lot of work so I am not asking you to do it or I don't want to wait with this change for that long but wanted to raise it here so we don't start to move in a direction what will make later improvements more difficult



================
Comment at: source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:196-197
 
-  if (reg_kind == eRegisterKindDWARF)
-    return arm64_dwarf::GetRegisterInfo(reg_num, reg_info);
+  if (reg_kind == eRegisterKindLLDB)
+    return arm64_lldb::GetRegisterInfo(reg_num, reg_info);
   return false;
----------------
What do you think about teaching this function to handle both eRegisterKindDWARF and eRegisterKindLLDB? That way you can use both in the emulation code. Also that would me that you don't have to update all usage inside the emulation code.


================
Comment at: source/Utility/ARM64_LLDB_Registers.cpp:18
+
+const char *arm64_lldb::GetRegisterName(unsigned reg_num,
+                                        bool altnernate_name) {
----------------
I think this function is completely unnecessary if you use the g_register_infos_arm64_le table (or one of it's wrapper)


Repository:
  rL LLVM

https://reviews.llvm.org/D25864