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)

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