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