[Lldb-commits] [PATCH] D20357: [LLDB][MIPS] Fix FPU Size Based on Dynamic FR/FRE bit
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Thu Jun 23 11:02:01 PDT 2016
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
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.
================
Comment at: include/lldb/lldb-private-types.h:57-58
@@ -56,2 +56,4 @@
// ax, ah, and al.
+ uint8_t dynamic_size; // if this value is 1 then size of this register is decided at run time.
+ // Default value is 0
};
----------------
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
```
================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:495
@@ +494,3 @@
+
+ if (reg_info->dynamic_size)
+ {
----------------
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.
================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:497-505
@@ +496,11 @@
+ {
+ bool fre;
+ bool fr1;
+ IsFR1_FRE (fr1, fre);
+
+ // fr1 fre fpu_reg_size
+ // 1 0 64
+ // 1 1 32
+ // 0 0 32
+ reg_info->byte_size = (fr1 ^ fre) ? 8 : 4;
+ }
----------------
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.
================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:930-963
@@ -903,1 +929,36 @@
+void
+NativeRegisterContextLinux_mips64::IsFR1_FRE (bool& fr1, bool& fre) const
+{
+ GPR_linux_mips regs;
+ RegisterValue reg_value;
+ const RegisterInfoInterface& reg_info_interface = GetRegisterInfoInterface ();
+ const RegisterInfo* reg_info_sr = reg_info_interface.GetRegisterInfo () + gpr_sr_mips64 ;
+ const RegisterInfo* reg_info_config5 = reg_info_interface.GetRegisterInfo () + gpr_config5_mips;
+ lldb_private::ArchSpec arch;
+ ::memset (®s, 0, sizeof(GPR_linux_mips));
+
+ fre = 0;
+ if (!(m_thread.GetProcess ()->GetArchitecture (arch)))
+ assert (false && "Failed to get Architecture.");
+
+ errno = 0;
+
+ // Reading SR/Config5 using PTRACE_PEEKUSER is not supported. So read entire register set
+ Error error = NativeProcessLinux::PtraceWrapper (PTRACE_GETREGS, m_thread.GetID(), NULL, ®s, sizeof regs);
+ if (!error.Success())
+ assert (false && "Unable to Read Register");
+
+ void* target_address = ((uint8_t*)®s) + reg_info_sr->byte_offset + 4 * (arch.GetMachine () == llvm::Triple::mips);
+ reg_value.SetBytes (target_address, 4, arch.GetByteOrder());
+ fr1 = reg_value.GetAsUInt32() & SR_FR;
+
+ // FRE is valid only when FR bit is set
+ if (fr1)
+ {
+ target_address = ((uint8_t*)®s) + reg_info_config5->byte_offset +
+ 4 * (arch.GetMachine () == llvm::Triple::mips);
+ reg_value.SetBytes(target_address, 4, arch.GetByteOrder ());
+ fre = reg_value.GetAsUInt32() & CONFIG5_FRE;
+ }
+}
----------------
This whole function goes away if we add "dynamic_size_dwarf_expr_bytes" to RegisterInfo. Else this stays.
================
Comment at: source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:295-297
@@ -294,1 +294,5 @@
+
+ uint8_t dynamic_size = 0;
+ reg_info_dict->GetValueForKeyAsInteger("dynamic_size", dynamic_size);
+ reg_info.dynamic_size = dynamic_size;
----------------
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.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1637-1640
@@ -1636,2 +1636,6 @@
+ if (reg_info->dynamic_size)
+ {
+ response.Printf ("dynamic_size:%" PRIu32 ";", 1);
+ }
return SendPacketNoLock(response.GetData(), response.GetSize());
----------------
Remove? Or we can change RegisterInfo to contain a "const uint8_t *dynamic_size_dwarf_expr_bytes; size_t dynamic_size_dwarf_len;".
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:956-1000
@@ -941,2 +955,47 @@
+bool
+GDBRemoteDynamicRegisterInfo::UpdateDynamicRegisterSize (ExecutionContext *exe_ctx,
+ RegisterContext *reg_ctx,
+ const lldb_private::ArchSpec &arch,
+ lldb::RegisterKind reg_kind)
+{
+ lldb_private::ConstString sr_reg_name("sr");
+ lldb_private::RegisterInfo *reg_info = GetRegisterInfo (sr_reg_name);
+ uint8_t sr_reg_num = reg_info->kinds[reg_kind];
+ lldb_private::ConstString config_reg_name("config5");
+ reg_info = GetRegisterInfo (config_reg_name);
+ uint8_t config5_reg_num = reg_info->kinds[reg_kind];
+
+ // In MIPS, the floating point registers size is depends on FR and FRE bit.
+ // if SR.26 ^ config5.8 == 1 then all floating point registers are 64 bits.
+ // else they are all 32 bits.
+ const uint8_t dwarf_opcode [] = {
+ 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
+ };
+
+
+ uint32_t addr_size = arch.GetAddressByteSize ();
+ DataExtractor dwarf_data (dwarf_opcode, sizeof(dwarf_opcode), arch.GetByteOrder (),
+ addr_size);
+ ModuleSP opcode_ctx;
+ DWARFExpression dwarf_expr (opcode_ctx, dwarf_data, nullptr, 0,sizeof(dwarf_opcode));
+ Value result;
+ Error error;
+ const lldb::offset_t offset = 0;
+ if(dwarf_expr.Evaluate (exe_ctx, nullptr, nullptr, reg_ctx, opcode_ctx, dwarf_data, nullptr,
+ offset, sizeof(dwarf_opcode), reg_kind, nullptr, nullptr, result, &error))
+ {
+ return result.GetScalar().UInt(1);
+ }
+ else
+ {
+ printf("Error executing DwarfExpression::Evaluate %s\n", error.AsCString());
+ return true;
+ }
+}
----------------
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.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h:49-55
@@ -48,2 +48,9 @@
HardcodeARMRegisters(bool from_scratch);
+
+ // Detect the register size dynamically.
+ bool
+ UpdateDynamicRegisterSize (ExecutionContext *exe_ctx,
+ RegisterContext *reg_ctx,
+ const lldb_private::ArchSpec &arch,
+ lldb::RegisterKind reg_kind);
};
----------------
Remove this and just evaluate the DWARF expression that was given to us via the qRegisterInfo or $qXfer:features:read:target.xml:0,1ffff
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:557
@@ -557,1 +556,3 @@
+ NULL,
+ 0 // dynamic_size default value
};
----------------
remove
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:642-645
@@ -640,2 +641,6 @@
}
+ else if (name.compare("dynamic_size") == 0)
+ {
+ reg_info.dynamic_size = 1;
+ }
}
----------------
change to:
```
else if (name.compare("dynamic_size_dwarf_expr") == 0)
{
// TODO: store DWARF expression somewhere
}
```
Repository:
rL LLVM
http://reviews.llvm.org/D20357
More information about the lldb-commits
mailing list