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?
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.
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.
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.
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;
BTW, I would suggest renaming "frame_off" to "offset",
"frame->fp.offset" reads better and is more compact.
--
Josh
