Yeah, I see the problem with my change. I'll need to look at this more closely. We may need to add something to the arm insn emulation that doesn't flag mov rN,rN as doing anything. I need to go read the manual and make sure that's the case.
Thanks. I'll check with you before modifying anything here. > On Apr 23, 2015, at 4:40 AM, Bhushan Attarde <[email protected]> > wrote: > > 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
