Messages in this thread | | | Subject | Re: [PATCH 2/3] LoongArch: Add prologue unwinder support | From | zhangqing <> | Date | Fri, 29 Jul 2022 13:39:27 +0800 |
| |
On 2022/7/29 上午10:14, Jinyang He wrote: > Hi, Qing, > > > On 07/28/2022 10:05 PM, Qing Zhang wrote: >> It unwind the stack frame based on prologue code analyze. >> CONFIG_KALLSYMS is needed, at least the address and length >> of each function. >> >> Three stages when we do unwind, >> (1)unwind_start(), the prapare of unwinding, fill unwind_state. >> (2)unwind_done(), judge whether the unwind process is finished or not. >> (3)unwind_next_frame(), unwind the next frame. >> >> Dividing unwinder helps to add new unwinders in the future, eg: >> unwind_frame, unwind_orc .etc >> >> Signed-off-by: Qing Zhang <zhangqing@loongson.cn> >> >> +static inline bool is_stack_alloc_ins(union loongarch_instruction *ip) >> +{ >> + /* addi.d $sp, $sp, -imm */ >> + return ip->reg2i12_format.opcode == addid_op && >> + ip->reg2i12_format.rj == LOONGARCH_GPR_SP && >> + ip->reg2i12_format.rd == LOONGARCH_GPR_SP && >> + ip->reg2i12_format.immediate & (1 << 11); > Checking the sign bit can be used in other place. >> +} >> + >> +static inline bool is_ra_save_ins(union loongarch_instruction *ip) >> +{ >> + /* st.d $ra, $sp, offset */ >> + return ip->reg2i12_format.opcode == std_op && >> + ip->reg2i12_format.rj == LOONGARCH_GPR_SP && >> + ip->reg2i12_format.rd == LOONGARCH_GPR_RA; >> +} >> + >> +static inline bool is_branch_insn(union loongarch_instruction insn) > Does it by using pointer parameter as above functions do. >> +{ >> + return insn.reg1i21_format.opcode >= beqz_op && >> + insn.reg1i21_format.opcode <= bgeu_op; >> +} >> + >> u32 larch_insn_gen_lu32id(enum loongarch_gpr rd, int imm); >> u32 larch_insn_gen_lu52id(enum loongarch_gpr rd, enum loongarch_gpr >> rj, int imm); >> u32 larch_insn_gen_jirl(enum loongarch_gpr rd, enum loongarch_gpr >> rj, unsigned long pc, unsigned long dest); >> diff --git a/arch/loongarch/include/asm/unwind.h >> b/arch/loongarch/include/asm/unwind.h >> index 243330b39d0d..09394e536ea9 100644 >> --- a/arch/loongarch/include/asm/unwind.h >> +++ b/arch/loongarch/include/asm/unwind.h >> @@ -14,6 +14,10 @@ >> struct unwind_state { >> struct stack_info stack_info; >> struct task_struct *task; >> +#if defined(CONFIG_UNWINDER_PROLOGUE) >> + unsigned long ra; >> + bool enable; > Annotating here is appreciating. Enable is the way of prologue analysis > while !enable is the way of guess. >> +#endif >> unsigned long sp, pc; >> bool first; >> bool error; > [...] >> + >> +unsigned long unwind_get_return_address(struct unwind_state *state) >> +{ >> + >> + if (unwind_done(state)) >> + return 0; >> + >> + if (state->enable) >> + return state->pc; >> + else if (state->first) >> + return state->pc; > Combine conditions. >> + >> + return *(unsigned long *)(state->sp); >> + >> +} >> +EXPORT_SYMBOL_GPL(unwind_get_return_address); >> + >> +static bool unwind_by_prologue(struct unwind_state *state) >> +{ >> + struct stack_info *info = &state->stack_info; >> + union loongarch_instruction *ip, *ip_end; >> + unsigned long frame_size = 0, frame_ra = -1; >> + unsigned long size, offset, pc = state->pc; >> + >> + if (state->sp >= info->end || state->sp < info->begin) >> + return false; >> + >> + if (!kallsyms_lookup_size_offset(pc, &size, &offset)) >> + return false; >> + >> + ip = (union loongarch_instruction *)(pc - offset); >> + ip_end = (union loongarch_instruction *)pc; >> + >> + while (ip < ip_end) { >> + if (is_stack_alloc_ins(ip)) { >> + frame_size = (1 << 12) - ip->reg2i12_format.immediate; > Due to there will be other place convert unsigned to signed, we have > a chance that create a inline function in inst.h. Do it as same as > checking the sign bit.Hi, Jinyang
I will fix all in v2. eg: #define is_imm12_negative(val) is_imm_negative(val, 12) static inline bool is_imm_negative(unsigned long val, unsigned int bit) { return val & (1UL << (bit-1)); }
static inline bool is_stack_alloc_ins(union loongarch_instruction *ip) { ... ip->reg2i12_format.rd == LOONGARCH_GPR_SP && is_imm12_negative(ip->reg2i12_format.immediate); }
static inline bool is_ra_save_ins(union loongarch_instruction *ip) { ... !is_imm12_negative(ip->reg2i12_format.immediate); }
Thanks, Qing > >> + ip++; >> + break; >> + } >> + ip++; >> + } >> + > [...] >> + >> + do { >> + if (state->enable) { >> + if (unwind_by_prologue(state)) >> + return true; >> + >> + if (info->type == STACK_TYPE_IRQ && >> + info->end == state->sp) { >> + regs = (struct pt_regs *)info->next_sp; >> + pc = regs->csr_era; >> + if (user_mode(regs) || !__kernel_text_address(pc)) >> + return false; >> + >> + state->pc = pc; >> + state->sp = regs->regs[3]; >> + state->ra = regs->regs[1]; >> + state->first = true; >> + get_stack_info(state->sp, state->task, info); >> + >> + return true; >> + } >> + } else { >> + if (state->first) >> + state->first = false; >> + >> + if (unwind_by_guess(state)) >> + return true; >> + } > I'd prefer separate the block of 'if...else...' into two inline > functions, that makes codes clear. > > Thanks, > Jinyang
| |