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

On Fri, Jan 24, 2025 at 1:41 PM Josh Poimboeuf <[email protected]> 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 <[email protected]> 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;
...

}