Re: [PATCH] cg: allow providers to specify a skip count for stack retrieval

[Kris Van Hees <kris.van.hees@oracle.com>]

On Wed, May 01, 2024 at 03:40:09PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> 
> I haven't played much with stack/fentry/fexit, so I hope this is right
> (e.g., stable number of skip frames).  Are our tests robust enough to assure
> us?
> 
> BUT!  What about stackdepth in bpf/get_bvar.c?

Ah yes, need to adjust that one also.  Thanks!

Incidentally, caller also needs to be fixed in this way.

AND this skip should NOT be applied to ustack() (nor ucaller and ustackdepth).

v2 coming soon :)

> On 5/1/24 14:22, Kris Van Hees wrote:
> > Some probes cause extra frames to be reported with bpf_get_stack() that
> > need to be skipped to ensure we report the correct stack trace to the
> > consumer.  FBT probes based on fentry/fexit probes need this.
> > 
> > Signed-off-by: Kris Van Hees<kris.van.hees@oracle.com>
> > ---
> >   libdtrace/dt_cg.c       | 6 ++++--
> >   libdtrace/dt_prov_fbt.c | 1 +
> >   libdtrace/dt_provider.h | 1 +
> >   3 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 27246a40..36975406 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2595,7 +2595,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> >   	dt_irlist_t	*dlp = &pcb->pcb_ir;
> >   	dt_regset_t	*drp = pcb->pcb_regs;
> >   	uint64_t	arg;
> > -	int		nframes, stacksize, prefsz, align = sizeof(uint64_t);
> > +	int		nframes, stacksize, prefsz, skip;
> > +	int		align = sizeof(uint64_t);
> >   	uint_t		lbl_valid = dt_irlist_label(dlp);
> >   	/* Get sizing information from dnp->dn_arg. */
> > @@ -2603,6 +2604,7 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> >   	prefsz = kind == DTRACEACT_USTACK ? sizeof(uint64_t) : 0;
> >   	nframes = DTRACE_USTACK_NFRAMES(arg);
> >   	stacksize = nframes * sizeof(uint64_t);
> > +	skip = yypcb->pcb_probe->prov->impl->skip_frames;
> >   	/* Handle alignment and reserve space in the output buffer. */
> >   	if (reg >= 0) {
> > @@ -2644,7 +2646,7 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> >   		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, off + prefsz));
> >   	}
> >   	emit(dlp,  BPF_MOV_IMM(BPF_REG_3, stacksize));
> > -	emit(dlp,  BPF_MOV_IMM(BPF_REG_4, (0 & BPF_F_SKIP_FIELD_MASK)
> > +	emit(dlp,  BPF_MOV_IMM(BPF_REG_4, (skip & BPF_F_SKIP_FIELD_MASK)
> >   					  | (kind == DTRACEACT_USTACK ? BPF_F_USER_STACK : 0)));
> >   	dt_regset_xalloc(drp, BPF_REG_0);
> >   	emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_get_stack));
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 0ddffa20..dfeecfd1 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -431,6 +431,7 @@ static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> >   dt_provimpl_t	dt_fbt_fprobe = {
> >   	.name		= prvname,
> >   	.prog_type	= BPF_PROG_TYPE_TRACING,
> > +	.skip_frames	= 4,
> >   	.populate	= &populate,
> >   	.load_prog	= &fprobe_prog_load,
> >   	.trampoline	= &fprobe_trampoline,
> > diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> > index a24b1d00..bf3af0be 100644
> > --- a/libdtrace/dt_provider.h
> > +++ b/libdtrace/dt_provider.h
> > @@ -44,6 +44,7 @@ typedef struct dt_argdesc {
> >   typedef struct dt_provimpl {
> >   	const char *name;			/* provider generic name */
> >   	int prog_type;				/* BPF program type */
> > +	uint32_t skip_frames;			/* # of stack frames to skip */
> >   	int (*populate)(dtrace_hdl_t *dtp);	/* register probes */
> >   	int (*provide)(dtrace_hdl_t *dtp,	/* provide probes */
> >   		       const dtrace_probedesc_t *pdp);
> > -- 2.42.0