Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading .sframe contents

Weinan Liu
2025-01-30T19:51:18Z

On Fri, Jan 24, 2025 at 1:41 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> On Fri, Jan 24, 2025 at 10:02:46AM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > +static __always_inline int __find_fre(struct sframe_section *sec,
> > > + struct sframe_fde *fde, unsigned long ip,
> > > + struct unwind_user_frame *frame)
> > > +{
> > > + unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> > > + struct sframe_fre *fre, *prev_fre = NULL;
> > > + struct sframe_fre fres[2];
> >
> > you only need prev_fre->ip_off, so why all this `which` and `fres[2]`
> > business if all you need is prev_fre_off and a bool whether you have
> > prev_fre at all?
>
> So this cleverness probably needs a comment. prev_fre is actually
> needed after the loop:
>
> > > + if (!prev_fre)
> > > + return -EINVAL;
> > > + fre = prev_fre;
>
> In the body of the loop, prev_fre is a tentative match, unless the next
> fre also matches, in which case that one becomes the new tentative
> match.
>
> I'll add a comment. Also it'll probably be less confusing if I rename
> "prev_fre" to "fre", and "fre" to "next_fre".
>

Nit: swap() might be a simplify way to alternate pointers between two
fre_addr[] entries.

For example,

static __always_inline int __find_fre(struct sframe_section *sec,
struct sframe_fde *fde, unsigned long ip,
struct unwind_user_frame *frame)
{
/* intialize fres[] with invalid value */
struct sframe_fre fres[2] = {0};
struct sframe_fre *fre = &fres[1], *prev_fre = fres;

for (i = 0; i < fde->fres_num; i++) {
swap(fre, next_fre);
ret = __read_fre(sec, fde, fre_addr, fre);
...

if (fre->ip_off > ip_off)
break;
}

if (fre->size == 0)
return -EINVAL;
...

}