Re: [PATCH 2/3] LoongArch: Add prologue unwinder support

[zhangqing]

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@xxxxxxxxxxx>
> >
> > +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