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

[Bhushan Attarde]

Thanks jasonmolenda for your review and comments.
I will do the corrections as suggested by you.
My comments are inlined.


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;
----------------
jasonmolenda wrote:
> 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.
Yes, mips32 support will be added later.

================
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;
----------------
jasonmolenda wrote:
> 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.
Yes, mips64 only ISA has fixed instruction length, however this function will be useful when micromips support will be added. micromips has mix sized instructions (2 and 4 bytes).

================
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)
+{
----------------
jasonmolenda wrote:
> 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)
I will correct this.

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

================
Comment at: source/Plugins/UnwindAssembly/mips/UnwindAssembly-mips.cpp:451
@@ +450,3 @@
+
+    unwind_plan.SetPlanValidAddressRange (m_func_bounds);
+    unwind_plan.SetRegisterKind (eRegisterKindLLDB);
----------------
jasonmolenda wrote:
> 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.)
Ok, I will remove this code (line 451).

http://reviews.llvm.org/D7696

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