In MIPS, the size of FPU registers is based on FR in Status registers. The user can change the FR bits during runtime which affects the size of FPU registers. This patch detects the state of these bits to decide the size of floating point registers while reading/writing FPU registers.
Details
- Reviewers
clayborg jingham - Commits
- rG4e15deb8e06c: Merging r277426: --------------------------------------------------------------…
rG7ae7c29bfbd3: Merging r277343: --------------------------------------------------------------…
rGf5cadce1cd66: [LLVM][MIPS] Add (D)SUBU, (D)ADDU, LUI instructions emulation . Fix emulation…
rG52b6cc5d5fac: [LLVM][MIPS] Fix FPU Size Based on Dynamic FR.
rLLDB277426: [LLVM][MIPS] Add (D)SUBU, (D)ADDU, LUI instructions emulation . Fix emulation…
rLLDB277343: [LLVM][MIPS] Fix FPU Size Based on Dynamic FR.
rL277426: [LLVM][MIPS] Add (D)SUBU, (D)ADDU, LUI instructions emulation . Fix emulation…
rL277343: [LLVM][MIPS] Fix FPU Size Based on Dynamic FR.
Diff Detail
- Repository
- rL LLVM
Event Timeline
This patch adds architecture specific code to generic GDB remote classes which should not happen. If we are going to fix correctly in the GDB remote classes, we need to abstract this. No mention of MIPS or any mips flags should be made inside of the GDB remote classes. The qRegisterInfo or the target XML data that is received from the GDB server describes all registers, you will need to add key/value pairs to the FP registers that describe these limitations then modify the DynamicRegisterInfo do the right thing. It shouldn't be too hard to add functionality that says that the byte size of a register depends on a value in another register.
One way we could abstract the size is to add a key named "bitsize-expr" that is a DWARF expression. The expression in your case would be a DWARF expression:
DW_OP_regx(32) DW_OP_lit1 DW_OP_lit26 DW_OP_shl
"DW_OP_regx(32)" would push the value of register 32 onto the stack. Then it would push 1 onto the stack (DW_OP_lit1) followed by 26 onto the stack (DW_OP_lit26) and then shift 1 << 26 with DW_OP_shl... Etc. The DWARF expression would result in the byte size being on the stack at the end as the DWARF expressions can branch and do other things. So it would be possible to come up with a byte stream that can be used by all registers that is agnostic to the architecture.
Then if a register has a "bitsize-expr", the size would need to be determined by running a DWARF expression.
Added field "dynamic_size" in struct RegisterInfo. If the value of this field is 1 then register size should be detected at runtime. In case of MIPS, the value of this field will be always 1 for all floating point registers. Then the size of register will be evaluated based on result of Dwarf Expression.
So this is close. My idea was that the lldb-server would not just set a key/value pair named "dynamic_size" to "1" in the "qRegisterInfo" or "$qXfer:features:read:target.xml:0,1ffff" XML data, it would add a key value pair: whose name is "dynamic_size_dwarf_expr" whose value is the HEX ASCII bytes encded. So qRegisterInfo would have:
dynamic_size_dwarf_expr:112233AABB
where "112233AABB" are the bytes for the DWARF expression:
llvm::dwarf::DW_OP_regx, sr_reg_num, llvm::dwarf::DW_OP_lit1, llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and, llvm::dwarf::DW_OP_regx, config5_reg_num, llvm::dwarf::DW_OP_lit1, llvm::dwarf::DW_OP_lit8, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and, llvm::dwarf::DW_OP_lit18, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_xor
The result of the expression should be the byte size for the register, not a zero or 1. This way, any target can hook up to LLDB and provide the needed info and LLDB will just work. With the above solution, you must go and modify LLDB source code.
See inlined comments for more details.
I really want to get this dynamic register size stuff right so the next target that needs it just works with no modifications to existing code. Thanks for making all the changes.
include/lldb/lldb-private-types.h | ||
---|---|---|
57–58 ↗ | (On Diff #61654) | We shouldn't have to add this as any register size adjustments can be taken care of in: const RegisterInfo * RegisterContext::GetRegisterInfoAtIndex (uint32_t reg_index) const; Another thing we could do is store the DWARF expression bytes right in the RegisterInfo: const uint8_t *dynamic_size_dwarf_expr_bytes; // A DWARF expression that when evaluated gives the byte size of this register size_t dynamic_size_dwarf_len; // The length of the DWARF expression in bytes in the dynamic_size_dwarf_expr_bytes member |
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp | ||
495 ↗ | (On Diff #61654) | Just check if reg_index is in the FPUs. We shouldn't need to add the dynamic_size to RegisterInfo, or we just check the new "dynamic_size_dwarf_expr_bytes" field in RegisterInfo. |
497–505 ↗ | (On Diff #61654) | If we don't add anything to RegisterInfo, then this code is fine. Else we will need to check "reg_info->dynamic_size_dwarf_expr_bytes" and evaluate the DWARF expression to get the size. |
930–963 ↗ | (On Diff #61654) | This whole function goes away if we add "dynamic_size_dwarf_expr_bytes" to RegisterInfo. Else this stays. |
source/Plugins/Process/Utility/DynamicRegisterInfo.cpp | ||
295–297 ↗ | (On Diff #61654) | I would change this to get a "dynamic_size_dwarf_expr" key whose value is a string that has HEXASCII encoded DWARF DW_OP bytes. We wouldn't store this in the "reg_info", but somewhere else locally in this class. Maybe a map of register number to DWARFExpression: typedef std::map<uint32_t, DWARFExpression> DynamicRegisterSizeMap; If we modify RegisterInfo to add the "dynamic_size_dwarf_expr_bytes", then we would store the bytes in there, but we will need to make a copy of the bytes. |
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp | ||
1637–1640 ↗ | (On Diff #61654) | Remove? Or we can change RegisterInfo to contain a "const uint8_t *dynamic_size_dwarf_expr_bytes; size_t dynamic_size_dwarf_len;". |
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp | ||
956–1000 ↗ | (On Diff #61654) | This should be removed in favor or having the DWARF expression supplied to us via the qRegisterInfo or $qXfer:features:read:target.xml:0,1ffff packets. The XML would have a key/value pair for "dynamic_size_dwarf_expr" that would be the DWARF expression. |
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h | ||
49–55 ↗ | (On Diff #61654) | Remove this and just evaluate the DWARF expression that was given to us via the qRegisterInfo or $qXfer:features:read:target.xml:0,1ffff |
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
557 ↗ | (On Diff #61654) | remove |
642–645 ↗ | (On Diff #61654) | change to: else if (name.compare("dynamic_size_dwarf_expr") == 0) { // TODO: store DWARF expression somewhere } |
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp | ||
---|---|---|
497–505 ↗ | (On Diff #61654) | If Dwarf expression need to be evaluate at server side, then we need to add one more function in DWARFExpression::Evaluate to take NativeRegisterContext has a parameter. So should I add one more functionality in it ? Thanks |
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp | ||
---|---|---|
497–505 ↗ | (On Diff #61654) | Please be careful about pulling more code to lldb-server, as it may cause the size to blow up due to transitive dependencies. If you're going to do that, please check the impact on the binary size, but I think it would be best to keep dwarf expression evaluation out of the server altogether. |
There should be no need to add code on the lldb-server side. It would just send the DWARF expression bytes over when qRegisterInfo or the target xml is sent to the host. The host would store this expression and run it each time the register info is asked for for a register so that the host can set the register byte size correctly.
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp | ||
---|---|---|
98 ↗ | (On Diff #63201) | Can we get rid of these const_casts? It looks like UpdateDynamicRegisterSize does not actually need this to be non-const. |
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp | ||
---|---|---|
98 ↗ | (On Diff #63201) | No, it require so that we can modify the reg_info->byte_size member. |
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp | ||
---|---|---|
98 ↗ | (On Diff #63201) | In that case, there is something else wrong with the design here. Well behaved code should never need to call const_cast. m_reg_info is non-const, so maybe you just need a non-const version of DynamicRegisterInfo::GetRegisterInfoAtIndex or something like that... |
Added lldb_private::RegisterInfo * DynamicRegisterInfo::GetRegisterInfoAtIndex (uint32_t i)
Many issues. See inlined comments.
include/lldb/lldb-private-types.h | ||
---|---|---|
57–58 ↗ | (On Diff #63224) | If you choose to add these fields to RegisterInfo.h, then you will need to update all macros for that create arrays of RegisterInfo structs to fill in NULL into dynamic_size_dwarf_expr_bytes, and 0 into dynamic_size_dwarf_len. |
source/Plugins/Process/Utility/DynamicRegisterInfo.cpp | ||
298–299 ↗ | (On Diff #63224) | We don't need a key named "dynamic_size_dwarf_len", we just need "dynamic_size_dwarf_expr_bytes". We can fill in "reg_info.dynamic_size_dwarf_len" after decoding the bytes in "dynamic_size_dwarf_expr_bytes". |
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp | ||
1639 ↗ | (On Diff #63224) | We really don't need a "dynamic_size_dwarf_len" key. Just "dynamic_size_dwarf_expr_bytes" and we can determine the byte size from how many bytes are encoded as hex ASCII chars. |
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp | ||
96–97 ↗ | (On Diff #63224) | Put these two statements inside the "if (reg_info->dynamic_size_dwarf_len)" statement |
99 ↗ | (On Diff #63224) | add space and make sure "reg_info" isn't NULL. if (reg_info && reg_info->dynamic_size_dwarf_len) |
954–994 ↗ | (On Diff #63224) | This should be a function in RegisterContext.h/RegisterContext.cpp and then the first two arguments are not needed. See previous comment for reasons why. |
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h | ||
50–55 ↗ | (On Diff #63224) | This should be moved to RegisterContext.h as any register from any context can have DWARF expressions that describe the byte size. If this is a function on the RegisterContext class, you won't need to first two parameters "exe_ctx" and "reg_ctx" since the RegisterContext contains a member variable named "m_thread" which is a "lldb_private::Thread &". |
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
542 ↗ | (On Diff #63224) | We are going to assume that 8 bytes is enough for any size expression? |
658 ↗ | (On Diff #63224) | We can't use "dwarf_opcode" here. It is a local variable above: uint8_t dwarf_opcode[8]; Then the register info will point to random data on the stack after this function. This need to be dynamically stored on the dynamic register info class so that it maintains a reference to the data so that it lives as long as the RegisterInfo structs do. |
4395 ↗ | (On Diff #63224) | We can't use a local variable for the dwarf_opcode data. |
4505–4508 ↗ | (On Diff #63224) | Don't need this key, remove this code. |
4518 ↗ | (On Diff #63224) | You must dynamically store the dwarf opcode bytes in the dynamic register info class so it keeps the bytes alive as long as the register info structs. |
A few comments:
- we need the DWARF opcode length in RegisterInfo, but not in the structured data (XML or JSON)
- all macros that initialize RegisterInfo structs must be expended to null and zero out the opcode pointer and size (in all register contexts for all archs)
- remove all virtual and overrides for GetDwarfOpcodeLength()
include/lldb/Host/common/NativeRegisterContext.h | ||
---|---|---|
90–92 ↗ | (On Diff #64693) | Remove this, the length needs to stay in the RegisterInfo. |
include/lldb/Host/common/NativeRegisterContextRegisterInfo.h | ||
40–42 ↗ | (On Diff #64693) | Remove this, the length needs to stay in the RegisterInfo. |
include/lldb/Target/RegisterContext.h | ||
53 ↗ | (On Diff #64693) | Remove opcode_len, the length needs to stay in the RegisterInfo. |
include/lldb/lldb-private-types.h | ||
57–58 ↗ | (On Diff #64693) | So we either remove both fields from RegisterInfo, or we leave both in. If we remove them both, then getting the register info from the RegisterContext will update the register info automatically if needed as determined by the RegisterContext subclass. I don't mind them being in RegisterInfo and I think both should stay. In this case, we do need the length for the expression in the RegisterInfo struct, we just don't need to *send* the length in the structured data (XML or JSON). |
source/Host/common/NativeRegisterContext.cpp | ||
273–278 ↗ | (On Diff #64693) | Remove this, the length needs to stay in the RegisterInfo. |
source/Host/common/NativeRegisterContextRegisterInfo.cpp | ||
52–58 ↗ | (On Diff #64693) | Remove this, the length needs to stay in the RegisterInfo. |
source/Plugins/Process/Utility/DynamicRegisterInfo.cpp | ||
317 ↗ | (On Diff #64693) | set the length in RegisterInfo from the size of the bytes extracted. |
320 ↗ | (On Diff #64693) | set the opcode length to zero here. |
424–428 ↗ | (On Diff #64693) | remove. |
435 ↗ | (On Diff #64693) | remove dwarf_expr_bytes_len. |
source/Plugins/Process/Utility/DynamicRegisterInfo.h | ||
43 ↗ | (On Diff #64693) | Remove the dwarf_expr_bytes_len, the length needs to stay in the RegisterInfo. If the opcode and length are non-zero, then a copy of the data will need to be made so it can persist. We already do this with the value_regs and invalidate_regs fields in RegisterInfo. |
57–59 ↗ | (On Diff #64693) | remove |
source/Plugins/Process/Utility/RegisterContextLinux_mips.cpp | ||
72–84 ↗ | (On Diff #64693) | remove |
source/Plugins/Process/Utility/RegisterContextLinux_mips.h | ||
34–35 ↗ | (On Diff #64693) | remove |
source/Plugins/Process/Utility/RegisterContextLinux_mips64.cpp | ||
124–136 ↗ | (On Diff #64693) | remove |
source/Plugins/Process/Utility/RegisterContextLinux_mips64.h | ||
36–38 ↗ | (On Diff #64693) | remove |
source/Plugins/Process/Utility/RegisterInfoInterface.h | ||
51–56 ↗ | (On Diff #64693) | remove |