- Added support for read/write FP registers in FR1 mode.
- Added 32 bit register context for mips32.
Details
- Reviewers
clayborg tberghammer jaydeep - Commits
- rGce815e4588fd: [MIPS][lldb-server] Add 32-bit register context and read/write FP registers on…
rLLDB238914: [MIPS][lldb-server] Add 32-bit register context and read/write FP registers on…
rL238914: [MIPS][lldb-server] Add 32-bit register context and read/write FP registers on…
Diff Detail
- Repository
- rL LLVM
Event Timeline
NativeRegisterContextLinux_mips.cpp and NativeRegisterContextLinux_mips64.cpp are both enabled at the same time (when mips is defined) what will cause a conflict because both implements NativeRegisterContextLinux::CreateHostNativeRegisterContextLinux for creating the appropriate register context. I don't know what is the relationship between mips and mips64 (if they should exist next to each outher as i386 and x86_64 or they are completely different ones as arm and aarch64) but you have to do something to always create the right context and have exactly one CreateHostNativeRegisterContextLinux implementation when mips is defined.
Please upload a revision with full context and where the base revision already contains D9935 to make it easier to review.
source/Core/RegisterValue.cpp | ||
---|---|---|
88–111 ↗ | (On Diff #26503) | We shouldn't have architecture specific code in RegisterValue. If I understand it correctly your problem is that both single and double precision floats can be stored in the same register and you want to display them correctly. I suggest to use alias registers for this issue (eg.: f1 -> hex, flt1 -> float, dbl1 -> double) in the register context so it will work in an architecture independent way. A similar solution is used on i386 and on x86_64 for handling the integer registers with the only difference that in that case you can address both part of the bigger register. |
source/Plugins/Process/Utility/RegisterContextLinux_mips.cpp | ||
112–113 | These registers should have uint encoding and hex format |
source/Core/RegisterValue.cpp | ||
---|---|---|
88–111 ↗ | (On Diff #26503) | RegisterValue.cpp shouldn't have arch specific stuff. Like tberghammer said, just make registers that have different names if needed. But you really shouldn't have to do this as you can view registers using a different format: (lldb) register read s31 This would display a float or double register as hex. If you need to show parts of a register you do make different registers. Registers through the GDB remote protocol with qRegisterInfo or the target.xml, can claim they are part of another register (RegisterInfo.value_regs). So if you have a 'f0' register for mips that is a double you can define a register that is "f0" whose size is 64 bits, and then make a "f0_low" (or some other better name) whose values_regs specifies the register number of "f0". When this register is read, it will then read the "f0" register and then take part of that value based off of the offset and size in the RegisterInfo structure. |
Hi @clayborg, @tberghammer,
NativeRegisterContextLinux_mips.cpp and NativeRegisterContextLinux_mips64.cpp are both enabled at the same time (when mips is defined) what will cause a conflict because both implements NativeRegisterContextLinux::CreateHostNativeRegisterContextLinux for creating the appropriate register context.
I will remove the NativeRegisterContextLinux_mips.cpp. NativeRegisterContextLinux_mips64.cpp will work for both mips and mips64.
We shouldn't have architecture specific code in RegisterValue
As @clayborg suggested, we can let the user use the --format option for printing float or double or hex values for floating point registers for now.
I will updated this patch with full context and address all review comments.
Thanks.
- Shifted code for defining g_register_infos_mips* to RegisterInfos_mips* classes.
- Shifted lldb internals codes for registers to lldb-mips64-register-enums.h.
- Addressed review comments by @tberghammer and @clayborg.
The general concept looks good.
For the mips specific part I don't have enough knowledge to verify, but I noticed one serious issue (see inline comment).
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp | ||
---|---|---|
478–498 |
Based on the two observation I am not sure if the general concept in the function is correct and those are just 2 typos or if the content of the function is accidentally swapped with the content of a ReadRegister function |
You are missing a break here (or if it is intentional then please add a comment)