Hi Jason,
The change you have suggested will cause problems for MIPS.
Consider below function for example,
0000000120000ca0 <bar>:
120000ca0: 67bdffc0 daddiu sp,sp,-64
120000ca4: ffbf0038 sd ra,56(sp)
120000ca8: ffbe0030 sd s8,48(sp)
120000cac: ffbc0028 sd gp,40(sp)
120000cb0: 03a0f02d move s8,sp
120000cb4: 3c1c0002 lui gp,0x2
120000cb8: 0399e02d daddu gp,gp,t9
120000cbc: 679c83a0 daddiu gp,gp,-31840
[...]
120000d08: 03c0e82d move sp,s8
120000d0c: dfbf0038 ld ra,56(sp) //offset from start of
function ->108
120000d10: dfbe0030 ld s8,48(sp) //offset from start of
function ->112
120000d14: dfbc0028 ld gp,40(sp) //offset from start of
function ->116
120000d18: 67bd0040 daddiu sp,sp,64//offset from start of function
->120
120000d1c: 03e00008 jr ra //offset from start of
function ->124
120000d20: 00000000 nop
With the current implementation, the unwinder gives following unwind info for
the function:
(lldb) target modules show-unwind -n bar
<stripped some text>
row[0]: 0: CFA=sp +0 => pc=ra
row[1]: 4: CFA=sp+64 => pc=ra
row[2]: 8: CFA=sp+64 => ra=[CFA-8] pc=[CFA-8]
row[3]: 12: CFA=sp+64 => fp=[CFA-16] ra=[CFA-8] pc=[CFA-8]
row[4]: 112: CFA=sp+64 => fp=[CFA-16] ra=ra pc=ra
row[5]: 116: CFA=sp+64 => fp=fp ra=ra pc=ra
row[6]: 124: CFA=sp +0 => fp=fp ra=ra pc=ra
After your change is applied, the unwinder gives following output:
(lldb) target modules show-unwind -n bar
<stripped some text>
row[0]: 0: CFA=sp +0 => pc=ra
row[1]: 4: CFA=sp+64 => pc=ra
row[2]: 8: CFA=sp+64 => ra=[CFA-8] pc=[CFA-8]
row[3]: 12: CFA=sp+64 => fp=[CFA-16] ra=[CFA-8] pc=[CFA-8]
row[4]: 124: CFA=sp +0 => fp=[CFA-16] ra=[CFA-8] pc=[CFA-8]
Note the registers restore happening in epilogue (at 120000d0c and at
120000d10) is not getting reflected in unwind rules.
So the rows for offsets 112 and 116 are not getting emitted.
In my patch, It seems that I have misunderstood the meaning of
EmulateInstruction::eContextRegisterLoad.
As you stated it means "a register saved into another register", however I have
used it for "a register is restored/loaded from stack in epilogue".
If this is causing problems for other targets like ARM then can we add a new
ContextType say 'eContextRegisterRestore' and then
- move code from eContextRegisterLoad into eContextRegisterRestore in
UnwindAssemblyInstEmulation::WriteRegister(). and
- leave eContextRegisterLoad unimplemented (as it was before my patch).
[................]
case EmulateInstruction::eContextRegisterLoad:
break;
case EmulateInstruction::eContextRegisterRestore:
{
const uint32_t unwind_reg_kind =
m_unwind_plan_ptr->GetRegisterKind();
const uint32_t reg_num = reg_info->kinds[unwind_reg_kind];
if (reg_num != LLDB_INVALID_REGNUM)
{
m_curr_row->SetRegisterLocationToRegister (reg_num, reg_num,
must_replace);
m_curr_row_modified = true;
m_curr_insn_restored_a_register = true;
if (reg_info->kinds[eRegisterKindGeneric] ==
LLDB_REGNUM_GENERIC_RA)
{
// This load was restoring the return address register,
// so this is also how we will unwind the PC...
RegisterInfo pc_reg_info;
if (instruction->GetRegisterInfo (eRegisterKindGeneric,
LLDB_REGNUM_GENERIC_PC, pc_reg_info))
{
uint32_t pc_reg_num =
pc_reg_info.kinds[unwind_reg_kind];
if (pc_reg_num != LLDB_INVALID_REGNUM)
{
m_curr_row->SetRegisterLocationToRegister
(pc_reg_num, reg_num, must_replace);
m_curr_row_modified = true;
}
}
}
}
}
[...........]
-Bhushan
-----Original Message-----
From: Jason Molenda [mailto:[email protected]]
Sent: Thursday, April 23, 2015 11:22 AM
To: Bhushan Attarde
Cc: lldb-commits
Subject: Re: [Lldb-commits] [lldb] r232619 - Initial Assembly profiler for
mips64
Hi, a small followup to this patch. I noticed that our arm UnwindProfiles are
having problems recently and tracked it down to the change in
UnwindAssemblyInstEmulation. The patch adds code to
UnwindAssemblyInstEmulation which recognizes
EmulateInstruction::eContextRegisterLoad -- a register saved into another
register.
The problem is when we save the caller's register value on the stack and then
*reuse* that register for something else. For instance, an armv7 code sequence
like
0x286b38 <+0>: push {r4, r5, r6, r7, lr}
0x286b3a <+2>: add r7, sp, #0xc
[...]
0x286b52 <+26>: blx 0xa334bc ; symbol stub for:
objc_msgSend
0x286b56 <+30>: mov r7, r7
r7 is being used for a frame pointer in this code. We save the caller's frame
pointer on the stack in the first instruction. Then at <+30>, we do a
register-to-register mov of r7. The instruction emulation should recognize the
initial save but then any changes to r7 should be ignored for the rest of the
function.
(I don't know what 'mov r7,r7' accomplishes in arm - that looks like a no-op to
me but maybe it has some behavior that I don't recognize; I'm not an arm expert)
I *can* work around this with a patch like (I omitted the indentation of the
block of code so the patch is easier to read). When we've got a "THIS register
was saved into ..." instruction, we only update the unwind rule for THIS if it
hasn't already been saved.
Index:
source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
===================================================================
--- source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
(revision 235572)
+++ source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
(working copy)
@@ -639,6 +639,10 @@
const uint32_t reg_num = reg_info->kinds[unwind_reg_kind];
if (reg_num != LLDB_INVALID_REGNUM)
{
+ UnwindPlan::Row::RegisterLocation current_reg_unwind;
+ if (m_curr_row->GetRegisterInfo (reg_num,
current_reg_unwind) == false
+ || current_reg_unwind.IsSame() == true)
+ {
m_curr_row->SetRegisterLocationToRegister (reg_num,
reg_num, must_replace);
m_curr_row_modified = true;
m_curr_insn_restored_a_register = true; @@ -658,6 +662,7 @@
}
}
}
+ }
}
}
break;
Can you try this on your mips architecture and see if it causes problems?
fwiw I have to do something like this with the x86 instruction unwinder too.
Once a register is saved, I ignore any "saves" of that register for the rest of
the function.
J
> On Mar 18, 2015, at 2:21 AM, Bhushan D. Attarde <[email protected]>
> wrote:
>
> Author: bhushan.attarde
> Date: Wed Mar 18 04:21:29 2015
> New Revision: 232619
>
> URL: http://llvm.org/viewvc/llvm-project?rev=232619&view=rev
> Log:
> Initial Assembly profiler for mips64
>
> Summary:
> This is initial implementation of assembly profiler which only scans
> prologue/epilogue assembly instructions to create CFI instructions.
>
> Reviewers: clayborg, jasonmolenda
>
> Differential Revision: http://reviews.llvm.org/D7696
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
