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

[Zachary Turner]

I would like to ask for some tests before this gets committed.
On Wed, Feb 18, 2015 at 8:00 PM Jason Molenda <jmolenda at apple.com> wrote:

> 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/
>
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150219/9e38bcce/attachment.html>