Re: [PATCH] fbt: implement return value support for fexit-based FBT return probes

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

On Thu, Aug 01, 2024 at 02:15:17PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> with a few nits...
> 
> On 8/1/24 10:24, Kris Van Hees wrote:
> > On Thu, Aug 01, 2024 at 01:49:35AM -0400, Eugene Loh wrote:
> > > Is there a test?
> > Yes, the testsuite already contains a test (tst.return1.d) whose failure
> > prompted the need for this patch.  And with the patch, it passes.
> 
> Thanks.  So the earlier culprit/insufficient patch (fe2101e5) went out in
> the last release with this test... failing?  Not tested (on fentry systems,
> where it makes a difference)?  A known failure in the last release?  (I
> forget.)

Well, against all odds it actually "works" on older kernels with the quite
insufficient patch because by total coincidence the right value happens to be
present in the correct arg slot.

> > > On 8/1/24 01:19, Kris Van Hees wrote:
> > > > Commit fe2101e5 ("fbt: implement based on fentry/fexit probes") failed
> > > > to provide a proper implementation to pass the function return value as
> > > > arg1 for FBT return probes based on fexit probes.
> > > > 
> > > > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > > > ---
> > > >    libdtrace/dt_prov_fbt.c | 20 +++++++++++++++++++-
> > > >    1 file changed, 19 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > > > index fa888ed8..99b42586 100644
> > > > --- a/libdtrace/dt_prov_fbt.c
> > > > +++ b/libdtrace/dt_prov_fbt.c
> > > > @@ -172,6 +172,7 @@ static int populate(dtrace_hdl_t *dtp)
> > > >     */
> > > >    static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > >    {
> > > > +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> > > >    	dt_irlist_t	*dlp = &pcb->pcb_ir;
> > > >    	dt_probe_t	*prp = pcb->pcb_probe;
> > > > @@ -185,6 +186,8 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > >    			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> > > >    		}
> > > >    	} else {
> > > > +		dt_module_t	*dmp;
> > > > +
> > > >    		/*
> > > >    		 * fbt:::return arg0 should be the function offset for the
> > > >    		 * return instruction.  The fexit prpbe fires at a point where
> 
> Might as well s/prpbe/probe/ while we're here.

Good catch.

> > > > @@ -194,6 +197,21 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > >    		 */
> > > >    		dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
> > > >    		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> > > > +
> > > > +		/*
> > > > +		 * The return value is provided by the fexit probe as an
> > > > +		 * argument slot past the last function argument.  We can get
> > > > +		 * the number of function arguments using the BTF id that has
> > > > +		 * been stored as the tracepoint event id.
> > > > +		 */
> > > > +		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
> > > > +		if (dmp != NULL) {
> > > > +			int32_t	btf_id = dt_tp_get_event_id(prp);
> > > > +			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
> > > > +
> > > > +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, i * 8));
> > > > +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
> > > > +		}
> 
> I assume we don't worry about the handling of dmp==NULL because it's
> "unlikely" (already been checked) and there's nothing more sensible to do
> anyhow?  Or load arg1 with 0xdeadbeef or something?

Unpredictable result is probably the best we can do right now.  Any deliberatea
value isn't really any better.
 
We need to look into a future solution to provide proper error reporting
during the trampoline creation.

> > > >    	}
> > > >    	dt_cg_tramp_epilogue(pcb);
> > > > @@ -274,7 +292,7 @@ static int fprobe_prog_load(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > > >    	dt_module_t			*dmp;
> > > >    	atype = strcmp(desc->prb, "entry") == 0 ? BPF_TRACE_FENTRY
> > > > -						     : BPF_TRACE_FEXIT;
> > > > +						: BPF_TRACE_FEXIT;
> > > >    	dmp = dt_module_lookup_by_name(dtp, desc->mod);
> > > >    	if (dmp == NULL)