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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
