[Lldb-commits] [PATCH] D20357: [LLDB][MIPS] Fix FPU Size Based on Dynamic FR
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Tue Jul 12 10:27:31 PDT 2016
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Many issues. See inlined comments.
================
Comment at: include/lldb/lldb-private-types.h:57-58
@@ -56,1 +56,4 @@
// ax, ah, and al.
+ 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
----------------
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.
================
Comment at: source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:298-299
@@ +297,4 @@
+ uint8_t dynamic_size = 0;
+ reg_info_dict->GetValueForKeyAsInteger("dynamic_size_dwarf_len", dynamic_size);
+ reg_info.dynamic_size_dwarf_len = dynamic_size;
+
----------------
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".
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1639
@@ +1638,3 @@
+ {
+ response.Printf("dynamic_size_dwarf_len:%" PRIu64 ";",reg_info->dynamic_size_dwarf_len);
+ response.PutCString("dynamic_size_dwarf_expr_bytes:");
----------------
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.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:96-97
@@ -91,2 +95,4 @@
{
- return m_reg_info.GetRegisterInfoAtIndex (reg);
+ ExecutionContext exe_ctx (CalculateThread());
+ const ArchSpec &arch = m_thread.GetProcess()->GetTarget().GetArchitecture();
+ RegisterInfo* reg_info = m_reg_info.GetRegisterInfoAtIndex (reg);
----------------
Put these two statements inside the "if (reg_info->dynamic_size_dwarf_len)" statement
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:99
@@ +98,3 @@
+ RegisterInfo* reg_info = m_reg_info.GetRegisterInfoAtIndex (reg);
+ if(reg_info->dynamic_size_dwarf_len)
+ {
----------------
add space and make sure "reg_info" isn't NULL.
```
if (reg_info && reg_info->dynamic_size_dwarf_len)
```
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:954-994
@@ -941,2 +953,43 @@
+uint32_t
+GDBRemoteDynamicRegisterInfo::UpdateDynamicRegisterSize (ExecutionContext *exe_ctx,
+ RegisterContext *reg_ctx,
+ const lldb_private::ArchSpec &arch,
+ RegisterInfo* reg_info, size_t reg)
+{
+ // In MIPS, the floating point registers size is depends on FR.
+ // if SR.26 == 1 then all floating point registers are 64 bits.
+ // else they are all 32 bits.
+
+ int evaluate_result;
+ uint8_t opcode_len = reg_info->dynamic_size_dwarf_len;
+ uint32_t addr_size = arch.GetAddressByteSize ();
+ uint8_t* opcode_ptr = m_dynamic_reg_size_map[reg].data();
+ DataExtractor dwarf_data (opcode_ptr, opcode_len,
+ arch.GetByteOrder (),addr_size);
+ ModuleSP opcode_ctx;
+ DWARFExpression dwarf_expr (opcode_ctx, dwarf_data, nullptr, 0, opcode_len);
+ 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, opcode_len, eRegisterKindDWARF, nullptr, nullptr, result, &error))
+ {
+ evaluate_result = result.GetScalar().SInt(-1);
+ switch (evaluate_result)
+ {
+ case 0: return 4;break;
+ case 1: return 8;break;
+ case -1: return reg_info->byte_size; break;
+ default: assert(false && "Incorrect Dwarf Opcode bytes");
+ break;
+ }
+ return 0;
+ }
+ else
+ {
+ printf("Error executing DwarfExpression::Evaluate %s\n", error.AsCString());
+ return reg_info->byte_size;
+ }
+}
----------------
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.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h:50-55
@@ -49,1 +49,8 @@
+
+ // Detect the register size dynamically.
+ uint32_t
+ UpdateDynamicRegisterSize (ExecutionContext *exe_ctx,
+ RegisterContext *reg_ctx,
+ const lldb_private::ArchSpec &arch,
+ RegisterInfo* reg_info, size_t reg);
};
----------------
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 &".
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:542
@@ -541,2 +541,3 @@
std::vector<uint32_t> invalidate_regs;
+ uint8_t dwarf_opcode[8];
RegisterInfo reg_info = { NULL, // Name
----------------
We are going to assume that 8 bytes is enough for any size expression?
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:658
@@ +657,3 @@
+ assert(dwarf_len == ret_val);
+ reg_info.dynamic_size_dwarf_expr_bytes = dwarf_opcode;
+ }
----------------
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.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4395
@@ -4375,2 +4394,3 @@
std::vector<uint32_t> invalidate_regs;
+ uint8_t dwarf_opcode[8];
bool encoding_set = false;
----------------
We can't use a local variable for the dwarf_opcode data.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4505-4508
@@ -4482,1 +4504,6 @@
}
+ else if (name == "dynamic_size_dwarf_len")
+ {
+ reg_info.dynamic_size_dwarf_len = StringConvert::ToUInt32 (value.data(), 0, 0);
+ }
+ else if (name == "dynamic_size_dwarf_expr_bytes")
----------------
Don't need this key, remove this code.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4518
@@ +4517,3 @@
+ assert(dwarf_len == ret_val);
+ reg_info.dynamic_size_dwarf_expr_bytes = dwarf_opcode;
+ }
----------------
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.
http://reviews.llvm.org/D20357
More information about the lldb-commits
mailing list