On Thu, Jul 10, 2025 at 06:35:14PM +0200, Jens Remus wrote:
> +#ifndef unwind_user_get_reg
> +
> +/**
> + * generic_unwind_user_get_reg - Get register value.
> + * @val: Register value.
> + * @regnum: DWARF register number to obtain the value from.
> + *
> + * Returns zero if successful. Otherwise -EINVAL.
> + */
> +static inline int generic_unwind_user_get_reg(unsigned long *val, int regnum)
> +{
> + return -EINVAL;
> +}
> +
> +#define unwind_user_get_reg generic_unwind_user_get_reg
> +
> +#endif /* !unwind_user_get_reg */
I believe the traditional way to do this is to give the function the
same name as the define:
#ifndef unwind_user_get_reg
static inline int unwind_user_get_reg(unsigned long *val, int regnum)
{
return -EINVAL;
}
#define unwind_user_get_reg unwind_user_get_reg
#endif
> +/**
> + * generic_sframe_set_frame_reginfo - Populate info to unwind FP/RA register
> + * from SFrame offset.
> + * @reginfo: Unwind info for FP/RA register.
> + * @offset: SFrame offset value.
> + *
> + * A non-zero offset value denotes a stack offset from CFA and indicates
> + * that the register is saved on the stack. A zero offset value indicates
> + * that the register is not saved.
> + */
> +static inline void generic_sframe_set_frame_reginfo(
> + struct unwind_user_reginfo *reginfo,
> + s32 offset)
> +{
> + if (offset) {
> + reginfo->loc = UNWIND_USER_LOC_STACK;
> + reginfo->frame_off = offset;
> + } else {
> + reginfo->loc = UNWIND_USER_LOC_NONE;
> + }
> +}
This just inits the reginfo struct, can we call it sframe_init_reginfo()?
Also the function comment seems completely superfluous as the function
is completely obvious.
Also the signature should match kernel style, something like:
static inline void
sframe_init_reginfo(struct unwind_user_reginfo *reginfo, s32 offset)
> @@ -98,26 +98,57 @@ static int unwind_user_next(struct unwind_user_state *state)
>
>
> /* Get the Return Address (RA) */
> - if (frame->ra_off) {
> + switch (frame->ra.loc) {
> + case UNWIND_USER_LOC_NONE:
> + if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
> + goto done;
> + ra = user_return_address(task_pt_regs(current));
> + break;
> + case UNWIND_USER_LOC_STACK:
> + if (!frame->ra.frame_off)
> + goto done;
> /* Make sure that the address is word aligned */
> shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3;
> - if ((cfa + frame->ra_off) & ((1 << shift) - 1))
> + if ((cfa + frame->ra.frame_off) & ((1 << shift) - 1))
> goto done;
> - if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
> + if (unwind_get_user_long(ra, cfa + frame->ra.frame_off, state))
> goto done;
> - } else {
> - if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
> + break;
> + case UNWIND_USER_LOC_REG:
> + if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
> goto done;
> - ra = user_return_address(task_pt_regs(current));
> + if (unwind_user_get_reg(&ra, frame->ra.regnum))
> + goto done;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + goto done;
The default case will never happen, can we make it a BUG()?
> }
>
> /* Get the Frame Pointer (FP) */
> - if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
> + switch (frame->fp.loc) {
> + case UNWIND_USER_LOC_NONE:
> + break;
The UNWIND_USER_LOC_NONE behavior is different here compared to above.
Do we also need UNWIND_USER_LOC_PT_REGS?
> + case UNWIND_USER_LOC_STACK:
> + if (!frame->fp.frame_off)
> + goto done;
> + if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
> + goto done;
> + break;
> + case UNWIND_USER_LOC_REG:
> + if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
> + goto done;
The topmost checking is *really* getting cumbersome, I do hope we can
get rid of that.
> + if (unwind_user_get_reg(&fp, frame->fp.regnum))
> + goto done;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> goto done;
> + }
BUG(1) here as well.
> state->ip = ra;
> state->sp = sp;
> - if (frame->fp_off)
> + if (frame->fp.loc != UNWIND_USER_LOC_NONE)
> state->fp = fp;
Instead of the extra conditional here, can fp be initialized to zero?
Either at the top of the function or in the RA switch statement?
--
Josh
