On 17.07.2025 04:01, Josh Poimboeuf wrote:
On Thu, Jul 10, 2025 at 06:35:14PM +0200, Jens Remus wrote:
quoted
+#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.
quoted
+/**
+ * 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.
quoted
@@ -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.
quoted
}
/* 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?
quoted
+ 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?
quoted
+ 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().
quoted
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@de.ibm.com
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/