[Lldb-commits] [PATCH] Initial Assembly profiler for mips64

[Jason Molenda]

This looks pretty good, thanks for putting this together.  I apologize for not having time to get back to you about this patch earlier.

I made a few in-line comments, they are simple changes.  Please feel free to commit once you've addressed them if you have commit access.  I didn't review the patch for correct unwinding of mips64 stack frames, I'll take your word that part is correct :) but in general it looks good.


REPOSITORY
  rL LLVM

================
Comment at: source/Plugins/UnwindAssembly/mips/UnwindAssembly-mips.cpp:193
@@ +192,3 @@
+    if (cpu == k_mips64)
+    {
+        m_machine_ra_regnum = k_machine_ra;
----------------
Only mips64 is supported by this code right now.  Do you think there will be mips32 support added later?  The conditionalized code to set up the register numbers (e.g. m_machine_ra_regnum) or get the number of registers defined in this ctor are only doing one thing right now with mips64 support.  If you want to leave this conditionalized code in place so mips32 is easier to add in the future, that's fine.

================
Comment at: source/Plugins/UnwindAssembly/mips/UnwindAssembly-mips.cpp:295
@@ +294,3 @@
+{
+    const uint32_t max_op_byte_size = m_arch.GetMaximumOpcodeByteSize();
+    llvm::SmallVector <uint8_t, 32> opcode_data;
----------------
Doesn't the mips64 ISA have a fixed instruction length?  It seems like instruction_length() should always return 4 - I don't think I'd even define the method if it were me.

For that matter, the only reason the x86 assembly profiler creates an llvm disassembler is to get the instruction length.  I don't think the m_disasm_context ivar is needed in your profiler.

================
Comment at: source/Plugins/UnwindAssembly/mips/UnwindAssembly-mips.cpp:369
@@ +368,3 @@
+// daddiu  sp,sp,-X [01001 11101 11101 xxxxxxxxxxxxxxxx]
+bool AssemblyParse_mips::add_imm_sp_pattern_p (int& amount)
+{
----------------
I'm sure you copied this same bad format from UnwindAssembly-x86.cpp but please put the return type on a line by itself to be consistent with the lldb coding style.  UnwindAssembly-x86.cpp was written early in lldb's bringup and the code style is a little off - I've been cleaning it up slowly over time when I notice mistakes but I'm sure these are still in that source file.  So,

bool
AssemblyParse_mips::add_imm_sp_pattern_p (int& amount)

(and the same for the rest of the methods also doing this)

================
Comment at: source/Plugins/UnwindAssembly/mips/UnwindAssembly-mips.cpp:418
@@ +417,3 @@
+
+    if(p == 0x03a0f02d)
+      return true;
----------------
We prefer a space between the "if" and the expression - this also comes up in mov_fp_to_sp_pattern_p().

================
Comment at: source/Plugins/UnwindAssembly/mips/UnwindAssembly-mips.cpp:451
@@ +450,3 @@
+
+    unwind_plan.SetPlanValidAddressRange (m_func_bounds);
+    unwind_plan.SetRegisterKind (eRegisterKindLLDB);
----------------
If you didn't have a function bounds you cap it to 512 bytes in the ctor.  You might not want to set a PlanValidAddressRange() here.  I don't remember if the unwinder actually makes use of the valid address range or not -- but it's conceivable that someone could be stopped at byte 516 in the function and the unwinder wouldn't use your unwind plan.  (or worse, the unwinder would start to use the offset in the future and break the mips64 unwinds.)

http://reviews.llvm.org/D7696

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/