[DTrace-devel] [PATCH] Replace dt_memcpy() with bpf_probe_read()

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Two nits below:

On 8/20/21 2:46 AM, Kris Van Hees wrote:
> The dt_memcpy() functionality can be provided by the bpf_probe_read()
> helper.  Thanks to Eugene Loh for pointing this out.
>
> Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -725,17 +725,14 @@ dt_cg_fill_gap(dt_pcb_t *pcb, int gap)
>   static void
>   dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
>   {
> -	dt_ident_t	*idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_memcpy");
> -
> -	assert(idp != NULL);
>   	if (dt_regset_xalloc_args(drp) == -1)
>   		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>   
>   	emit(dlp,  BPF_MOV_REG(BPF_REG_1, dst));
> -	emit(dlp,  BPF_MOV_REG(BPF_REG_2, src));
> -	emit(dlp,  BPF_MOV_IMM(BPF_REG_3, size));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_3, src));
>   	dt_regset_xalloc(drp, BPF_REG_0);
> -	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> +	emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
>   	dt_regset_free_args(drp);
>   	/* FIXME: check BPF_REG_0 for error? */
>   	dt_regset_free(drp, BPF_REG_0);

Fine by me, but if you want to have consistent indentation on the emit() 
statements, here is another such opportunity.

> @@ -875,19 +872,17 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>   		 * output buffer at [%r9 + off].  The amount of bytes copied is
>   		 * the lesser of the data size and the maximum string size.
>   		 */
> -		emit(dlp,   BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> -		emit(dlp,   BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> -		emit(dlp,   BPF_MOV_REG(BPF_REG_2, dnp->dn_reg));
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
>   		dt_regset_free(drp, dnp->dn_reg);
> -		emit(dlp,   BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_2, BPF_REG_0));
>   		dt_regset_free(drp, BPF_REG_0);
> -		emit(dlp,   BPF_BRANCH_IMM(BPF_JLT, BPF_REG_3, size, vcopy));
> -		emit(dlp,   BPF_MOV_IMM(BPF_REG_3, size));
> -		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_memcpy");
> -		assert(idp != NULL);
> +		emit(dlp,  BPF_BRANCH_IMM(BPF_JLT, BPF_REG_2, size, vcopy));
> +		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
>   		dt_regset_xalloc(drp, BPF_REG_0);
> -		emitle(dlp, vcopy,
> -			    BPF_CALL_FUNC(idp->di_id), idp);
> +		emitl(dlp, vcopy,
> +			   BPF_CALL_HELPER(BPF_FUNC_probe_read));
>   		dt_regset_free_args(drp);
>   		dt_regset_free(drp, BPF_REG_0);

Okay, but we normally load func args in order from r1 to r5.  In fact, 
since you have to switch r2 and r3, you do switch the order at the other 
call site.  So, maybe do so here as well?  It means jiggling that jump 
label, but the placement of that jump label is now just a historical 
vestige.