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

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at 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.)

>> 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 at 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.

>>> @@ -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?

>>>    	}
>>>    	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)