On 17.07.2025 04:01, Josh Poimboeuf wrote:
> 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
Thanks! I will use use suggestion.
>> +/**
>> + * 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)
Ditto.
>> @@ -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()?
Whatever Steve and you agree on. I am new to Kernel development.
>> }
>>
>> /* 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.
See my comments below.
> Do we also need UNWIND_USER_LOC_PT_REGS?
Sorry, I cannot follow. Do you suggest to rename UNWIND_USER_LOC_REG to
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.
Restoring from arbitrary registers is only valid in the topmost frame,
as their values (i.e. task_pt_regs(current)) are only available there.
For other frames only SP, FP, and RA register values are available.
I think this test makes sense. Is this test really that expensive?
>> + if (unwind_user_get_reg(&fp, frame->fp.regnum))
>> + goto done;
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> goto done;
>> + }
>
> BUG(1) here as well.
Same as for other WARN_ON_ONCE() vs. BUG().
>> 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?
No. But fp could be initialized to state->fp, so that when it is not
saved and thus not restored it keeps it previous value.
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@xxxxxxxxxx
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
