Re: [RFC PATCH v1 08/16] unwind_user: Enable archs that save RA/FP in other registers

[Jens Remus <jremus@linux.ibm.com>]

On 17.07.2025 04:50, Josh Poimboeuf wrote:
> On Wed, Jul 16, 2025 at 07:01:09PM -0700, Josh Poimboeuf wrote:
>>>  	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 it retains its
value.

> So it's been a while since I looked at the original code, but I actually
> think there's a bug here.
> 
> There's a subtlety in the original code:
> 
> 	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
> 		goto done;
> 
> 	state->ip = ra;
> 	state->sp = cfa;
> 	if (frame->fp_off)
> 		state->fp = fp;
> 
> 	arch_unwind_user_next(state);
> 
> Note that unlike !frame->ra_off, !frame->fp_off does NOT end the unwind.
> That only means the FP offset is unknown for the current frame.  Which
> is a perfectly valid condition, e.g. if the function doesn't have frame
> pointers or if it's before the prologue.
> 
> In that case, the unwind should continue, and state->fp's existing value
> should be preserved, as it might already have a valid value from a
> previous frame.

I fully agree with all of the above.

> So the following is wrong:
> 
> 	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;
> 
> Instead of having !fp.frame_off stopping the unwind, it should just
> break out of the switch statement and keep going.

If frame->fp.loc is UNWIND_USER_LOC_STACK then frame->fp.frame_off must
have a value != 0.  At least if we keep the original semantic.

We can omit this check, if we assume all producers of frame behave
correctly.  For instance user unwind sframe would not produce that
(see below).  Could it somehow be made a debug-config-only check?

> And that means the following is also wrong:
> 
> 	state->ip = ra;
> 	state->sp = sp;
> 	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
> 		state->fp = fp;
> 
> because state->fp needs to preserved for the STACK+!fp.frame_off case.

unwind user sframe will not produce that:

static inline void
sframe_init_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;
	}
}

> So, something like this?
> 
> 	bool preserve_fp = false;
> 	...
> 
> 	/* Get the Frame Pointer (FP) */
> 	switch (frame->fp.loc) {
> 	case UNWIND_USER_LOC_NONE:
> 		preserve_fp = true;
> 		break;
> 	case UNWIND_USER_LOC_STACK:
> 		if (!frame->fp.frame_off) {
> 			preserve_fp = true;
> 			break;
> 		}
> 	...
> 
> 	state->ip = ra;
> 	state->sp = sp;
> 	if (!preserve_fp)
> 		state->fp = fp;

I don't think all of this is necessary.

frame->fp.loc == UNWIND_USER_LOC_NONE already indicates the state->fp
retains it's previous value.

> BTW, I would suggest renaming "frame_off" to "offset",
> "frame->fp.offset" reads better and is more compact.

Makes sense.  I'll change that.

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/